Runtime: Task.WhenAll - Inner exceptions are lost

[gravity00]

Hi,

Summary: when await Task.WhenAll(tasks) if more than one task fails, only one exception is thrown and I believe an AggregateException should be thrown instead.

Explanation:

I know that await only throws the first exception present in task.Exception and found the racional behind it (it makes sense considering the options). My problem is not with that, but the fact that a method that waits for multiple tasks doesn't throw himself an AggregateException.

By using the TaskCompletionSource<T>.SetException(IEnumerable<Exception>) they'll all be appended to the task's exception and when an await happens, only the first one is thrown. If instead it used something like tcs.SetException(new AggregateException(exceptions)), no exceptions would be lost.

I believe this problem arises much sooner in the TPL API, because I always found it odd that a Task as an AggregateException instead of just an Exception with the exact exception thrown by the underline code, for the same reason it only has a single result e not an AggregateResult. I'm certain there's some rational behind it but I found nothing.

Here's a simple code snippet, simulating the Task.WhenAll behavior that should give the idea:

using System;
using System.Collections.Concurrent;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;

namespace SemaphoreSlimTests
{
    public static class Program
    {
        public static async Task Main(string[] args)
        {
            Console.WriteLine("Application started...");
            Console.WriteLine();

            const int maxTasks = 10;

            var tasks = Enumerable
                .Range(0, maxTasks)
                .Select(async i =>
                {
                    await Task.Yield();
                    if (i % 3 == 0)
                        throw new Exception($"Simulated exception [{i}]");
                    await Task.Delay(i % 3 * 500);
                })
                .ToArray();

            Console.WriteLine("--- WhenAll_SetExceptionCollection ---");
            try
            {
                await TaskEx.WhenAll_SetExceptionCollection(tasks);
            }
            catch (Exception e)
            {
                Console.ForegroundColor = ConsoleColor.Red;
                Console.WriteLine(e);
                Console.ResetColor();
            }
            Console.WriteLine();

            Console.WriteLine("--- WhenAll_InnerAggregateException ---");
            try
            {
                await TaskEx.WhenAll_InnerAggregateException(tasks);
            }
            catch (Exception e)
            {
                Console.ForegroundColor = ConsoleColor.Red;
                Console.WriteLine(e);
                Console.ResetColor();
            }
            Console.WriteLine();

            Console.WriteLine("Application terminated. Press <enter> to exit...");
            Console.ReadLine();
        }
    }

    public static class TaskEx
    {
        public static Task WhenAll_SetExceptionCollection(Task[] tasks)
        {
            var tcs = new TaskCompletionSource<bool>();

            var totalTasks = tasks.Length;
            var exceptions = new ConcurrentBag<Exception>();
            foreach (var task in tasks)
            {
                task.ContinueWith(t =>
                {
                    if (t.IsFaulted)
                        exceptions.Add(t.Exception.InnerException);

                    if (Interlocked.Decrement(ref totalTasks) == 0)
                    {
                        if (exceptions.Count == 0)
                            tcs.SetResult(true);
                        else
                        {
                            // this is the current Task.WhenAll behavior
                            // the underline Task will have it's Exception property
                            // filled with all these as inner exceptions
                            // but when we await only the first one will be thrown
                            tcs.SetException(exceptions);
                        }
                    }
                });
            }

            return tcs.Task;
        }

        public static Task WhenAll_InnerAggregateException(Task[] tasks)
        {
            var tcs = new TaskCompletionSource<bool>();

            var totalTasks = tasks.Length;
            var exceptions = new ConcurrentBag<Exception>();
            foreach (var task in tasks)
            {
                task.ContinueWith(t =>
                {
                    if (t.IsFaulted)
                        exceptions.Add(t.Exception.InnerException);

                    if (Interlocked.Decrement(ref totalTasks) == 0)
                    {
                        if (exceptions.Count == 0)
                            tcs.SetResult(true);
                        else
                        {
                            // this is the proposed behaviour with the following racional:
                            // a bunch of exception have been thrown, so here's an 
                            // AggregateException with all of them and when you
                            // await this task, you'll receive all of them
                            tcs.SetException(new AggregateException(exceptions));
                        }
                    }
                });
            }

            return tcs.Task;
        }
    }
}

