Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure an await to propagate all errors by throwing an AggregateException #47605

Closed
theodorzoulias opened this issue Jan 28, 2021 · 13 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading.Tasks

Comments

@theodorzoulias
Copy link
Contributor

Let's throw another marginally useful idea for configuring awaits.

Background and Motivation

The await operator currently propagates only the first of the exceptions stored in a Task object. This is convenient in the vast majority of cases, because most asynchronous APIs by design will never complete with more than one errors. There is a handful of APIs though that do propagate multiple exceptions quite normally, and most of this information is lost when these APIs are awaited in the standard way. Although it is possible for a programmer to observe and handle all errors in this case, the workaround needed is slightly cumbersome and error prone.

Proposed API

A new value ThrowAggregateException for the AwaitBehavior enum, which serves as a parameter for the ConfigureAwait method. This is an extension of the #47525 proposal.

namespace System.Threading.Tasks
{
    [Flags]
    public enum AwaitBehavior
    {
        //...
        ThrowAggregateException = 0x8, // Instead of throwing only the first exception
    }
}

Usage Examples

This configuration could be used when awaiting the Task.WhenAll method, the upcoming Parallel.ForEachAsync method, and the Completion property of the TPL Dataflow blocks:

try
{
    await Task.WhenAll(tasks).ConfigureAwait(AwaitBehavior.ThrowAggregateException);
}
catch (AggregateException aex)
{
    aex.Handle(ex => { /* ... */ });
}

The current workaround is to store the task in a variable before awaiting it, and in case of
exception to query the task's Exception property:

var whenAllTask = Task.WhenAll(tasks);
try
{
    await whenAllTask;
}
catch (Exception ex)
{
    if (whenAllTask.IsFaulted)
    {
        whenAllTask.Exception.Handle(ex => { /* ... */ });
    }
    else
    {
        // handle ex, which is certainly an OperationCanceledException
    }
}

Alternative Designs

An extension method WithAggregateException has been suggested in a StackOverflow question:

public static async Task WithAggregateException(this Task source)
{
    try
    {
        await source.ConfigureAwait(false);
    }
    catch
    {
        if (source.Exception == null) throw;
        ExceptionDispatchInfo.Capture(source.Exception).Throw();
    }
}

Risks

None that I can think of.

@theodorzoulias theodorzoulias added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 28, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Threading.Tasks untriaged New issue has not been triaged by the area owner labels Jan 28, 2021
@ghost
Copy link

ghost commented Jan 28, 2021

Tagging subscribers to this area: @tarekgh
See info in area-owners.md if you want to be subscribed.

Issue Details

Let's throw another marginally useful idea for configuring awaits.

Background and Motivation

The await operator currently propagates only the first of the exceptions stored in a Task object. This is convenient in the vast majority of cases, because most asynchronous APIs by design will never complete with more than one errors. There is a handful of APIs though that do propagate multiple exceptions quite normally, and most of this information is lost when these APIs are awaited in the standard way. Although it is possible for a programmer to observe and handle all errors in this case, the workaround needed is slightly cumbersome and error prone.

Proposed API

A new value ThrowAggregateException for the AwaitBehavior enum, which serves as a parameter for the ConfigureAwait method. This is an extension of the #47525 proposal.

namespace System.Threading.Tasks
{
    [Flags]
    public enum AwaitBehavior
    {
        //...
        ThrowAggregateException = 0x8, // Instead of throwing only the first exception
    }
}

Usage Examples

This configuration could be used when awaiting the Task.WhenAll method, the upcoming Parallel.ForEachAsync method, and the Completion property of the TPL Dataflow blocks:

try
{
    await Task.WhenAll(tasks).ConfigureAwait(AwaitBehavior.ThrowAggregateException);
}
catch (AggregateException aex)
{
    aex.Handle(ex => { /* ... */ });
}

The current workaround is to store the task in a variable before awaiting it, and in case of
exception to query the task's Exception property:

var whenAllTask = Task.WhenAll(tasks);
try
{
    await whenAllTask;
}
catch (Exception ex)
{
    if (whenAllTask.IsFaulted)
    {
        whenAllTask.Exception.Handle(ex => { /* ... */ });
    }
    else
    {
        // handle ex, which is certainly an OperationCanceledException
    }
}

Alternative Designs

An extension method WithAggregateException has been suggested in a StackOverflow question:

public static async Task WithAggregateException(this Task source)
{
    try
    {
        await source.ConfigureAwait(false);
    }
    catch
    {
        if (source.Exception == null) throw;
        ExceptionDispatchInfo.Capture(source.Exception).Throw();
    }
}

Risks

None that I can think of.

Author: theodorzoulias
Assignees: -
Labels:

api-suggestion, area-System.Threading.Tasks, untriaged

Milestone: -

@tarekgh
Copy link
Member

tarekgh commented Jan 28, 2021

