Is catching TaskCanceledException and checking Task.Canceled a good idea?

Question

There are some people on my team who really love coding with async Task. And sometimes they like to use CancellationToken parameters.

What I'm unsure about is whether we should as a team be using this style of code (A):

async Task<someObject> DoStuff(CancellationToken t)
{
    while (!t.IsCanceled)
    {
        try {
            Task.Delay(5000, t);
        }
        catch (AggregateException e) // or is it TaskCanceledException or OperationCanceledException? I don't know? :)
        {
        }
        // poll something, return someObject, or null
    }
    return null;
}

This obviously means the caller probably has to check the cancellation token themselves to determine whether to continue processing, and they might have to handle null retVals:

var retVal = await DoStuff(token);
if (token.IsCanceled) { ... }

However, if we adopt a second style of code (B) that relies on TaskCanceledException:

async Task<someObject> DoStuff(CancellationToken t)
{
    while(true)
    {
        Task.Delay(5000, t);
        // poll something, return someObject, or null
    }
}

The implementation code is definitely simpler - and the caller has the option of handling the exception or not, as appropriate... but I can't help worrying that callers might forget that TaskCanceledException is something they have to worry about, and processes may crash as a result of them not catching these exceptions (on foreground or background threads).

So, my overly optimistically phrased question is: which do you think is the best style that everyone should always use, and why? :)

Solution

In the .Net framework itself when you pass a CancellationToken as a parameter you will get back a TaskCanceledException. I would not go against that and create my own design pattern because people who are familiar with .Net will be familiar with your code.

My guideline is this: The one that cancels the token is the one that should handle the TaskCanceledException, so If you're using a CancellationToken inside your method for your own reasons, go ahead and use a try-catch block. But if you get the token as a parameter, let the exception be thrown.

Comments

[clarkitect]

I'm interested in the answer(s) to this, and I have my own as well, but I suspect this question is soliciting opinion which is not exactly considered on-topic for SO. Have you considered requesting moderator assistance in moving this to codereview instead?

[Tim Lovell-Smith]

I think opinion is totally on-topic for SO in the case of 'what in your opinion is good programming practice'.

[Tim Lovell-Smith]

I also do not consider this a code review question as it's targeted at a particular topic, it's not 'tell me how to code this bettr'.

[clarkitect]

I didn't feel that strongly about it; it just looked similar to subjective question(s) that have been flagged in the past. I'm glad it's getting good discussion :)

[noseratio]

@TimLovell-Smith, you should never swallow AggregateException like in your code, without inspecting inner exceptions. Also, it can be either OperationCanceledException or TaskCanceledException. Check this document: Using Cancellation Support in .NET 4.0.

[Tim Lovell-Smith]

Great answer for including a coding guideline for ensuring that exceptions get handled!

[Ricibob]

Good answer - but would be nice to see some code examples in the same vein as the org question.

[Corio]

Also, it's generally a good practice to catch OperationCanceledException as stated in the "Recommended patterns for CancellationToken" article ("Handling cancellation exceptions" section).