Application started...

--- WhenAll_SetExceptionCollection ---
System.Exception: Simulated exception [9]
   at SemaphoreSlimTests.Program.<>c.<<Main>b__0_0>d.MoveNext() in C:\Dev-Playground\SemaphoreSlimTests\SemaphoreSlimTests\Program.cs:line 25
--- End of stack trace from previous location where exception was thrown ---
   at SemaphoreSlimTests.Program.Main(String[] args) in C:\Dev-Playground\SemaphoreSlimTests\SemaphoreSlimTests\Program.cs:line 32

--- WhenAll_InnerAggregateException ---
System.AggregateException: One or more errors occurred. (Simulated exception [6]) (Simulated exception [9]) (Simulated exception [0]) (Simulated exception [3])
 ---> System.Exception: Simulated exception [6]
   at SemaphoreSlimTests.Program.<>c.<<Main>b__0_0>d.MoveNext() in C:\Dev-Playground\SemaphoreSlimTests\SemaphoreSlimTests\Program.cs:line 25
   --- End of inner exception stack trace ---
   at SemaphoreSlimTests.Program.Main(String[] args) in C:\Dev-Playground\SemaphoreSlimTests\SemaphoreSlimTests\Program.cs:line 45
 ---> (Inner Exception dotnet/corefx#1) System.Exception: Simulated exception [9]
   at SemaphoreSlimTests.Program.<>c.<<Main>b__0_0>d.MoveNext() in C:\Dev-Playground\SemaphoreSlimTests\SemaphoreSlimTests\Program.cs:line 25
--- End of stack trace from previous location where exception was thrown ---
   at SemaphoreSlimTests.Program.Main(String[] args) in C:\Dev-Playground\SemaphoreSlimTests\SemaphoreSlimTests\Program.cs:line 32<---

 ---> (Inner Exception dotnet/corefx#2) System.Exception: Simulated exception [0]
   at SemaphoreSlimTests.Program.<>c.<<Main>b__0_0>d.MoveNext() in C:\Dev-Playground\SemaphoreSlimTests\SemaphoreSlimTests\Program.cs:line 25<---

 ---> (Inner Exception dotnet/corefx#3) System.Exception: Simulated exception [3]
   at SemaphoreSlimTests.Program.<>c.<<Main>b__0_0>d.MoveNext() in C:\Dev-Playground\SemaphoreSlimTests\SemaphoreSlimTests\Program.cs:line 25<---


Application terminated. Press <enter> to exit...

[stephentoub]

when await Task.WhenAll(tasks) if more than one task fails, only one exception is thrown and I believe an AggregateException should be thrown instead

I understand the desire, but that's not the design (it actually was this way early on and we changed it based on explicit feedback), and it would be a big breaking change to change this.

You can create your own version that does this if you want, or just wrap it, e.g.
C# Task t = Task.WhenAll(...); try { await t; } catch { throw t.Exception; }

[gravity00]

Thanks for the feedback @stephentoub

I understand the breaking change, it would certainly lead to some unexpected behavior for clients depending on it, but the thing is: considering how many people probably don't know about this behavior shouldn't this be discussed in dept at least?

I know I can create some helpers, but it is one of those things that should just work imo.

Discovering this after seeing 3 failed HTTP request in Microsoft Insights but only a single exception in the logs, spending hours and then realizing the problem was with Task.WhenAll, and now having to search in all of our code base for similar usages and replacing for some wrapper, certainly is something Microsoft can understand that it may be a design problem?

Kinda like disposing WCF services that would throw an exception if it was on a faulted state and Abort was not invoked, the original exception would be lost since a new one would be thrown? If I were not told about this, I would just using var service = new /* some service */ and, with some luck, would discover this problem on production?

[stephentoub]

shouldn't this be discussed in dept at least?

The point I was trying to make is that it was discussed in depth, years ago, when these were originally added. We originally did what you're suggesting, with the Task returned from WhenAll containing a single AggregateException that contained all the exceptions, i.e. task.Exception would return an AggregateException wrapper which contained another AggregateException which then contained the actual exceptions; then when it was awaited, the inner AggregateException would be propagated. The strong feedback we received that caused us to change the design was that a) the vast majority of such cases had fairly homogenous exceptions, such that propagating all in an aggregate wasn't that important, b) propagating the aggregate then broke expectations around catches for the specific exception types, and c) for cases where someone did want the aggregate, they could do so explicitly with the two lines like I wrote. We also had extensive discussions about what the behavior of await sould be with regards to tasks containing multiple exceptions, and this is where we landed.

I get that this isn't what you expected, but the design you're suggesting is what others told us they didn't expect. And as this has now been the behavior for a long time, I don't see a strong reason to change.

Regardless, thank you for the feedback.

[gravity00]

b) propagating the aggregate then broke expectations around catches for the specific exception types

