Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing the contention condition caused by RegisterResolversManifests #11079

Merged
merged 13 commits into from
Dec 16, 2024

Conversation

SimaTian
Copy link
Member

@SimaTian SimaTian commented Dec 4, 2024

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.

@SimaTian SimaTian force-pushed the SdkResolverService_race_fix branch 2 times, most recently from 532900a to 2cdfbf7 Compare December 6, 2024 09:59
@SimaTian SimaTian changed the title Fixing the race condition caused by RegisterResolversManifests Fixing the contention condition caused by RegisterResolversManifests Dec 6, 2024
@SimaTian SimaTian force-pushed the SdkResolverService_race_fix branch from 2cdfbf7 to d7826cd Compare December 6, 2024 10:18
Copy link

@donJoseLuis donJoseLuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few minor comments; man one moving away from using the thread class

src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs Outdated Show resolved Hide resolved
src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs Outdated Show resolved Hide resolved
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First quick pass.

There is lots of test related code in production code - can we isolate it inot tests

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making the efforts to clean the production part of the code! I like this!!

@SimaTian SimaTian merged commit 3a63bca into main Dec 16, 2024
10 checks passed
@SimaTian SimaTian deleted the SdkResolverService_race_fix branch December 16, 2024 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collection was modified in SdkResolverService.ResolveSdkUsingResolversWithPatternsFirst
5 participants