buvinghausen / TaskTupleAwaiter

Async helper library to allow leveraging the new ValueTuple data types in C# 7.0
MIT License
70 stars 10 forks source link

AssertAllAdapters call is failing in .NET 8.0 #24

Closed buvinghausen closed 6 months ago

buvinghausen commented 11 months ago

Yo @jnm2 just a heads up but I was prepping the nuget package for the upcoming .NET 8.0 release and noticed all 9 calls to AssertAllAdapters when running in the 8.0 context.

I will try to figure out what is going on between now and next month when it RTMs but I just wanted to let you know in the event you had some ideas.

I've already put the net8.0 moniker in the props files if you do take a look and have RC2 installed.

jnm2 commented 11 months ago

Weird, it looks like the behavior of Task.WhenAll has changed starting in .NET 8. I wonder if this is known or if there's a dotnet/runtime issue on it. I don't see it in https://learn.microsoft.com/en-us/dotnet/core/compatibility/8.0#core-net-libraries.

jnm2 commented 11 months ago

Here's a repro. I think we should ask about this on dotnet/runtime:

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.
}
jnm2 commented 11 months ago

Asked: https://github.com/dotnet/runtime/issues/93504

jnm2 commented 11 months ago

This appears to be by design. The order has never been documented, and there is a performance benefit associated with the change. I think we should probably loosen the tests so that we make no assumptions about which exception it is, only that it is one of the exception instances. (And not, e.g., wrapping it inside AggregateException.)

buvinghausen commented 10 months ago

Here was the tweak to not compile and run the test for .NET 8 and beyond