Replace Task.WaitAll with Task.WhenAll in async method. - #479
Conversation
Task.WaitAll has an overload that accepts a CancellationToken whereas the Task.WhenAll method does not. So I've added some useful extension methods that enable the same pattern you had before, but doing so in an async fashion rather than blocking a thread.
There was a problem hiding this comment.
is WithCancellation needed? simplifyTasks is already hot. if there is any cancellation, it will throw.
There was a problem hiding this comment.
Also, Task.WhenAll already handles cancellation, doesn't it? I tried a quick and dirty experiment and i got a good OperationCanceledException out of it if any of the tasks were canceled. Not sure what if hte WithCancellation bit is needed.
This is what the docs say:
If any of the supplied tasks completes in a faulted state, the returned task will
also complete in a Faulted state, where its exceptions will contain the aggregation
of the set of unwrapped exceptions from each of the supplied tasks.
If none of the supplied tasks faulted but at least one of them was canceled, the
returned task will end in the Canceled state.
There was a problem hiding this comment.
Oh.... i think i see. Task.WhenAll will only complete once all the inner tasks are done. You're trying to get it so that the task transitions immediately to cancelled if the cancellationToken triggers.
That's definitely interesting. Though is it necessary in practice? Are our inner reduction tasks not responding to cancellation quickly enough?
There was a problem hiding this comment.
if we change Task.WaitAll(...) to await Task.WhenAll(Tasks).ConfigureAwait(false)
don't we get most of benefit if not all? every tasks given to WhenAll in this particular case has cancellation token (in fact same cancellation token).
so, What WithCancellation trying to achieve here is not needed. also WithCancellation seems like kind of bandage solution where it tries to fix an issue where implementor of tasks didn't do proper cancellation management.
in Roslyn code base, if that is the case, I would say it is just a bug and fix the implementation of that task rather than use WithCancellation.
There was a problem hiding this comment.
This whole discussion seems to suggest I added the cancellationToken to the wait function. I did not. It was already there. I merely preserved prior behavior. The WaitAll method took a cancellationToken. This not only allowed the async method's Task to complete quickly upon cancellation regardless of the tasks it is waiting on, but it increased the odds that in that event an OperationCanceledException would be thrown instead of some other fault.
I'm happy to remove the WithCancellationToken extension method call if you would rather not have it though.
There was a problem hiding this comment.
I am on the side of just doing simple change for this case. I didn't mean to suggest that you added something that is not needed. sorry. :)
There was a problem hiding this comment.
No worries. I just wanted to make sure we were on the same page about what would be a deviation in behavior. Is the simple change leaving the behavior as-is, or removing the cancellationToken from the WhenAll? I'll do either one.
|
Overall, now that I confirmed I can actually read code, 👍. My concern about the potential of abuse is still there, but I guess it's better we have the potential for abuse then the current actual abuse of the thread pool. |
|
Well I'm still happy to remove |
|
My personal preference would be to remove it. I think Roslyn's tasks are, in general, well behaved enough to not need this. And, if they aren't, they need to fixed. I think this method has the potential to mask bad behavior too much. |
|
There are a bunch of commits added here that don't seem related to your change. |
|
@CyrusNajmabadi The unrelated commits you see are an artifact from the fact that I merged master into my pull request branch to resolve merge conflicts. They aren't new to the target branch. |
This in response to code review feedback with the expectation that the WhenAll's returned Task will complete quickly enough anyway upon cancellation because each task being awaited for cancels quickly upon request.
|
A good chunk of these files are showing up as binary diffs now. Were they converted to UTF-16 on accident? |
|
Git command line tools show that only the required files have any changes. On Sun, Feb 15, 2015, 10:58 AM Jared Parsons notifications@github.com
|
|
@AArnott I've only seen GitHub classify a file as binary when Git does as well. We need to figure out what is going on here. |
|
@AArnott I just did a quick test where I merged your branch with dotnet/roslyn master and the diffs are coming out clean. It looks like you branched right around the last time we had to do some fix up from UTF-16 to UTF-8 and the diffs are just reflecting everything not being 100% up to date. |
|
@jaredpar @CyrusNajmabadi Given the extra commits and files changed that GitHub reports that the git command line tools don't, I've submitted a support request to GitHub so at least they're aware of a potential bug in their online experience. |
|
Its strange. If u merge your branch with master and create a new PR it will show the true diff vs what it shows now. That's the behavior I saw. JaredPar from a phone From: Andrew Arnott notifications@github.com @jaredparhttps://github.com/jaredpar @CyrusNajmabadihttps://github.com/CyrusNajmabadi Given the extra commits and files changed that GitHub reports that the git command line tools don't, I've submitted a support request to GitHub so at least they're aware of a potential bug in their online experience. Reply to this email directly or view it on GitHubhttps://github.com//pull/479#issuecomment-74435106. |
|
@AArnott does this contain only WhenAll changes? if it is, then I can merge it to master. not sure why it shows all other changes. |
|
It's not a terrible idea to just create a new branch at thsi point and just cherry-pick the ocmmits you care about into your new branch. You can then submit another review. I do this when something funky happens to a branch i'm working on. |
|
@heejaechang Yes, this pull request contains just 5 lines or so changed in one file, per "git diff" between the two branches. |
|
I got a reply from github support on the extra commits and files changed that github claims are part of this pull request. In short, it's because the merge I made to resolve conflicts (due to the massive Src->src rename) had the parent branches of the merge commit in the reverse order from what their algorithm expected. There's more explanation, but that's the bottom line. It's a safe pull request to merge. |
Replace Task.WaitAll with Task.WhenAll in async method.
|
To close the loop the actual committed change was as simple as it should have been: |
Add nullable attributes to IVerifier methods
8 participants
Task.WaitAll has an overload that accepts a CancellationToken whereas the Task.WhenAll method does not. So I've added some useful extension methods that enable the same pattern you had before, but doing so in an async fashion rather than blocking a thread.
Fix #478