If I only get the first exception, how does this behavior solve that problem? As far as we know, the catch will get whichever exception is added first to the internal list. If I have the following code only the first catch will ever be executed, despite having two different exceptions, one will be just lost:

try
{
    // imagine these were HTTP requests
    await Task.WhenAll(Task.Run(async () =>
    {
        await Task.Yield();
        throw new HttpRequestException();
    }), Task.Run(async () =>
    {
        await Task.Yield();
        throw new TimeoutException();
    }));
}
catch (HttpRequestException e)
{
    Console.WriteLine(e);
}
catch (TimeoutException e)
{
    Console.WriteLine(e);
}
catch (Exception e)
{
    Console.WriteLine(e);
}

I'm still trying to understand how the initial design was changed to a behavior that clearly has major design flaws in it, specially by making it extremely easy to lose exceptions.

How can someone expect to await for a collection of tasks to terminate and not receive a collection of exceptions in case of one or more fails? There is a big difference between running code in parallel or awaiting each individually with an associated try-catch.

We also had extensive discussions about what the behavior of await should be with regards to tasks containing multiple exceptions, and this is where we landed

For me that behavior is the correct and don't see any problem with it. Maybe all this confusion started because Tasks have an AggregateException instead of just an Exception? What was the racional behind it? I sincerely can't find a discussion about it and would like to understand more.

[stephentoub]

If I only get the first exception, how does this behavior solve that problem?

It solves a problem because it hits one of the catch blocks that had been established, vs not hitting any of them. And as mentioned, the majority case we saw was where, if there were multiple exceptions, they were generally either all of the same type or at least generally all related to the same failure condition, so handling just one was sufficient.

that clearly has major design flaws in it

I explained. I understand you don't like the behavior. Others felt similarly about the behavior you're desiring.

I sincerely can't find a discussion

This was all well before there were open source repos for .NET.

At this point, I do not see this behavior changing. Since you find it unacceptable for your needs, I suggest you write your own WhenAll variant that has the desired behavior, either as a two-line wrapper for the built-in WhenAll like I showed above, or alternatively "from scratch" built on top of ContinueWith or the like. If you want to enforce that your developers in your codebases use it instead, you can also write a Roslyn analyzer that flags occurrences of Task.WhenAll and a fixer that replaces them with calls to your own method.

Thanks.

[gravity00]

The Roslyn analyzer is a good point, specially because we should be able to come up with something relatively fast. Thanks for the reminder!

Since the behavior isn't expected to change, I'm closing this bug.

Thanks for the insights and explanation, appreciated.

[noseratio]

I came across this as I faced the same issue as @gravity00.

To add to @stephentoub's explanation, this behavior is not specific to Task.WhenAll, but rather is how AggregateException gets unwrapped for any await continuation.

Unfortunately, a simple solution like try { await t; } catch { throw t.Exception; } has a pitfall: it doesn't propagate the cancellation status to outer catch blocks, i.e. the task become faulted (vs canceled).

I've gone into a bit more details and posted an alternative solution here on SO. In a nutshell, this is what works for me:

public static Task WithAggregatedExceptions(this Task @this)
{
return @this.ContinueWith(ante =>
ante.IsFaulted &&
(ante.Exception.InnerExceptions.Count > 1 ||
ante.Exception.InnerException is AggregateException) ?
Task.FromException(ante.Exception.Flatten()) :
ante,
TaskContinuationOptions.ExecuteSynchronously).Unwrap();
}