Ensure JTF.SwitchToMainThreadAsync always throws when canceled - #576
Merged
Andrew Arnott (AArnott) merged 2 commits intoDec 20, 2019
Merged
Conversation
Andrew Arnott (AArnott)
requested review from
David Kean (davkean),
Lifeng Lu (lifengl) and
Sam Harwell (sharwell)
December 20, 2019 03:55
Member
Author
Sam Harwell (sharwell)
approved these changes
Dec 20, 2019
Sam Harwell (sharwell)
left a comment
Contributor
There was a problem hiding this comment.
This seems like intuitive behavior. We should take care to make the synchronization context behavior match what TaskAwaiter does so it's no more confusing than necessary.
Even if the caller is on the main thread or reaches the main thread before cancellation from a threadpool thread can be processed, callers tend to expect that the `OperationCanceledException` will be thrown regardless. So this changes behavior to meet expectations. This also removes the VSTHRD201 analyzer recently added that helped people workaround the fact that this hadn't been the behavior. The analyzer is no longer necessary. Closes microsoft#434
Andrew Arnott (AArnott)
force-pushed
the
STMTA_Cancellation
branch
from
December 20, 2019 15:10
642d68c to
6289102
Compare
Member
Author
|
Sam Harwell (@sharwell) I know you already signed off, but since I ended up "not fixing" the issue you called out on the basis that I think it's behaving properly, I'd love to get your confirmation that you agree, or give you another chance to tell me why I'm wrong. :) |
Andrew Arnott (AArnott)
force-pushed
the
STMTA_Cancellation
branch
from
December 20, 2019 16:04
532f021 to
13504ca
Compare
Contributor
|
LGTM I just wanted to make sure the case was carefully considered. 👍 |
Sam Harwell (sharwell)
added a commit
to sharwell/gitextensions
that referenced
this pull request
Feb 12, 2020
This check was implemented in microsoft/vs-threading#576.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Even if the caller is on the main thread or reaches the main thread before cancellation from a threadpool thread can be processed, callers tend to expect that the
OperationCanceledExceptionwill be thrown regardless. So this changes behavior to meet expectations.This also removes the VSTHRD201 analyzer recently added that helped people workaround the fact that this hadn't been the behavior. The analyzer is no longer necessary.
Closes #434