Fix issues with reentrancy due to pumping in lock acquisition#82522
Open
jasonmalinowski wants to merge 1 commit intodotnet:mainfrom
Open
Fix issues with reentrancy due to pumping in lock acquisition#82522jasonmalinowski wants to merge 1 commit intodotnet:mainfrom
jasonmalinowski wants to merge 1 commit intodotnet:mainfrom
Conversation
In the .NET Framework, a wait on a lock (like a regular Monitor, waiting on a Task, etc.) can do a pumping wait, where certain other operations can still proceed while the thread is blocked on the wait. This can result in reentrancy, since (if nothing else) COM calls being sent to the UI thread can run. This can result in all sorts of bad behavior: - @ToddGrun sent me a dump where we were trying to acquire the workspace lock on the UI thread to update the correct context for a file added to multiple projects. The lock was held by some other thread, so on the UI thread blocked waiting for that to be released. Because that wait was a pumping wait, further work then started running on the UI thread that needed to acquire that lock, so a second Wait() on the same thread happened. Once the other work released the lock, the first Wait() was released -- but that thread cannot resume since the second, nested Wait() is still blocked. - https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2600824: here, we're responding to an event from the shell and again are trying to acquire the lock. While we're blocked, a COM operation starts to do autorecovery saving, and since that calls back into the RDT, this was unexpected and things get corrupted. - https://developercommunity.visualstudio.com/t/Visual-Studio-2022-frequently-freezes-at/10053736 where a COM call from the web project systems can come into the UI thread while we're in the middle of processing the project system, resulting in further strange behavior. In VS, the general rule is to not use SemaphoreSlim or other locks for this sort of reason, and use vs-threading locks, which amongst other things apply a different synchronization context which changes how the wait is done -- it does a non-pumping wait. (The VS threading rules also generally don't want you using COM marshalling because of this and other funky behaviors.) Alas, we don't use VS threading locks here for a few reasons: 1. We take advantage of the fact that SemaphoreSlim allows both sync and async waiters, and sync is given priority when picking who goes next. This is necessary since we have project systems that still calls on the UI thread, or other UI thread updating we have to do and we absolutely don't want the UI thread blocked up behind async work. 2. These helpers live at a fairly low level in our stack, and we can't easily take a dependency on vs-threading without having to opt it (and it's dependencies) into source build. As a tactical fix, I'm going to borrow some of the code from vs-threading which applies the custom SynchronizationContext that does not do a CoWait and use that whenever we are doing a synchronous block on a SemaphoreSlim with the helper we use across the codebase. This is not a perfect fix, since there's plenty of other waits (like Monitor acquisition) that we could potentially be blocked by too. In practice, this is the path that is being used any time have seen issues in the last few years, so hopefully this has the greatest benefit. Unfortunately, the deeper fixes of "change the .NET framework behavior" and "get rid of other things in VS using COM marshalling" is quite difficult to do, given how much changing that breaks. For example, for the feedback ticket above regarding web project systems, we did make try to make a change to switch that over to a more JTF-friendly wait, but since there's other code in that layer that assumed COM marshalling rules, other things broke. Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2600824 Fixes https://developercommunity.visualstudio.com/t/Visual-Studio-2022-frequently-freezes-at/10053736
cb97a12 to
155670d
Compare
Contributor
|
Horrible flashbacks to the same issue 10 years ago. |
JoeRobich
approved these changes
Feb 24, 2026
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
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.
In the .NET Framework, a wait on a lock (like a regular Monitor, waiting on a Task, etc.) can do a pumping wait, where certain other operations can still proceed while the thread is blocked on the wait. This can result in reentrancy, since (if nothing else) COM calls being sent to the UI thread can run. This can result in all sorts of bad behavior:
@ToddGrun sent me a dump where we were trying to acquire the workspace lock on the UI thread to update the correct context for a file added to multiple projects. The lock was held by some other thread, so on the UI thread blocked waiting for that to be released. Because that wait was a pumping wait, further work then started running on the UI thread that needed to acquire that lock, so a second Wait() on the same thread happened. Once the other work released the lock, the first Wait() was released -- but that thread cannot resume since the second, nested Wait() is still blocked.
https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2600824: here, we're responding to an event from the shell and again are trying to acquire the lock. While we're blocked, a COM operation starts to do autorecovery saving, and since that calls back into the RDT, this was unexpected and things get corrupted.
https://developercommunity.visualstudio.com/t/Visual-Studio-2022-frequently-freezes-at/10053736 where a COM call from the web project systems can come into the UI thread while we're in the middle of processing the project system, resulting in further strange behavior.
In VS, the general rule is to not use SemaphoreSlim or other locks for this sort of reason, and use vs-threading locks, which amongst other things apply a different synchronization context which changes how the wait is done -- it does a non-pumping wait. (The VS threading rules also generally don't want you using COM marshalling because of this and other funky behaviors.) Alas, we don't use VS threading locks here for a few reasons:
As a tactical fix, I'm going to borrow some of the code from vs-threading which applies the custom SynchronizationContext that does not do a CoWait and use that whenever we are doing a synchronous block on a SemaphoreSlim with the helper we use across the codebase. This is not a perfect fix, since there's plenty of other waits (like Monitor acquisition) that we could potentially be blocked by too. In practice, this is the path that is being used any time have seen issues in the last few years, so hopefully this has the greatest benefit.
Unfortunately, the deeper fixes of "change the .NET framework behavior" and "get rid of other things in VS using COM marshalling" is quite difficult to do, given how much changing that breaks. For example, for the feedback ticket above regarding web project systems, we did make try to make a change to switch that over to a more JTF-friendly wait, but since there's other code in that layer that assumed COM marshalling rules, other things broke.
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2600824
Fixes https://developercommunity.visualstudio.com/t/Visual-Studio-2022-frequently-freezes-at/10053736