Fixing the contention condition caused by RegisterResolversManifests - #11079
#11079Merged
SimaTian merged 13 commits intoDec 16, 2024
maindotnet/msbuild:mainfrom
SdkResolverService_race_fixdotnet/msbuild:SdkResolverService_race_fixCopy head branch name to clipboard
Merged
Fixing the contention condition caused by RegisterResolversManifests#11079SimaTian merged 13 commits intomaindotnet/msbuild:mainfrom SdkResolverService_race_fixdotnet/msbuild:SdkResolverService_race_fixCopy head branch name to clipboard
SimaTian merged 13 commits into
maindotnet/msbuild:mainfrom
SdkResolverService_race_fixdotnet/msbuild:SdkResolverService_race_fixCopy head branch name to clipboard
Conversation
SimaTian
force-pushed
the
SdkResolverService_race_fix
branch
2 times, most recently
from
December 6, 2024 09:59
532900a to
2cdfbf7
Compare
SimaTian
force-pushed
the
SdkResolverService_race_fix
branch
from
December 6, 2024 10:18
2cdfbf7 to
d7826cd
Compare
donJoseLuis
approved these changes
Dec 6, 2024
donJoseLuis
left a comment
There was a problem hiding this comment.
few minor comments; man one moving away from using the thread class
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
…et/msbuild into SdkResolverService_race_fix
JanKrivanek
reviewed
Dec 11, 2024
JanKrivanek
left a comment
Member
There was a problem hiding this comment.
First quick pass.
There is lots of test related code in production code - can we isolate it inot tests
rainersigwald
approved these changes
Dec 11, 2024
JanKrivanek
approved these changes
Dec 13, 2024
JanKrivanek
left a comment
Member
There was a problem hiding this comment.
Thank you for making the efforts to clean the production part of the code! I like this!!
|
GitHub did not automatically close #7927. See "Multiple issues" in https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword. |
Member
|
/backport to vs17.12 |
Contributor
|
Started backporting to vs17.12: https://github.com/dotnet/msbuild/actions/runs/13973433101 |
This was referenced Jul 10, 2026
[release/9.0]: Bump Microsoft.Build.Framework and Microsoft.Build.Utilities.Core
dotnet/efcore#38637
Closed
Closed
This was referenced Jul 19, 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
6 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.
Fixes #11043, #7927
Context
There was a contention condition described here:
One thread enters and locks, then initializes a list, starts pushing things onto the list, which is now no longer null.
Second thread then checks, sees the list is not empty and bypasses the lock, acquires enumerator.
First thread pushes additional item into the list.
Second thread throws.
Changes Made
Initializing the list to a placeholder first, only assigning it when it is finished. Due to this, until the list is fully initialized, all threads have to enter the lock, then they will check that the list is now initialized and return.
This will result in some minor locking overhead, but it is not as strict as locking the collection for all reads.
Furthermore, since the collection is supposed to be read only as written here, I've made it into IReadOnlyList instead of the current IList.
Testing
I got a reproduction, although a bit clunky.
First commit checks should fail. Then I will push the fix which should succeed.