Suggestion - Preserve all exceptions

[michael-wolfenden]

As mention in the following tweet

Do you know that you can easily lose exceptions by awaiting a faulted task that has more than one error? For instance by combining them via Task.WaitAll

An example is:

async Task Main()
{
    try
    {
        await Task.WhenAll(A(), B());
    }
    catch (Exception e)
    {
        // Exception here is InvalidOperationException("A")
        // The second exception is lost
        Console.Write(e)
    }
}

public async Task<int> A() { throw new InvalidOperationException("A"); }
public async Task<int> B() { throw new InvalidOperationException("B"); }

TaskTupleAwaiter also exhibits the same behaviour

async Task Main()
{
    try
    {
        var (a, b) = await(A(), B());
    }
    catch (Exception e)
    {
        // Exception here is InvalidOperationException("A")
        // The second exception is lost
        Console.Write(e)
    }
}

public async Task<int> A() { throw new InvalidOperationException("A"); }
public async Task<int> B() { throw new InvalidOperationException("B"); }

Sergey proposes the PreserveAllExceptions task extension, that can be chained onto the WhenAll, i.e. await Task.WhenAll(A(), B()).PreserveAllExceptions(); that will wrap any original AggregateExceptions into another one if more then one error occurred.

public static Task PreserveAllExceptions(this Task task)
{
    return task.ContinueWith(t =>
    {
        // If the task failed with more than 1 error,
        // then create another task from the aggregate exception.
        if (t.IsFaulted && t.Exception.InnerExceptions.Count > 1)
        {
            return Task.FromException(t.Exception);
        }

        // Otherwise just return the original task
        return t;
    })
    .Unwrap(); // Unwraping the task to get a faulted task if need
}

Would it be possible to add the PreserveAllExceptions method to all the WhenAll calls made internally within this library so that all exceptions are preserved?

I would be happy to submit a pull request.

[jnm2]

This was an intentional design decision around await by the C# team, so I would not want await (x, y); to start changing behavior and throwing AggregateException sometimes when it used to throw other exception types. However, it would make sense to have await (x, y).PreserveAllExceptions(); add this extra exception-handling complexity and performance overhead.

This is nearly the worst of both worlds, too. Unlike Task.Result, you don't always catch AggregateException, and unlike await task you don't always catch the thrown exception type. You have to think of both catch clauses and there's nothing to remind you when you forget.

try
{
    await Task.WhenAll(x, y).PreserveAllExceptions();
}
catch (SomeException ex)
{
    // Handle SomeException
}
catch (AggregateException aggregateEx) when (
    aggregateEx.InnerExceptions.OfType<SomeException>().TrySingle(out var ex))
{
    // Handle SomeException
    // But don't forget to handle aggregateEx.InnerExceptions[0], too.
}

It encourages catching AggregateException which leads you in the direction of the catch-all antipattern. For something top-level like a Cake script this would make sense. But for a structured application, it would typically force you to handle exceptions far too low, preventing the responsibility from being handled with the proper context higher up the call stack.

/cc @SergeyTeplyakov

[sharwell]

Would it be possible to add the PreserveAllExceptions method to all the WhenAll calls made internally within this library so that all exceptions are preserved?

I would be interested in the particulars of this. If the only impact of this is enabling a PreserveAllExceptions extension method to throw the correct AggregateException, it seems like a reasonable plan. Be careful not to change the behavior for other users though.

that will wrap any original AggregateExceptions into another one if more then one error occurred.

Wrapping the exception is unnecessary overhead. Just use a custom struct awaiter based on TaskAwaiter.

[michael-wolfenden]

It encourages catching AggregateException which leads you in the direction of the catch-all antipattern.

Actually, isn't this the only way to ensure the correct behaviour.

For example if we need to provide some compensating action when a SqlException is thrown

try
{
    var (a, b) = (
        ThrowsArgumentNullException(),
        ThrowsSqlException()
    );
}
catch (SqlException ex)
{
    // This will never be throw in this scenario.
    // Even worse is that this behavior is dependent
    // on the ordering of the tasks
}

[jnm2]

@michael-wolfenden But even in the scenario you're showing there, the other exception is an ArgumentNullException. A contract was invalidated; the program is fundamentally broken to an unknown degree and should fail fast. Therefore no compensating action would be needed for the SqlException.

Or, let's say the first exception is an OperationCancelledException. Since you're in a wait-all, that means you definitely want to cancel without any results. Yet again, no compensating action.

I'm just having a hard time thinking of a real-world business case to need to know about more than one of the exceptions. I've used async and WhenAll heavily and diagnosed many a confusing exception log; logging secondary errors can obscure the real problem. If the two exceptions truely are unrelated and have different root causes, you'll discover the second as soon as you fix the first.

[OlegKarasik]

@michael-wolfenden

Catching all exception on the top level doesn't help much. The thing here is that in you example:

try
{
    var (a, b) = (
        ThrowsArgumentNullException(),
        ThrowsSqlException()
    );
}
catch (SqlException ex)
{
    // This will never be throw in this scenario.
    // Even worse is that this behavior is dependent
    // on the ordering of the tasks
}

The ThrowsSqlException as well as ThrowsArgumentNullException should understand concepts like logging, tracing and resource ownership so on the top level isn't really matters what kind of exception occurred.

In scenarios where you really care about handling all the exceptions it is better to use more flexible approach that would not only allow you to handle each exception in the place it should be but also to gather as much information as possible.

Here is simple example of such approach:

class ExceptionInfo
{
  TaskMetadata Metadata { get; } // Task metadata also can be used to identify the Task
  Exception Exception { get; }
}

class ExceptionTracker
{
  public void Track(TaskMetadata t, Exception e) { ... }
  public IReadOnlyList<ExceptionInfo> GetExceptions() { ... }
}

async Task Main()
{
  var exceptionTracker = new ExceptionTracker();

  await Task.WhenAll(A(exceptionTracker), B(exceptionTracker));

  var exceptions = exceptionTracker.GetExceptions();
  // analyze and decide what to do.
}

public async Task<int> A(ExceptionTracker exceptionTracker) 
{
  exceptionTracker.Track(new TaskMetadata(), new InvalidOperationException("A"));
}

public async Task<int> B(ExceptionTracker exceptionTracker) 
{ 
  exceptionTracker.Track(new TaskMetadata(), new InvalidOperationException("B"));
}

[jnm2]

Because of the pitfalls I mentioned above, we want to stay true to the C# language and BCL design of suppressing all but the first exception when a task is awaited.

I like Sam's suggestion of a PreserveAllExceptions() extension method that returns an awaitable that always throws AggregateException when there are more than one or (particularly) exactly one exception. It can be done similarly to how I did the ConfigureAwait extension methods. I think this needs to be prototyped and hardened in the field before adding to the library. Fortunately, this is an easy thing to do.

Thanks for the good discussion and I'm sorry I can't say yes. :-(