CC @stephentoub

@stephentoub
Copy link
Member

stephentoub commented Jan 29, 2021

There are certainly cases where it'd be helpful, but they're in the vast minority, and it wouldn't really save much code. We're talking about the difference between:

await t.ConfigureAwait(AwaitBehavior.ThrowAggregateException);

and

await t.ConfigureAwait(AwaitBehavior.NoThrow);
t.Wait();

and if all you want is access to the exceptions, you don't even need that second line, just access t.Exception.

Given the infrequency with which this would be needed and the trivial ability to get the behavior with one more line (if that), it doesn't seem warranted to pay the costs of building it in.

@theodorzoulias
Copy link
Contributor Author

await t.ConfigureAwait(AwaitBehavior.NoThrow);
t.Wait();

Thanks for this workaround, it is good enough for me. The AwaitBehavior.NoThrow opens new possibilities for solving old problems. I'm closing this up.

@Kryptos-FR
Copy link

@stephentoub will the code above work with ValueTask? I thought that awaiting more than once could be an issue.

Or does it mean we need to first convert it to a Task? Where is it better to do it?

// before the first await
Task<T> t = MethodAsync().AsTask();
await t.ConfigureAwait(AwaitBehavior.NoThrow);
t.Wait();

// just before Wait()
ValueTask<T> t = MethodAsync();
await t.ConfigureAwait(AwaitBehavior.NoThrow);
t.AsTask().Wait();

@stephentoub
Copy link
Member

will the code above work with ValueTask?

Technically, yes (with .GetAwaiter().GetResult() instead of .Wait()). We'll need to come up with a good way to more specifically describe what's allowed. You can only hook up one continuation (.GetAwaiter().{Unsafe}OnCompleted) and only use GetResult() once; NoThrow will do the former but not the latter, so it's ok to then do the latter once after.

@noseratio
Copy link

Hey @theodorzoulias, long time no no talk! :)
I was dealing with this problem too, I agree sometimes it's desirable to await without unwrapping of AggregateException. I think I've come with an handy extension method for that, WithAggregatedExceptions.

@theodorzoulias
Copy link
Contributor Author

Hi @noseratio! Thanks for pinging. I've seen your answer, and I've upvoted it a long time ago. It's a great solution, but Stephen Toub's approach is hard to beat!

public static async Task WithAggregateException(this Task source)
{
    try { await source.ConfigureAwait(false); } catch { source.Wait(); }
}

@noseratio
Copy link

It's a great solution, but Stephen Toub's approach is hard to beat!

Stephen's approach is simple and nice, I agree. The extension approach might be of interest to those who don't want to introduce another local variable for a Task, while also taking advantage of ex.Flatten().

@theodorzoulias
Copy link
Contributor Author

theodorzoulias commented Feb 15, 2021

@noseratio as you pointed out elsewhere, the WithAggregateException method posted in my previous comment does not handle the case of a canceled task nicely. It propagates a task that transitions to a Faulted state instead of being Canceled, and holds an AggregateException containing an inner OperationCanceledException. Which is rather awkward.

Below is an improved version of this method, that hopefully works as expected.

public static async Task WithAggregateException(this Task source)
{
    try { await source.ConfigureAwait(false); }
    catch (OperationCanceledException) when (source.IsCanceled) { throw; }
    catch { source.Wait(); }
}

@noseratio
Copy link

@theodorzoulias this snippet is rather elegant but there's still one thing. In general, I'm not a fan of using ConfigureAwait(false) in extension methods.

E.g., if source is a promise-like task which will be completed via TaskCompletionSource.SetResult on a thread with a synchronization context (e.g., a UI thread), there may be some redundant context switching, introduced by this tiny WithAggregateException method.

I know many would disagree, but these days, I prefer to not use ConfigureAwait at all 🙂

@theodorzoulias
Copy link
Contributor Author

I'm not a fan of using ConfigureAwait(false) in extension methods.

@noseratio that's a controversial opinion! Anyway, in the future we'll have more incentives to use the ConfigureAwait method, because it will be enhanced with some cool new options (#47525). Like the SuppressExceptions option, that should be handy quite often.

@noseratio
Copy link

@noseratio that's a controversial opinion! Anyway, in the future we'll have more incentives to use the ConfigureAwait method, because it will be enhanced with some cool new options (#47525).

I like that proposal about ConfigureAwait enhancements, it looks interesting! But I'm also glad I've embraced the use of await TaskScheduler.Default.SwitchTo() explicitly where I might be concerned about ConfigureAwait, at least about its synchronization context-related side-effects :)

@ghost ghost locked as resolved and limited conversation to collaborators Mar 18, 2021
@tannergooding tannergooding removed the untriaged New issue has not been triaged by the area owner label Jun 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading.Tasks
Projects
None yet
Development

No branches or pull requests

6 participants