HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Task.Finally extension, good, bad, or ugly?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
uglybadextensiongoodfinallytask

Problem

I wrote this, and it has helped to avoid the 'Some exception weren't handled' problem. Is there something glaringly wrong with this that I might have missed?

/// 
    /// Handles any exceptions on this task, and executes action on specified scheduler.
    /// 
    /// The task.
    /// The exception handler.
    /// The final action.
    /// The scheduler.
    public static void Finally(this Task task, Action exceptionHandler, 
                                               Action finalAction, TaskScheduler scheduler)
    {
        task.ContinueWith(t =>
        {
            if(finalAction != null) finalAction();

            if(t.IsCanceled || !t.IsFaulted || exceptionHandler == null) return;
            var innerException = t.Exception.Flatten().InnerExceptions.FirstOrDefault();
            exceptionHandler(innerException ?? t.Exception);
        }, scheduler);
    }

Solution

I think what you've really got here is two extension methods hiding inside one. Your method is doing the work of both a "finally" and a "catch", and I recommend refactoring it into two methods. The catch method may look something like this:

Edit: clearly I missed checking the exception type before executing the action, but I'm away from a computer and trying to edit code with my iPhone isn't working too well.

public static void Catch(this Task task, Action exceptionHandler, 
                                     TaskScheduler scheduler = null) where TException : Exception
{
    if (exceptionHandler == null)
        throw new ArgumentNullException("exceptionHandler cannot be null");

    task.ContinueWith(t =>
    {
        if(t.IsCanceled || !t.IsFaulted || t.Exception == null) return;
        var innerException = t.Exception.Flatten().InnerExceptions.FirstOrDefault();
        exceptionHandler(innerException ?? t.Exception);
    }, scheduler ?? TaskScheduler.Default);
}


Edit: Finally got time to get back to this. Here's an example of a complete solution:

using System;
using System.Threading.Tasks;

namespace ConsoleApplication1
{
class Program
{
    static void Main(string[] args)
    {
        Task.Run(() => {
                Console.WriteLine("In the Task");
                throw new ArgumentException("Thrown from task");
                //throw new Exception("Thrown from task");
            })
            .Catch((e) => Console.WriteLine("Caught Exception {0}", e))
            .Catch((ae) => Console.WriteLine("Caught Argument Excetion {0}", ae))
            .Finally(() => Console.WriteLine("in the Finally"));

        Console.ReadKey();
        Console.ReadKey();
    }
}

static class TaskExtensions
{

    public static void Finally(this Task task, Action finalAction, 
                               TaskScheduler scheduler = null)
    {
        if (finalAction == null)
            throw new ArgumentNullException("finalAction cannot be null");

        task.ContinueWith(t => finalAction(), scheduler ?? TaskScheduler.Default);
    }

    public static Task Catch(this Task task, Action exceptionHandler,
                                         TaskScheduler scheduler = null) where TException : Exception
    {
        if (exceptionHandler == null)
            throw new ArgumentNullException("exceptionHandler cannot be null");

        task.ContinueWith(t =>
        {
            if (t.IsCanceled || !t.IsFaulted || t.Exception == null) 
                return;

            var exception =
                t.Exception.Flatten().InnerExceptions.FirstOrDefault() ?? t.Exception;

            if (exception is TException)
            {
                exceptionHandler((TException) exception);
            }
        }, scheduler ?? TaskScheduler.Default);

        return task;
    }
}
}

Code Snippets

public static void Catch<TException>(this Task task, Action<TException> exceptionHandler, 
                                     TaskScheduler scheduler = null) where TException : Exception
{
    if (exceptionHandler == null)
        throw new ArgumentNullException("exceptionHandler cannot be null");

    task.ContinueWith(t =>
    {
        if(t.IsCanceled || !t.IsFaulted || t.Exception == null) return;
        var innerException = t.Exception.Flatten().InnerExceptions.FirstOrDefault();
        exceptionHandler(innerException ?? t.Exception);
    }, scheduler ?? TaskScheduler.Default);
}
using System;
using System.Threading.Tasks;

namespace ConsoleApplication1
{
class Program
{
    static void Main(string[] args)
    {
        Task.Run(() => {
                Console.WriteLine("In the Task");
                throw new ArgumentException("Thrown from task");
                //throw new Exception("Thrown from task");
            })
            .Catch<Exception>((e) => Console.WriteLine("Caught Exception {0}", e))
            .Catch<ArgumentException>((ae) => Console.WriteLine("Caught Argument Excetion {0}", ae))
            .Finally(() => Console.WriteLine("in the Finally"));

        Console.ReadKey();
        Console.ReadKey();
    }
}

static class TaskExtensions
{

    public static void Finally(this Task task, Action finalAction, 
                               TaskScheduler scheduler = null)
    {
        if (finalAction == null)
            throw new ArgumentNullException("finalAction cannot be null");

        task.ContinueWith(t => finalAction(), scheduler ?? TaskScheduler.Default);
    }

    public static Task Catch<TException>(this Task task, Action<TException> exceptionHandler,
                                         TaskScheduler scheduler = null) where TException : Exception
    {
        if (exceptionHandler == null)
            throw new ArgumentNullException("exceptionHandler cannot be null");

        task.ContinueWith(t =>
        {
            if (t.IsCanceled || !t.IsFaulted || t.Exception == null) 
                return;

            var exception =
                t.Exception.Flatten().InnerExceptions.FirstOrDefault() ?? t.Exception;

            if (exception is TException)
            {
                exceptionHandler((TException) exception);
            }
        }, scheduler ?? TaskScheduler.Default);

        return task;
    }
}
}

Context

StackExchange Code Review Q#21568, answer score: 4

Revisions (0)

No revisions yet.