buvinghausen / TaskTupleAwaiter

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

Support .NET 8's new ConfigureAwaitOptions #25

Closed jnm2 closed 8 months ago

jnm2 commented 1 year ago

.NET 8 adds an overload to Task.ConfigureAwait: https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.task.configureawait?view=net-8.0#system-threading-tasks-task-configureawait(system-threading-tasks-configureawaitoptions)

It would make sense to support the new ForceYielding and SuppressThrowing flags with a corresponding ConfigureAwait extension method on tuples of tasks.

buvinghausen commented 1 year ago

@jnm2 just to be clear for anyone using the library today the code still runs fine on .NET 8 despite the exception order change that triggered the test failures right?

jnm2 commented 1 year ago

Correct. The test was overly specific to start with, since Steve has clarified that there was no documented order.

buvinghausen commented 1 year ago

@jnm2 I took a stab at adding the ConfigureAwaitOptions for .NET 8 for Task<T1>...Task<T2>

Can you take a look and see if you are cool with it? If so I will carry it forward for the remaining structs. Note I made all the constructors internal so you have to use the static methods as intended by the API. For .NET 8 I just keep the options flag enum and use the ternary to port the Boolean flag into the options so it shouldn't break the API and we are keeping as little in memory as possible with the greatest support footprint.

buvinghausen commented 1 year ago

I just changed it to standardize around the variable name options that will require fewer conditional compilation directives.....

I'm through Arity 5 still have 6-10 to go but I'm getting tired so I will finish it off in the AM.

jnm2 commented 1 year ago

Looks great to me! I used 9b0ba5b7a239abefe251cdc71821ae2f1edbd26a...42df3ea6dc20ec4ccd42d71e5bee9bd345d46df5 to look at the diff.

jnm2 commented 1 year ago

Since we're making some constructors internal that used to be public, we might want to consider this a new major version technically. However, in practice, I'm doubtful that anyone would have been using that API.

buvinghausen commented 1 year ago

Yeah that was my intention.....

buvinghausen commented 1 year ago

Ok the implementation side is done just need to cover the new functions in tests

buvinghausen commented 1 year ago

@jnm2 what tests should we have before I do the nuget release? RTM 8.0 today

jnm2 commented 1 year ago

I'd be comfortable releasing what I've seen. So long as we're directly passing ConfigureAwaitOptions to Task.WhenAll(...).ConfigureAwait(ConfigureAwaitOptions), and all the tests covering .ConfigureAwait(bool) and direct awaits still pass, I'm not really worried. Tests could follow later.

buvinghausen commented 1 year ago

Cool I just added a conditional directive to not include the FirstExceptionIsThrown test for >= .NET 8.0 so now we have all passing tests just 10 fewer for .NET 8.

Looks like my nuget token has expired I will get it reissued tomorrow and release all the packages in the morning. Thanks for all your help

jnm2 commented 1 year ago

Thanks for your work!