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

Add Task.WaitAsync methods #48842

Merged
merged 1 commit into from Mar 11, 2021
Merged

Conversation

stephentoub
Copy link
Member

Fixes #47525
Fixes #27722

Note that #47525 api-approved a different surface area, but we will rediscuss next week instead using the revised proposal, which this PR implements. If we instead decide to stick with what was approved, I'll replace this with stephentoub@a91eb11, which has the same guts but different public APIs exposing it. For now, I'm marking it no merge until the revised surface area is approved.

@ghost
Copy link

ghost commented Feb 26, 2021

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

Issue Details

Fixes #47525
Fixes #27722

Note that #47525 api-approved a different surface area, but we will rediscuss next week instead using the revised proposal, which this PR implements. If we instead decide to stick with what was approved, I'll replace this with stephentoub@a91eb11, which has the same guts but different public APIs exposing it. For now, I'm marking it no merge until the revised surface area is approved.

Author: stephentoub
Assignees: -
Labels:

area-System.Threading.Tasks

Milestone: 6.0.0

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@stephentoub stephentoub added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 26, 2021
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Feb 26, 2021

Implementation LGTM, but I'm also having second thoughts about the ValueTask methods now.

@eiriktsarpalis
Copy link
Member

Pinging @dotnet/ncl since the change touches networking product code as well.

@stephentoub
Copy link
Member Author

but I'm also having second thoughts about the ValueTask methods now.

Yup ;-)

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, all I've found were really minor nits ;)

Thank you @stephentoub !

@stephentoub stephentoub removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 9, 2021
@stephentoub stephentoub merged commit 062ae96 into dotnet:main Mar 11, 2021
@stephentoub stephentoub deleted the taskwaitasync branch March 11, 2021 17:11
@davidfowl
Copy link
Member

Waiting for the WaitAsync().Wait() bugs to be filed 🤣

@paulomorgado
Copy link
Contributor

This seems more akin the WhenAny and WhenAll.

It should be called just When.

/cc @stephentoub

@stephentoub
Copy link
Member Author

We explored lots of options and names. I think "When" is strictly worse than where we landed.

@paulomorgado
Copy link
Contributor

Wait is from the "old days" before async/await.

Why is When strictly worse than WaitAsync?

@stephentoub
Copy link
Member Author

stephentoub commented Mar 13, 2021

Wait is from the "old days" before async/await.

It's not. SemaphoreSlim, for example, has Wait and WaitAsync methods.

"When" only shows up in the static WhenAny and WhenAll, where is about one or all of the arguments completing. That's not the case here, where it's an instance method that's about waiting for the instance, just asynchronously, just as with SemaphoreSlim.WaitAsync, and with the wait curtailed by the arguments, just as with Wait. "When" makes it sound like the operation won't complete until the timeout or cancellation is requested, which is not the case.

@paulomorgado
Copy link
Contributor

SemaphoreSlim.WaitAsync is the asynchronous version of SemaphoreSlim.Wait.

My understanding of the general guidance is that Task returning methods should be suffixed Async, except if they already have methods suffixed Async, like WebClient. In that case, they should be suffixed TaskAsync.

The exceptions to this rule are methods operating on Task itself. That´s why there's Task.Run instead of Task.RunAsync. And Task.WhenAll, and Task.WhenAny.

@stephentoub
Copy link
Member Author

stephentoub commented Mar 14, 2021

SemaphoreSlim.WaitAsync is the asynchronous version of SemaphoreSlim.Wait.

That's exactly the same case here.

The exceptions to this rule are methods operating on Task itself. That´s why there's Task.Run instead of Task.RunAsync. And Task.WhenAll, and Task.WhenAny.

Those are all statics. This is not. It is the asynchronous version of the synchronous Wait, just as with SemaphoreSlim. Async isn't used in the statics on task because it's deemed redundant in that context; it's not here.

@paulomorgado
Copy link
Contributor

Got it!

@stephentoub stephentoub changed the title Add {Value}Task.WaitAsync methods Add Task.WaitAsync methods Mar 17, 2021
@ghost
Copy link

ghost commented Apr 15, 2021

I know I'm coming in late here, but @stephentoub any chance you can provide a control option to honor custom TaskSchedulers? Presently ConfigureAwait(false) suppresses both executioncontext capture and taskscheduler capture. Hoping some day that the default will permit custom schedulers for applications that need some control over thread scheduling (e.g. to implement quotas)

@dotnet dotnet locked as resolved and limited conversation to collaborators May 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
9 participants