Conversation
- This is a contained fix to address a deadlock plagueing RPS tests on VS shutdown (it'd hang). See the associated issue for context. Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1529381
allisonchou
left a comment
There was a problem hiding this comment.
lgtm, although I still don't fully understand why the original deadlocks? (possibly need that threading overview meeting with @ryanbrandenburg we discussed before 😄)
So the gist is that whenever we had a pattern like this: Aka ProjectManagerChanged {
JTF.Run(() => { ... await switch to main thread...})
}What effectively is happening is that our calling thread (the one calling ProjectManagerChanged) would then block synchronously until it was able to get back onto the main thread. That combined with our codebase doing Hopefully this clarifies a little but I absolutely still owe y'all a threading brown bag |
|
Hello @NTaylorMullen! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
That does help, thank you for the explanation! |
|
Thanks for catching my thread crimes Taylor! I could SWEAR I did all the instances of this, I don't know if I lost it in a rebase and didn't notice or what. |
| _joinableTaskContext.AssertUIThread(); | ||
| await _documentManager.OnTextViewClosedAsync(textView, subjectBuffers); | ||
| }); | ||
| _joinableTaskContext.AssertUIThread(); |
There was a problem hiding this comment.
💡 Asserting the current thread in an asynchronous method is an anti-pattern. This should either be a synchronous method that asserts its affinity, or an asynchronous method that switches on demand.
|
|
||
| private void Document_ChangedOnDisk(object sender, EventArgs e) | ||
| { | ||
| _ = Document_ChangedOnDiskAsync((EditorDocument)sender, CancellationToken.None); |
There was a problem hiding this comment.
💡 This is an untracked fire-and-forget operation, which means tests will not be able to detect it or wait for it to complete.
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1529381