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

Question

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?

    /// <summary>
    /// Handles any exceptions on this task, and executes action on specified scheduler.
    /// </summary>
    /// <param name="task">The task.</param>
    /// <param name="exceptionHandler">The exception handler.</param>
    /// <param name="finalAction">The final action.</param>
    /// <param name="scheduler">The scheduler.</param>
    public static void Finally(this Task task, Action<Exception> 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);
    }

Answer 1

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<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);
}

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<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;
    }
}
}

Answer 2

A couple of personal preferences here:

• Allow for no scheduler to be passed in and use the default scheduler in that case.
• Allow for no exceptionHandler if the caller is certain no exceptions will occur.
• Check that task isn't null and throw appropriate exception if it is.
• Check that t.Exception isn't null before using to get the innerException.
• Have the method return the passed in Task so it can be fluently chained.

Refinagled code:

/// <summary>
/// Handles any exceptions on this task, and executes action on specified scheduler.
/// </summary>
/// <param name="task">The task.</param>
/// <param name="exceptionHandler">The exception handler.</param>
/// <param name="finalAction">The final action.</param>
/// <param name="scheduler">The scheduler.</param>
/// <returns>The passed-in task</returns>
public static Task Finally(this Task task, Action finalAction, 
                                           Action<Exception> exceptionHandler = null, TaskScheduler scheduler = null)
{
    if (task == null) throw new ArgumentNullException("task");
    if (scheduler == null) scheduler = TaskScheduler.Default;
    task.ContinueWith(t =>
    {
        if(finalAction != null) finalAction();

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

That way, you can write simplified calls such as:

var t = Task.Factory
    .StartNew(() => Console.WriteLine("Started"))
    .Finally(() => Console.WriteLine("Finished"));