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

Support .NET 8's new ConfigureAwaitOptions #25

Closed
jnm2 opened this issue Oct 14, 2023 · 12 comments
Closed

Support .NET 8's new ConfigureAwaitOptions #25

jnm2 opened this issue Oct 14, 2023 · 12 comments

Comments

@jnm2
Copy link
Collaborator

jnm2 commented Oct 14, 2023

.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
Copy link
Owner

@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
Copy link
Collaborator Author

jnm2 commented Oct 25, 2023

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

@buvinghausen
Copy link
Owner

@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
Copy link
Owner

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
Copy link
Collaborator Author

jnm2 commented Oct 26, 2023

Looks great to me! I used 9b0ba5b...42df3ea to look at the diff.

@jnm2
Copy link
Collaborator Author

jnm2 commented Oct 26, 2023

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
Copy link
Owner

Yeah that was my intention.....

@buvinghausen
Copy link
Owner

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

@buvinghausen
Copy link
Owner

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

@jnm2
Copy link
Collaborator Author

jnm2 commented Nov 14, 2023

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
Copy link
Owner

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
Copy link
Collaborator Author

jnm2 commented Nov 15, 2023

Thanks for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants