Awaiting nongeneric Task.WhenAll changes behavior in .NET 8 to throw the last exception instead of the first
Closed this issue · 7 comments
Description
The WhenAll awaiter has always thrown the exception from the first faulting task only, where 'first' refers to the order within the collection of tasks passed to WhenAll, rather than to the order in which the exceptions were thrown.
This changes in .NET 8, and it only changes for nongeneric Task.WhenAll. Generic Task.WhenAll retains the behavior that WhenAll has always had.
There is no mention of this in https://learn.microsoft.com/en-us/dotnet/core/compatibility/8.0#core-net-libraries.
Was this intentional, or perhaps an accidental side effect during #88154 or other recent performance improvements? /cc @stephentoub
Reproduction Steps
var ex1 = new Exception();
var ex2 = new Exception();
try
{
var source1 = new TaskCompletionSource<object>();
var source2 = new TaskCompletionSource<object>();
// If the (Task) cast is removed, the behavior changes back to .NET 7 behavior.
var whenAllTask = Task.WhenAll((Task)source1.Task, source2.Task);
source2.SetException(ex2);
source1.SetException(ex1);
await whenAllTask;
}
catch (Exception ex)
{
Console.WriteLine(ReferenceEquals(ex, ex1)); // False on .NET 8, true on .NET 7.
}Expected behavior
The exception thrown during the await continuing to be the exception from the first task, as in all prior versions of .NET.
Actual behavior
The exception thrown during the await is the exception from the last task.
Regression?
No response
Known Workarounds
No response
Configuration
Used .NET SDK 8.0.100-rc.2.23502.2.
Other information
No response
Tagging subscribers to this area: @dotnet/area-system-threading-tasks
See info in area-owners.md if you want to be subscribed.
Issue Details
Description
The WhenAll awaiter has always thrown the exception from the first faulting task only, where 'first' refers to the order within the collection of tasks passed to WhenAll, rather than to the order in which the exceptions were thrown.
This changes in .NET 8, and it only changes for nongeneric Task.WhenAll. Generic Task.WhenAll retains the behavior that WhenAll has always had.
There is no mention of this in https://learn.microsoft.com/en-us/dotnet/core/compatibility/8.0#core-net-libraries.
Was this intentional, or perhaps an accidental side effect during #88154? /cc @stephentoub
Reproduction Steps
var ex1 = new Exception();
var ex2 = new Exception();
try
{
var source1 = new TaskCompletionSource<object>();
var source2 = new TaskCompletionSource<object>();
// If the (Task) cast is removed, the behavior changes back to .NET 7 behavior.
var whenAllTask = Task.WhenAll((Task)source1.Task, source2.Task);
source2.SetException(ex2);
source1.SetException(ex1);
await whenAllTask;
}
catch (Exception ex)
{
Console.WriteLine(ReferenceEquals(ex, ex1)); // False on .NET 8, true on .NET 7.
}Expected behavior
The exception thrown during the await continuing to be the exception from the first task, as in all prior versions of .NET.
Actual behavior
The exception thrown during the await is the exception from the last task.
Regression?
No response
Known Workarounds
No response
Configuration
Used .NET SDK 8.0.100-rc.2.23502.2.
Other information
No response
| Author: | jnm2 |
|---|---|
| Assignees: | - |
| Labels: |
|
| Milestone: | - |
It seems more related to #81065, where the order of the provided tasks is explicitly replaced with the order of raised exceptions.
IMHO such heavy changes should not be done without a good test coverage.
@stephentoub Should we revert this performance enhancement until it is reworked without any functional regression?
(I'm actually open to the new behavior being better, but it seems safer to consider it to rise to the level of a breaking change.)
IMHO such heavy changes should not be done without a good test coverage. @stephentoub Should we revert this performance enhancement until it is reworked without any functional regression?
I don't believe this is a "functional regression", and there is plenty of good test coverage. Can you please point me to where it's documented that the order in which WhenAll stores exceptions matches the order of the supplied tasks rather than the order in which the tasks complete, or more generally that there's any guaranteed order?
Has this been found to negatively impact anything other than some tests that were specifically coded to the previous ordering?
I'm not finding any documentation. I would have liked to read the C# design notes around how await is intended to ignore all exceptions except one, in case there was a C# design aspect onto how await interacts with awaitable things such as Task, but I don't know if they are publicly available.
I'm pretty sure the impact is positive, not negative. http://joelabrahamsson.com/exception-order-when-awaiting-multiple-async-tasks-in-c/ complains about this exact thing not matching JavaScript promises.
This exception order has had enough attention to the type of audience who reads https://learn.microsoft.com/en-us/dotnet/core/compatibility/8.0#core-net-libraries that I'm guessing people will be glad to read it there. I'm not going to say you're compelled to add an entry for it. This issue itself is something I can link people to in the future to read about the change.
I agree there is no guaranteed order, what may lead to some surprises. I would then recommend to add it to the existing list of known behavioral breaking changes with a clear statement "Task.WhenAll may return exceptions in a different order" with a specific attention to the await case.
About the test coverage, I propose to enhance it with new dedicated tests. See #93619
I just saw this behavior change when presenting Task.WhenAll to return just one exception, and showing how to get the information about all the exceptions. I've written about this in my Professional C# book in Chapter 11 Tasks and asynchronous programming.
@stephentoub
I agree it's not a "functional regression", the #81065 change is not only faster, but also a more expected result. All my attendees would have expected to see the first exception with the result.
But it should be listed with the behavior changes.