Fixing the contention condition caused by RegisterResolversManifests #11079
Conversation
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
SimaTian
force-pushed
the
SdkResolverService_race_fix
branch
2 times, most recently
from
532900a to
2cdfbf7
Compare
SimaTian
changed the title
SimaTian
force-pushed
the
SdkResolverService_race_fix
branch
from
2cdfbf7 to
d7826cd
Compare
donJoseLuis
approved these changes
Dec 6, 2024
src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Rainer Sigwald <[email protected]>
…et/msbuild into SdkResolverService_race_fix
JanKrivanek
reviewed
Dec 11, 2024
src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
Outdated
Show resolved
Hide resolved
rainersigwald
approved these changes
Dec 11, 2024
JanKrivanek
approved these changes
Dec 13, 2024
src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs
Outdated
Show resolved
Hide resolved
|
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. |
|
/backport to vs17.12 |
|
Started backporting to vs17.12: https://github.com/dotnet/msbuild/actions/runs/13973433101 |
SimaTian
added a commit
that referenced
this pull request
Mar 27, 2025
…11079) * Contention condition reproduction via unit test * updating the lists only after they're complete to avoid the contention. * Update src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs Co-authored-by: Rainer Sigwald <[email protected]> * addressing review comments * minor touchup * #if DEBUG fix * refactoring to get rid of #if directives * removing unnecessary include * variable rename --------- Co-authored-by: Rainer Sigwald <[email protected]>
SimaTian
added a commit
that referenced
this pull request
May 23, 2025
…Manifests (#11612) Fixes #11079 Work item (Internal use): https://dev.azure.com/devdiv/DevDiv/_workitems/edit/2400156 Summary: **Backport of #11079 to vs17.12** There was a contention condition described #7927 (comment): 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. We want to backport it to 17.12 (which is 9.0.100) so it is a part of the source build. That means that a large part of our Linux userbase which runs on source builds SDKs is exposed to the problem and they could experience failures at any time - and they won't have the fix unless we backport. Customer Impact: It is an annoying error that occasionally kills a multiprocessed pipeline due to a contention condition. Fortunately it only manifests occassionaly, however it happens often enough to be noticeable. We have at least two well documented cases as of recently and even more if we extend the time window. Regression: No, it is a long standing bug. Testing src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs test to validate the error is no longer there. (See the original ticket for discussion) Risk Low, the change is already rolled out and tested in the main branch.
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.
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.