Skip to content

Commit e8a0328

Browse files
SimaTiangithub-actions
authored andcommitted
updating the lists only after they're complete to avoid the contention.
1 parent fcd04a1 commit e8a0328

File tree

2 files changed

+21
-10
lines changed

2 files changed

+21
-10
lines changed

src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -168,8 +168,8 @@ internal void TryResolveSdk(out bool success)
168168

169169

170170
[Fact]
171-
// Scenario: we want to test that we solved the race described here: https://github.com/dotnet/msbuild/issues/7927#issuecomment-1232470838
172-
public void AssertResolverPopulationRaceNotPresent()
171+
// Scenario: we want to test that we solved the contention described here: https://github.com/dotnet/msbuild/issues/7927#issuecomment-1232470838
172+
public void AssertResolverPopulationContentionNotPresent()
173173
{
174174
s_sdkResolverService.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true), resolverOnly: true);
175175

src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -51,12 +51,12 @@ internal class SdkResolverService : ISdkResolverService
5151
/// <summary>
5252
/// Stores the list of manifests of specific SDK resolvers which could be loaded.
5353
/// </summary>
54-
private IList<SdkResolverManifest> _specificResolversManifestsRegistry;
54+
private IReadOnlyList<SdkResolverManifest> _specificResolversManifestsRegistry;
5555

5656
/// <summary>
5757
/// Stores the list of manifests of general SDK resolvers which could be loaded.
5858
/// </summary>
59-
private IList<SdkResolverManifest> _generalResolversManifestsRegistry;
59+
private IReadOnlyList<SdkResolverManifest> _generalResolversManifestsRegistry;
6060

6161
#if DEBUG
6262
internal bool _fake_initialization = false;
@@ -273,7 +273,7 @@ private SdkResult ResolveSdkUsingResolversWithPatternsFirst(int submissionId, Sd
273273
return new SdkResult(sdk, null, null);
274274
}
275275

276-
private List<SdkResolver> GetResolvers(IList<SdkResolverManifest> resolversManifests, LoggingContext loggingContext, ElementLocation sdkReferenceLocation)
276+
private List<SdkResolver> GetResolvers(IReadOnlyList<SdkResolverManifest> resolversManifests, LoggingContext loggingContext, ElementLocation sdkReferenceLocation)
277277
{
278278
// Create a sorted by priority list of resolvers. Load them if needed.
279279
List<SdkResolver> resolvers = new List<SdkResolver>();
@@ -517,9 +517,12 @@ private void RegisterResolversManifests(ElementLocation location)
517517
}
518518
}
519519

520-
_specificResolversManifestsRegistry = new List<SdkResolverManifest>();
521-
_generalResolversManifestsRegistry = new List<SdkResolverManifest>();
520+
var _specificResolversManifestsRegistryPlaceholder = new List<SdkResolverManifest>();
521+
var _generalResolversManifestsRegistryPlaceholder = new List<SdkResolverManifest>();
522522

523+
// Break the list of all resolvers manifests into two parts: manifests with specific and general resolvers.
524+
// Since the collections are meant to be immutable, we have to only ever assign them when they're complete.
525+
// Otherwise race can happen, see https://github.com/dotnet/msbuild/issues/7927
523526
foreach (SdkResolverManifest manifest in allResolversManifests)
524527
{
525528
#if DEBUG
@@ -530,17 +533,25 @@ private void RegisterResolversManifests(ElementLocation location)
530533
#endif
531534
if (manifest.ResolvableSdkRegex == null)
532535
{
533-
_generalResolversManifestsRegistry.Add(manifest);
536+
_generalResolversManifestsRegistryPlaceholder.Add(manifest);
534537
}
535538
else
536539
{
537-
_specificResolversManifestsRegistry.Add(manifest);
540+
_specificResolversManifestsRegistryPlaceholder.Add(manifest);
538541
}
539542
}
540543
if (sdkDefaultResolversManifest != null)
541544
{
542-
_generalResolversManifestsRegistry.Add(sdkDefaultResolversManifest);
545+
_generalResolversManifestsRegistryPlaceholder.Add(sdkDefaultResolversManifest);
543546
}
547+
548+
// Until this is set(and this is under lock), the ResolveSdkUsingResolversWithPatternsFirst will always
549+
// enter if branch leaving to this section.
550+
// Then it will wait at the lock and return after we release it since the collections we have filled them before releasing the lock.
551+
// The collections are never modified after this point.
552+
// So I've made them ReadOnly
553+
_generalResolversManifestsRegistry = _generalResolversManifestsRegistryPlaceholder.AsReadOnly();
554+
_specificResolversManifestsRegistry = _specificResolversManifestsRegistryPlaceholder.AsReadOnly();
544555
}
545556
}
546557

0 commit comments

Comments
 (0)