Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
    }
share|improve this question
    
PS: I think I wrote this. It was a while back, so it may be that I filched it from somewhere else. –  Benjol Feb 11 '13 at 10:20

2 Answers 2

up vote 4 down vote accepted

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;
    }
}
}
share|improve this answer
    
You're right. I actually realised this since posting this question. I suddenly noticed that I didn't 'need' a catch, and then saw why. –  Benjol Feb 17 '13 at 20:35

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"));
share|improve this answer
    
Thanks. I omitted two overloads, which cover the no scheduler, no finalAction cases, but not (yet) the no exceptionHandler. Point taken about the exception being null, though unless I'm mistaken, in your rewriting, it will still choke on the following line if it is null. –  Benjol Feb 12 '13 at 5:56
    
Well, depends on the code the caller has written in exceptionHandler - your null-coalescing operator (??) may give null but the line itself won't choke. –  Jesse C. Slicer Feb 12 '13 at 13:13
    
OK, fair point. –  Benjol Feb 12 '13 at 14:40
    
Should the default scheduler be TaskScheduler.Default or TaskScheduler.Current? Task.Factory.StartNew() uses Current, Task.Run() Default. I'm not sure which one is better in this case. –  svick Nov 21 '13 at 18:56
    
How do we summon Stephen Toub here? –  Jesse C. Slicer Nov 21 '13 at 19:02

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.