From fcd04a145ce74687325c5905eab33c31e158e865 Mon Sep 17 00:00:00 2001 From: SimaTian Date: Fri, 6 Dec 2024 11:18:02 +0100 Subject: [PATCH 01/10] Contention condition reproduction via unit test --- .../BackEnd/SdkResolverService_Tests.cs | 148 +++++++++++++----- .../SdkResolution/SdkResolverService.cs | 59 +++++-- 2 files changed, 162 insertions(+), 45 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs index 62e842a2558..0d5a37648f9 100644 --- a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs +++ b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs @@ -6,6 +6,7 @@ using System.Diagnostics.Tracing; using System.Linq; using System.Text.RegularExpressions; +using System.Threading; using System.Threading.Tasks; using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.BackEnd.SdkResolution; @@ -29,9 +30,12 @@ public class SdkResolverService_Tests : IDisposable { private readonly MockLogger _logger; private readonly LoggingContext _loggingContext; + private static SdkResolverService s_sdkResolverService; + public SdkResolverService_Tests() { + s_sdkResolverService = new SdkResolverService(); _logger = new MockLogger(); ILoggingService loggingService = LoggingService.CreateLoggingService(LoggerMode.Synchronous, 1); loggingService.RegisterLogger(_logger); @@ -43,18 +47,20 @@ public SdkResolverService_Tests() public void Dispose() { - SdkResolverService.Instance.InitializeForTests(); + var service = new SdkResolverService(); + service.InitializeForTests(); } [Fact] // Scenario: Sdk is not resolved. public void AssertAllResolverErrorsLoggedWhenSdkNotResolved() { - SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true)); + var service = new SdkResolverService(); + service.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true)); SdkReference sdk = new SdkReference("notfound", "referencedVersion", "minimumVersion"); - var result = SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); + var result = service.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); result.Success.ShouldBeFalse(); result.ShouldNotBeNull(); @@ -84,7 +90,8 @@ public void AssertResolutionWarnsIfResolvedVersionIsDifferentFromReferencedVersi { var sdk = new SdkReference("foo", "1.0.0", null); - SdkResolverService.Instance.InitializeForTests( + var service = new SdkResolverService(); + service.InitializeForTests( null, new List { @@ -96,7 +103,7 @@ public void AssertResolutionWarnsIfResolvedVersionIsDifferentFromReferencedVersi Enumerable.Empty())) }); - var result = SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); + var result = service.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); result.Path.ShouldBe("path"); @@ -107,12 +114,13 @@ public void AssertResolutionWarnsIfResolvedVersionIsDifferentFromReferencedVersi [Fact] public void AssertResolverThrows() { - SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeErrorResolver: true)); + var service = new SdkResolverService(); + service.InitializeForTests(new MockLoaderStrategy(includeErrorResolver: true)); SdkReference sdk = new SdkReference("1sdkName", "version1", "minimumVersion"); // When an SDK resolver throws, the expander will catch it and stop the build. - SdkResolverException e = Should.Throw(() => SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true)); + SdkResolverException e = Should.Throw(() => service.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true)); e.Resolver.Name.ShouldBe("MockSdkResolverThrows"); e.Sdk.Name.ShouldBe("1sdkName"); } @@ -122,11 +130,12 @@ public void AssertResolverThrows() // and it successfully resolves sdk. public void AssertSecondResolverWithPatternCanResolve() { - SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true)); + var service = new SdkResolverService(); + service.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true)); SdkReference sdk = new SdkReference("2sdkName", "referencedVersion", "minimumVersion"); - var result = SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); + var result = service.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); result.Path.ShouldBe("resolverpathwithresolvablesdkpattern2"); _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolverWithResolvableSdkPattern2 running"); @@ -134,16 +143,74 @@ public void AssertSecondResolverWithPatternCanResolve() _logger.BuildMessageEvents.Select(i => i.Message).ShouldNotContain("MockSdkResolver2 running"); } +#if DEBUG + internal void TryResolveSdk(out bool success) + { + success = true; + SdkReference sdk = new SdkReference("2sdkName", "referencedVersion", "minimumVersion"); + try + { + s_sdkResolverService.ResolveSdk(BuildEventContext.InvalidSubmissionId, + sdk, + _loggingContext, + new MockElementLocation("file"), + "sln", + "projectPath", + interactive: false, + isRunningInVisualStudio: false, + failOnUnresolvedSdk: true); + } + catch (Exception) + { + success = false; + } + } + + + [Fact] + // Scenario: we want to test that we solved the race described here: https://github.com/dotnet/msbuild/issues/7927#issuecomment-1232470838 + public void AssertResolverPopulationRaceNotPresent() + { + s_sdkResolverService.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true), resolverOnly: true); + + List manifests = new List(); + for (int i = 1; i != 20; i++) + { + var man = new SdkResolverManifest(DisplayName: "TestResolversManifest", Path: null, ResolvableSdkRegex: new Regex("abc")); + manifests.Add(man); + man = new SdkResolverManifest(DisplayName: "TestResolversManifest", Path: null, null); + manifests.Add(man); + } + s_sdkResolverService._fakeManifestRegistry = manifests.AsReadOnly(); + s_sdkResolverService._fake_initialization = true; + + SdkReference sdk = new SdkReference("2sdkName", "referencedVersion", "minimumVersion"); + + bool result1 = false; + bool result2 = false; + Thread thread1 = new Thread(() => TryResolveSdk(out result1)); + Thread thread2 = new Thread(() => TryResolveSdk(out result2)); + thread1.Start(); + Thread.Sleep(200); + thread2.Start(); + thread2.Join(); + thread1.Join(); + Assert.True(result1); + Assert.True(result2); + } +#endif + [Fact] // Scenario: MockSdkResolverWithResolvableSdkPattern1 is a specific resolver, it is loaded but did not resolve sdk. // MockSdkResolver1 is a general resolver (i.e. resolver without pattern), it resolves sdk on a fallback. public void AssertFirstResolverCanResolve() { - SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy()); + var service = new SdkResolverService(); + service.InitializeForTests(new MockLoaderStrategy()); SdkReference sdk = new SdkReference("1sdkName", "referencedVersion", "minimumVersion"); - var result = SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); + var result = service.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); result.Path.ShouldBe("resolverpath1"); _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolver1 running"); @@ -155,11 +222,12 @@ public void AssertFirstResolverCanResolve() // becuase MockSdkResolver1 is general and MockSdkResolverWithResolvableSdkPattern1 is specific. public void AssertFirstResolverWithPatternCanResolve() { - SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true)); + var service = new SdkResolverService(); + service.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true)); SdkReference sdk = new SdkReference("11sdkName", "referencedVersion", "minimumVersion"); - var result = SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); + var result = service.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); result.Path.ShouldBe("resolverpathwithresolvablesdkpattern1"); _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain("MockSdkResolverWithResolvableSdkPattern1 running"); @@ -169,10 +237,11 @@ public void AssertFirstResolverWithPatternCanResolve() [Fact] public void AssertSdkResolutionMessagesAreLogged() { - SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy()); + var service = new SdkResolverService(); + service.InitializeForTests(new MockLoaderStrategy()); SdkReference sdk = new SdkReference("1sdkName", "referencedVersion", "minimumVersion"); - var result = SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); + var result = service.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); // First resolver attempted to resolve, but failed. _logger.BuildMessageEvents.Select(i => i.Message).ShouldContain(ResourceUtilities.FormatResourceStringStripCodeAndKeyword("SDKResolverAttempt", nameof(MockResolverReturnsNull), sdk.ToString(), "null", @@ -185,11 +254,12 @@ public void AssertSdkResolutionMessagesAreLogged() public void AssertSdkResolutionMessagesAreLoggedInEventSource() { using var eventSourceTestListener = new EventSourceTestHelper(); - SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy(false, false, true)); + var service = new SdkResolverService(); + service.InitializeForTests(new MockLoaderStrategy(false, false, true)); var sdkName = Guid.NewGuid().ToString(); SdkReference sdk = new SdkReference(sdkName, "referencedVersion", "minimumVersion"); - SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); + service.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); var eventsLogged = eventSourceTestListener.GetEvents(); eventsLogged.ShouldContain(x => x.EventId == 64); // Start of the sdk resolve eventsLogged.ShouldContain(x => x.EventId == 65 && x.Payload[1].ToString() == sdkName); @@ -198,13 +268,14 @@ public void AssertSdkResolutionMessagesAreLoggedInEventSource() [Fact] public void AssertFirstResolverErrorsSupressedWhenResolved() { - SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy()); + var service = new SdkResolverService(); + service.InitializeForTests(new MockLoaderStrategy()); // 2sdkName will cause MockSdkResolver1 to fail with an error reason. The error will not // be logged because MockSdkResolver2 will succeed. SdkReference sdk = new SdkReference("2sdkName", "version2", "minimumVersion"); - var result = SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); + var result = service.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); result.Path.ShouldBe("resolverpath2"); @@ -222,15 +293,16 @@ public void AssertResolverHasStatePreserved() { const int submissionId = 5; - SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy()); + var service = new SdkResolverService(); + service.InitializeForTests(new MockLoaderStrategy()); SdkReference sdk = new SdkReference("othersdk", "1.0", "minimumVersion"); // First call should not know state - SdkResolverService.Instance.ResolveSdk(submissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true).Path.ShouldBe("resolverpath"); + service.ResolveSdk(submissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true).Path.ShouldBe("resolverpath"); // Second call should have received state - SdkResolverService.Instance.ResolveSdk(submissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true).Path.ShouldBe(MockSdkResolverWithState.Expected); + service.ResolveSdk(submissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true).Path.ShouldBe(MockSdkResolverWithState.Expected); } [Fact] @@ -238,15 +310,16 @@ public void AssertResolverStateNotPreserved() { const int submissionId = BuildEventContext.InvalidSubmissionId; - SdkResolverService.Instance.InitializeForTests(new MockLoaderStrategy()); + var service = new SdkResolverService(); + service.InitializeForTests(new MockLoaderStrategy()); SdkReference sdk = new SdkReference("othersdk", "1.0", "minimumVersion"); // First call should not know state - SdkResolverService.Instance.ResolveSdk(submissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true).Path.ShouldBe("resolverpath"); + service.ResolveSdk(submissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true).Path.ShouldBe("resolverpath"); // Second call should have received state - SdkResolverService.Instance.ResolveSdk(submissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true).Path.ShouldBe("resolverpath"); + service.ResolveSdk(submissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true).Path.ShouldBe("resolverpath"); } [Fact] @@ -255,11 +328,12 @@ public void AssertResolversLoadedIfDefaultResolverSucceeds() const int submissionId = BuildEventContext.InvalidSubmissionId; MockLoaderStrategy mockLoaderStrategy = new MockLoaderStrategy(includeDefaultResolver: true); - SdkResolverService.Instance.InitializeForTests(mockLoaderStrategy); + var service = new SdkResolverService(); + service.InitializeForTests(mockLoaderStrategy); SdkReference sdk = new SdkReference("notfound", "1.0", "minimumVersion"); - SdkResolverService.Instance.ResolveSdk(submissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true).Path.ShouldBe("defaultpath"); + service.ResolveSdk(submissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true).Path.ShouldBe("defaultpath"); #if NETCOREAPP // On Core, we check the default resolver *first*, so regular resolvers are not loaded. @@ -388,9 +462,10 @@ public void SdkResolverCanReturnNoPaths(bool includePropertiesAndItems) itemsToAdd, warnings: null)); - SdkResolverService.Instance.InitializeForTests(null, new List() { resolver }); + var service = new SdkResolverService(); + service.InitializeForTests(null, new List() { resolver }); - var result = SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); + var result = service.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); result.Success.ShouldBeTrue(); result.Path.ShouldBeNull(); @@ -424,9 +499,10 @@ public void SdkResultCanReturnPropertiesAndItems() itemsToAdd, warnings: null)); - SdkResolverService.Instance.InitializeForTests(null, new List() { resolver }); + var service = new SdkResolverService(); + service.InitializeForTests(null, new List() { resolver }); - var result = SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); + var result = service.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); result.Success.ShouldBeTrue(); result.Path.ShouldBe(expectedPath); @@ -470,9 +546,10 @@ public void SdkResultCanReturnMultiplePaths(bool includePropertiesAndItems) itemsToAdd, warnings: null)); - SdkResolverService.Instance.InitializeForTests(null, new List() { resolver }); + var service = new SdkResolverService(); + service.InitializeForTests(null, new List() { resolver }); - var result = SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); + var result = service.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); result.Success.ShouldBeTrue(); @@ -515,9 +592,10 @@ public void AssertResolutionWarnsIfResolvedVersionIsDifferentFromReferencedVersi itemsToAdd, warnings: null)); - SdkResolverService.Instance.InitializeForTests(null, new List() { resolver }); + var service = new SdkResolverService(); + service.InitializeForTests(null, new List() { resolver }); - var result = SdkResolverService.Instance.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); + var result = service.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), "sln", "projectPath", interactive: false, isRunningInVisualStudio: false, failOnUnresolvedSdk: true); result.Success.ShouldBeTrue(); diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index 252bf62ab14..e0136f405a1 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -8,6 +8,9 @@ using System.Linq; using System.Reflection; using System.Text.RegularExpressions; +#if DEBUG +using System.Threading; +#endif using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Construction; using Microsoft.Build.Evaluation; @@ -55,6 +58,11 @@ internal class SdkResolverService : ISdkResolverService /// private IList _generalResolversManifestsRegistry; +#if DEBUG + internal bool _fake_initialization = false; + internal IReadOnlyList _fakeManifestRegistry; +#endif + /// /// Stores an which can load registered SDK resolvers. /// @@ -178,6 +186,13 @@ private SdkResult ResolveSdkUsingResolversWithPatternsFirst(int submissionId, Sd List matchingResolversManifests = new(); foreach (SdkResolverManifest manifest in _specificResolversManifestsRegistry) { +#if DEBUG + // If we're checking about the race condition, we should better make sure we would hit it. + if (_fake_initialization) + { + Thread.Sleep(10); + } +#endif try { if (manifest.ResolvableSdkRegex.IsMatch(sdk.Name)) @@ -392,30 +407,37 @@ private bool TryResolveSdkUsingSpecifiedResolvers( /// /// An to use for loading SDK resolvers. /// Explicit set of SdkResolvers to use for all SDK resolution. - internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IReadOnlyList resolvers = null) + /// Debug parameter for initializing only the resolver part + internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IReadOnlyList resolvers = null, bool resolverOnly = false) { if (resolverLoader != null) { _sdkResolverLoader = resolverLoader; + if (resolverOnly) + { + return; + } } else { _sdkResolverLoader = CachingSdkResolverLoader.Instance; } - _specificResolversManifestsRegistry = null; - _generalResolversManifestsRegistry = null; + List _specificResolversManifestsRegistryPlaceholder = null; + List _generalResolversManifestsRegistryPlaceholder = null; _manifestToResolvers = null; if (resolvers != null) { - _specificResolversManifestsRegistry = new List(); - _generalResolversManifestsRegistry = new List(); + _specificResolversManifestsRegistryPlaceholder = new List(); + _generalResolversManifestsRegistryPlaceholder = new List(); _manifestToResolvers = new Dictionary>(); SdkResolverManifest sdkResolverManifest = new SdkResolverManifest(DisplayName: "TestResolversManifest", Path: null, ResolvableSdkRegex: null); - _generalResolversManifestsRegistry.Add(sdkResolverManifest); + _generalResolversManifestsRegistryPlaceholder.Add(sdkResolverManifest); _manifestToResolvers[sdkResolverManifest] = resolvers; + _generalResolversManifestsRegistry = _generalResolversManifestsRegistryPlaceholder.AsReadOnly(); + _specificResolversManifestsRegistry = _specificResolversManifestsRegistryPlaceholder.AsReadOnly(); } } @@ -465,10 +487,21 @@ private void RegisterResolversManifests(ElementLocation location) { return; } + IReadOnlyList allResolversManifests; +#if DEBUG + if (_fake_initialization) + { + allResolversManifests = _fakeManifestRegistry; + } + else + { +#endif + allResolversManifests = _sdkResolverLoader.GetResolversManifests(location); +#if DEBUG + } +#endif - var allResolversManifests = _sdkResolverLoader.GetResolversManifests(location); - - _manifestToResolvers = new Dictionary>(); + _manifestToResolvers = new Dictionary>(); SdkResolverManifest sdkDefaultResolversManifest = null; #if NETCOREAPP @@ -484,11 +517,17 @@ private void RegisterResolversManifests(ElementLocation location) } } - // Break the list of all resolvers manifests into two parts: manifests with specific and general resolvers. _specificResolversManifestsRegistry = new List(); _generalResolversManifestsRegistry = new List(); + foreach (SdkResolverManifest manifest in allResolversManifests) { +#if DEBUG + if (_fake_initialization) + { + Thread.Sleep(10); + } +#endif if (manifest.ResolvableSdkRegex == null) { _generalResolversManifestsRegistry.Add(manifest); From e8a032812838e4e820275412abc2e9c81f817b39 Mon Sep 17 00:00:00 2001 From: SimaTian Date: Fri, 6 Dec 2024 11:47:18 +0100 Subject: [PATCH 02/10] updating the lists only after they're complete to avoid the contention. --- .../BackEnd/SdkResolverService_Tests.cs | 4 +-- .../SdkResolution/SdkResolverService.cs | 27 +++++++++++++------ 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs index 0d5a37648f9..afad12c20e5 100644 --- a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs +++ b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs @@ -168,8 +168,8 @@ internal void TryResolveSdk(out bool success) [Fact] - // Scenario: we want to test that we solved the race described here: https://github.com/dotnet/msbuild/issues/7927#issuecomment-1232470838 - public void AssertResolverPopulationRaceNotPresent() + // Scenario: we want to test that we solved the contention described here: https://github.com/dotnet/msbuild/issues/7927#issuecomment-1232470838 + public void AssertResolverPopulationContentionNotPresent() { s_sdkResolverService.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true), resolverOnly: true); diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index e0136f405a1..d1af2ff268c 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -51,12 +51,12 @@ internal class SdkResolverService : ISdkResolverService /// /// Stores the list of manifests of specific SDK resolvers which could be loaded. /// - private IList _specificResolversManifestsRegistry; + private IReadOnlyList _specificResolversManifestsRegistry; /// /// Stores the list of manifests of general SDK resolvers which could be loaded. /// - private IList _generalResolversManifestsRegistry; + private IReadOnlyList _generalResolversManifestsRegistry; #if DEBUG internal bool _fake_initialization = false; @@ -273,7 +273,7 @@ private SdkResult ResolveSdkUsingResolversWithPatternsFirst(int submissionId, Sd return new SdkResult(sdk, null, null); } - private List GetResolvers(IList resolversManifests, LoggingContext loggingContext, ElementLocation sdkReferenceLocation) + private List GetResolvers(IReadOnlyList resolversManifests, LoggingContext loggingContext, ElementLocation sdkReferenceLocation) { // Create a sorted by priority list of resolvers. Load them if needed. List resolvers = new List(); @@ -517,9 +517,12 @@ private void RegisterResolversManifests(ElementLocation location) } } - _specificResolversManifestsRegistry = new List(); - _generalResolversManifestsRegistry = new List(); + var _specificResolversManifestsRegistryPlaceholder = new List(); + var _generalResolversManifestsRegistryPlaceholder = new List(); + // Break the list of all resolvers manifests into two parts: manifests with specific and general resolvers. + // Since the collections are meant to be immutable, we have to only ever assign them when they're complete. + // Otherwise race can happen, see https://github.com/dotnet/msbuild/issues/7927 foreach (SdkResolverManifest manifest in allResolversManifests) { #if DEBUG @@ -530,17 +533,25 @@ private void RegisterResolversManifests(ElementLocation location) #endif if (manifest.ResolvableSdkRegex == null) { - _generalResolversManifestsRegistry.Add(manifest); + _generalResolversManifestsRegistryPlaceholder.Add(manifest); } else { - _specificResolversManifestsRegistry.Add(manifest); + _specificResolversManifestsRegistryPlaceholder.Add(manifest); } } if (sdkDefaultResolversManifest != null) { - _generalResolversManifestsRegistry.Add(sdkDefaultResolversManifest); + _generalResolversManifestsRegistryPlaceholder.Add(sdkDefaultResolversManifest); } + + // Until this is set(and this is under lock), the ResolveSdkUsingResolversWithPatternsFirst will always + // enter if branch leaving to this section. + // Then it will wait at the lock and return after we release it since the collections we have filled them before releasing the lock. + // The collections are never modified after this point. + // So I've made them ReadOnly + _generalResolversManifestsRegistry = _generalResolversManifestsRegistryPlaceholder.AsReadOnly(); + _specificResolversManifestsRegistry = _specificResolversManifestsRegistryPlaceholder.AsReadOnly(); } } From 37048d6b2dd7f562d55d502cbda63d9e65f3b88b Mon Sep 17 00:00:00 2001 From: Tomas Bartonek Date: Mon, 9 Dec 2024 12:31:09 +0100 Subject: [PATCH 03/10] Update src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs Co-authored-by: Rainer Sigwald --- .../BackEnd/Components/SdkResolution/SdkResolverService.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index d1af2ff268c..1600841d22a 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -494,12 +494,10 @@ private void RegisterResolversManifests(ElementLocation location) allResolversManifests = _fakeManifestRegistry; } else - { #endif + { allResolversManifests = _sdkResolverLoader.GetResolversManifests(location); -#if DEBUG } -#endif _manifestToResolvers = new Dictionary>(); From 5de4f3e503865d03fe0a421a1e75b8ce21217c07 Mon Sep 17 00:00:00 2001 From: SimaTian Date: Mon, 9 Dec 2024 13:34:22 +0100 Subject: [PATCH 04/10] addressing review comments --- .../BackEnd/SdkResolverService_Tests.cs | 56 +++++++------------ .../SdkResolution/SdkResolverService.cs | 45 +++++++++------ 2 files changed, 47 insertions(+), 54 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs index afad12c20e5..aff0931fcd1 100644 --- a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs +++ b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Diagnostics.Tracing; using System.Linq; +using System.Runtime.CompilerServices; using System.Text.RegularExpressions; using System.Threading; using System.Threading.Tasks; @@ -26,16 +27,14 @@ namespace Microsoft.Build.Engine.UnitTests.BackEnd { - public class SdkResolverService_Tests : IDisposable + public class SdkResolverService_Tests { private readonly MockLogger _logger; private readonly LoggingContext _loggingContext; - private static SdkResolverService s_sdkResolverService; public SdkResolverService_Tests() { - s_sdkResolverService = new SdkResolverService(); _logger = new MockLogger(); ILoggingService loggingService = LoggingService.CreateLoggingService(LoggerMode.Synchronous, 1); loggingService.RegisterLogger(_logger); @@ -45,12 +44,6 @@ public SdkResolverService_Tests() new BuildEventContext(0, 0, BuildEventContext.InvalidProjectContextId, 0, 0)); } - public void Dispose() - { - var service = new SdkResolverService(); - service.InitializeForTests(); - } - [Fact] // Scenario: Sdk is not resolved. public void AssertAllResolverErrorsLoggedWhenSdkNotResolved() @@ -144,13 +137,13 @@ public void AssertSecondResolverWithPatternCanResolve() } #if DEBUG - internal void TryResolveSdk(out bool success) + internal string TryResolveSdk(SdkResolverService service) { - success = true; + var message = ""; SdkReference sdk = new SdkReference("2sdkName", "referencedVersion", "minimumVersion"); try { - s_sdkResolverService.ResolveSdk(BuildEventContext.InvalidSubmissionId, + service.ResolveSdk(BuildEventContext.InvalidSubmissionId, sdk, _loggingContext, new MockElementLocation("file"), @@ -160,43 +153,32 @@ internal void TryResolveSdk(out bool success) isRunningInVisualStudio: false, failOnUnresolvedSdk: true); } - catch (Exception) + catch (Exception e) { - success = false; + message = e.ToString(); } + return message; } [Fact] // Scenario: we want to test that we solved the contention described here: https://github.com/dotnet/msbuild/issues/7927#issuecomment-1232470838 - public void AssertResolverPopulationContentionNotPresent() + public async Task AssertResolverPopulationContentionNotPresent() { - s_sdkResolverService.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true), resolverOnly: true); - - List manifests = new List(); - for (int i = 1; i != 20; i++) - { - var man = new SdkResolverManifest(DisplayName: "TestResolversManifest", Path: null, ResolvableSdkRegex: new Regex("abc")); - manifests.Add(man); - man = new SdkResolverManifest(DisplayName: "TestResolversManifest", Path: null, null); - manifests.Add(man); - } - s_sdkResolverService._fakeManifestRegistry = manifests.AsReadOnly(); - s_sdkResolverService._fake_initialization = true; + var service = new SdkResolverService(); + service.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true), contentionConditionTest: true); SdkReference sdk = new SdkReference("2sdkName", "referencedVersion", "minimumVersion"); - bool result1 = false; - bool result2 = false; - Thread thread1 = new Thread(() => TryResolveSdk(out result1)); - Thread thread2 = new Thread(() => TryResolveSdk(out result2)); - thread1.Start(); + var res1 = Task.Run(() => TryResolveSdk(service)); + Thread.Sleep(200); - thread2.Start(); - thread2.Join(); - thread1.Join(); - Assert.True(result1); - Assert.True(result2); + var res2 = Task.Run(() => TryResolveSdk(service)); + string message1 = await res1; + string message2 = await res2; + + Assert.Equal("", message1); + Assert.Equal("", message2); } #endif diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index 1600841d22a..a1352717a16 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -407,14 +407,25 @@ private bool TryResolveSdkUsingSpecifiedResolvers( /// /// An to use for loading SDK resolvers. /// Explicit set of SdkResolvers to use for all SDK resolution. - /// Debug parameter for initializing only the resolver part - internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IReadOnlyList resolvers = null, bool resolverOnly = false) + /// Debug parameter for initializing only the stuff required for the Contention Condition check test. + internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IReadOnlyList resolvers = null, bool contentionConditionTest = false) { if (resolverLoader != null) { _sdkResolverLoader = resolverLoader; - if (resolverOnly) + + if (contentionConditionTest) { + _fake_initialization = true; + List manifests = new List(); + for (int i = 1; i != 20; i++) + { + var man = new SdkResolverManifest(DisplayName: "TestResolversManifest", Path: null, ResolvableSdkRegex: new Regex("abc")); + manifests.Add(man); + man = new SdkResolverManifest(DisplayName: "TestResolversManifest", Path: null, null); + manifests.Add(man); + } + _fakeManifestRegistry = manifests.AsReadOnly(); return; } } @@ -423,21 +434,21 @@ internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IReadO _sdkResolverLoader = CachingSdkResolverLoader.Instance; } - List _specificResolversManifestsRegistryPlaceholder = null; - List _generalResolversManifestsRegistryPlaceholder = null; + List specificResolversManifestsRegistryPlaceholder = null; + List generalResolversManifestsRegistryPlaceholder = null; _manifestToResolvers = null; if (resolvers != null) { - _specificResolversManifestsRegistryPlaceholder = new List(); - _generalResolversManifestsRegistryPlaceholder = new List(); + specificResolversManifestsRegistryPlaceholder = new List(); + generalResolversManifestsRegistryPlaceholder = new List(); _manifestToResolvers = new Dictionary>(); SdkResolverManifest sdkResolverManifest = new SdkResolverManifest(DisplayName: "TestResolversManifest", Path: null, ResolvableSdkRegex: null); - _generalResolversManifestsRegistryPlaceholder.Add(sdkResolverManifest); + generalResolversManifestsRegistryPlaceholder.Add(sdkResolverManifest); _manifestToResolvers[sdkResolverManifest] = resolvers; - _generalResolversManifestsRegistry = _generalResolversManifestsRegistryPlaceholder.AsReadOnly(); - _specificResolversManifestsRegistry = _specificResolversManifestsRegistryPlaceholder.AsReadOnly(); + _generalResolversManifestsRegistry = generalResolversManifestsRegistryPlaceholder.AsReadOnly(); + _specificResolversManifestsRegistry = specificResolversManifestsRegistryPlaceholder.AsReadOnly(); } } @@ -515,8 +526,8 @@ private void RegisterResolversManifests(ElementLocation location) } } - var _specificResolversManifestsRegistryPlaceholder = new List(); - var _generalResolversManifestsRegistryPlaceholder = new List(); + var specificResolversManifestsRegistryPlaceholder = new List(); + var generalResolversManifestsRegistryPlaceholder = new List(); // Break the list of all resolvers manifests into two parts: manifests with specific and general resolvers. // Since the collections are meant to be immutable, we have to only ever assign them when they're complete. @@ -531,16 +542,16 @@ private void RegisterResolversManifests(ElementLocation location) #endif if (manifest.ResolvableSdkRegex == null) { - _generalResolversManifestsRegistryPlaceholder.Add(manifest); + generalResolversManifestsRegistryPlaceholder.Add(manifest); } else { - _specificResolversManifestsRegistryPlaceholder.Add(manifest); + specificResolversManifestsRegistryPlaceholder.Add(manifest); } } if (sdkDefaultResolversManifest != null) { - _generalResolversManifestsRegistryPlaceholder.Add(sdkDefaultResolversManifest); + generalResolversManifestsRegistryPlaceholder.Add(sdkDefaultResolversManifest); } // Until this is set(and this is under lock), the ResolveSdkUsingResolversWithPatternsFirst will always @@ -548,8 +559,8 @@ private void RegisterResolversManifests(ElementLocation location) // Then it will wait at the lock and return after we release it since the collections we have filled them before releasing the lock. // The collections are never modified after this point. // So I've made them ReadOnly - _generalResolversManifestsRegistry = _generalResolversManifestsRegistryPlaceholder.AsReadOnly(); - _specificResolversManifestsRegistry = _specificResolversManifestsRegistryPlaceholder.AsReadOnly(); + _specificResolversManifestsRegistry = specificResolversManifestsRegistryPlaceholder.AsReadOnly(); + _generalResolversManifestsRegistry = generalResolversManifestsRegistryPlaceholder.AsReadOnly(); } } From 5488c7b449f8759287b8e2d63ae9b169b68f0d5a Mon Sep 17 00:00:00 2001 From: SimaTian Date: Mon, 9 Dec 2024 13:37:32 +0100 Subject: [PATCH 05/10] minor touchup --- src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs index aff0931fcd1..3ce737193af 100644 --- a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs +++ b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs @@ -5,7 +5,6 @@ using System.Collections.Generic; using System.Diagnostics.Tracing; using System.Linq; -using System.Runtime.CompilerServices; using System.Text.RegularExpressions; using System.Threading; using System.Threading.Tasks; @@ -32,7 +31,6 @@ public class SdkResolverService_Tests private readonly MockLogger _logger; private readonly LoggingContext _loggingContext; - public SdkResolverService_Tests() { _logger = new MockLogger(); From 802facbc0a315f01f4376d1588cd94d1264e0fbc Mon Sep 17 00:00:00 2001 From: SimaTian Date: Mon, 9 Dec 2024 13:52:52 +0100 Subject: [PATCH 06/10] #if DEBUG fix --- .../BackEnd/Components/SdkResolution/SdkResolverService.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index a1352717a16..04fdd675fb2 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -413,7 +413,7 @@ internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IReadO if (resolverLoader != null) { _sdkResolverLoader = resolverLoader; - +#if DEBUG if (contentionConditionTest) { _fake_initialization = true; @@ -428,6 +428,7 @@ internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IReadO _fakeManifestRegistry = manifests.AsReadOnly(); return; } +#endif } else { From 14850f3d75b7a4263003c182f470a829ad0bdb83 Mon Sep 17 00:00:00 2001 From: SimaTian Date: Fri, 13 Dec 2024 16:28:27 +0100 Subject: [PATCH 07/10] refactoring to get rid of #if directives --- .../BackEnd/SdkResolverService_Tests.cs | 43 +++++++++++- .../SdkResolution/SdkResolverService.cs | 65 ++++--------------- 2 files changed, 55 insertions(+), 53 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs index 3ce737193af..4a891408cc9 100644 --- a/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs +++ b/src/Build.UnitTests/BackEnd/SdkResolverService_Tests.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Configuration; using System.Diagnostics.Tracing; using System.Linq; using System.Text.RegularExpressions; @@ -163,8 +164,8 @@ internal string TryResolveSdk(SdkResolverService service) // Scenario: we want to test that we solved the contention described here: https://github.com/dotnet/msbuild/issues/7927#issuecomment-1232470838 public async Task AssertResolverPopulationContentionNotPresent() { - var service = new SdkResolverService(); - service.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true), contentionConditionTest: true); + var service = new SdkResolverServiceTextExtension(); + service.InitializeForTests(new MockLoaderStrategy(includeResolversWithPatterns: true)); SdkReference sdk = new SdkReference("2sdkName", "referencedVersion", "minimumVersion"); @@ -697,6 +698,44 @@ public void IsRunningInVisualStudioIsSetForResolverContext() isRunningInVisualStudio.ShouldBeTrue(); } + internal sealed class SdkResolverServiceTextExtension : SdkResolverService + { + + internal bool _fake_initialization = false; + internal IReadOnlyList _fakeManifestRegistry; + + internal override void WaitIfTestRequires() + { + if (_fake_initialization) + { + Thread.Sleep(10); + } + } + internal override IReadOnlyList GetResolverManifests(ElementLocation location) + { + return _fakeManifestRegistry; + } + + internal override void InitializeForTests(SdkResolverLoader resolverLoader = null, IReadOnlyList resolvers = null) + { + if (resolverLoader != null) + { + _sdkResolverLoader = resolverLoader; + _fake_initialization = true; + List manifests = new List(); + for (int i = 1; i != 20; i++) + { + var man = new SdkResolverManifest(DisplayName: "TestResolversManifest", Path: null, ResolvableSdkRegex: new Regex("abc")); + manifests.Add(man); + man = new SdkResolverManifest(DisplayName: "TestResolversManifest", Path: null, null); + manifests.Add(man); + } + _fakeManifestRegistry = manifests.AsReadOnly(); + return; + } + } + } + private sealed class MockLoaderStrategy : SdkResolverLoader { private List _resolvers; diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index 04fdd675fb2..e42f112613b 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -10,6 +10,7 @@ using System.Text.RegularExpressions; #if DEBUG using System.Threading; + #endif using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Construction; @@ -51,17 +52,12 @@ internal class SdkResolverService : ISdkResolverService /// /// Stores the list of manifests of specific SDK resolvers which could be loaded. /// - private IReadOnlyList _specificResolversManifestsRegistry; + protected IReadOnlyList _specificResolversManifestsRegistry; /// /// Stores the list of manifests of general SDK resolvers which could be loaded. /// - private IReadOnlyList _generalResolversManifestsRegistry; - -#if DEBUG - internal bool _fake_initialization = false; - internal IReadOnlyList _fakeManifestRegistry; -#endif + protected IReadOnlyList _generalResolversManifestsRegistry; /// /// Stores an which can load registered SDK resolvers. @@ -70,7 +66,7 @@ internal class SdkResolverService : ISdkResolverService /// Unless the 17.10 changewave is disabled, we use a singleton instance because the set of SDK resolvers /// is not expected to change during the lifetime of the process. /// - private SdkResolverLoader _sdkResolverLoader = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10) + protected SdkResolverLoader _sdkResolverLoader = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10) ? CachingSdkResolverLoader.Instance : new SdkResolverLoader(); @@ -186,13 +182,7 @@ private SdkResult ResolveSdkUsingResolversWithPatternsFirst(int submissionId, Sd List matchingResolversManifests = new(); foreach (SdkResolverManifest manifest in _specificResolversManifestsRegistry) { -#if DEBUG - // If we're checking about the race condition, we should better make sure we would hit it. - if (_fake_initialization) - { - Thread.Sleep(10); - } -#endif + WaitIfTestRequires(); try { if (manifest.ResolvableSdkRegex.IsMatch(sdk.Name)) @@ -402,33 +392,20 @@ private bool TryResolveSdkUsingSpecifiedResolvers( return false; } + internal virtual void WaitIfTestRequires() { } + + internal virtual IReadOnlyList GetResolverManifests(ElementLocation location) => _sdkResolverLoader.GetResolversManifests(location); + /// /// Used for unit tests only. This is currently only called through reflection in Microsoft.Build.Engine.UnitTests.TransientSdkResolution.CallResetForTests /// /// An to use for loading SDK resolvers. /// Explicit set of SdkResolvers to use for all SDK resolution. - /// Debug parameter for initializing only the stuff required for the Contention Condition check test. - internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IReadOnlyList resolvers = null, bool contentionConditionTest = false) + internal virtual void InitializeForTests(SdkResolverLoader resolverLoader = null, IReadOnlyList resolvers = null) { if (resolverLoader != null) { _sdkResolverLoader = resolverLoader; -#if DEBUG - if (contentionConditionTest) - { - _fake_initialization = true; - List manifests = new List(); - for (int i = 1; i != 20; i++) - { - var man = new SdkResolverManifest(DisplayName: "TestResolversManifest", Path: null, ResolvableSdkRegex: new Regex("abc")); - manifests.Add(man); - man = new SdkResolverManifest(DisplayName: "TestResolversManifest", Path: null, null); - manifests.Add(man); - } - _fakeManifestRegistry = manifests.AsReadOnly(); - return; - } -#endif } else { @@ -499,19 +476,9 @@ private void RegisterResolversManifests(ElementLocation location) { return; } - IReadOnlyList allResolversManifests; -#if DEBUG - if (_fake_initialization) - { - allResolversManifests = _fakeManifestRegistry; - } - else -#endif - { - allResolversManifests = _sdkResolverLoader.GetResolversManifests(location); - } - _manifestToResolvers = new Dictionary>(); + IReadOnlyList allResolversManifests = GetResolverManifests(location); + _manifestToResolvers = new Dictionary>(); SdkResolverManifest sdkDefaultResolversManifest = null; #if NETCOREAPP @@ -535,12 +502,8 @@ private void RegisterResolversManifests(ElementLocation location) // Otherwise race can happen, see https://github.com/dotnet/msbuild/issues/7927 foreach (SdkResolverManifest manifest in allResolversManifests) { -#if DEBUG - if (_fake_initialization) - { - Thread.Sleep(10); - } -#endif + WaitIfTestRequires(); + if (manifest.ResolvableSdkRegex == null) { generalResolversManifestsRegistryPlaceholder.Add(manifest); From ce5dc260e094e5b85839f3bc7ee71e5a7590346e Mon Sep 17 00:00:00 2001 From: SimaTian Date: Fri, 13 Dec 2024 16:34:18 +0100 Subject: [PATCH 08/10] removing unnecessary include --- .../BackEnd/Components/SdkResolution/SdkResolverService.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index e42f112613b..68f2895d861 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -8,10 +8,6 @@ using System.Linq; using System.Reflection; using System.Text.RegularExpressions; -#if DEBUG -using System.Threading; - -#endif using Microsoft.Build.BackEnd.Logging; using Microsoft.Build.Construction; using Microsoft.Build.Evaluation; @@ -394,6 +390,8 @@ private bool TryResolveSdkUsingSpecifiedResolvers( internal virtual void WaitIfTestRequires() { } + // This is a convenience wrapper that we override for one test so that we don't introduce unnecessary #if DEBUG + // segments into the production code. internal virtual IReadOnlyList GetResolverManifests(ElementLocation location) => _sdkResolverLoader.GetResolversManifests(location); /// From 39aadf99903c7bfc58c63b7e3fb8a9ec36d0b9c7 Mon Sep 17 00:00:00 2001 From: SimaTian Date: Mon, 16 Dec 2024 09:52:58 +0100 Subject: [PATCH 09/10] variable rename --- .../SdkResolution/SdkResolverService.cs | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs index 68f2895d861..a2bfdcf6bec 100644 --- a/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs +++ b/src/Build/BackEnd/Components/SdkResolution/SdkResolverService.cs @@ -410,21 +410,21 @@ internal virtual void InitializeForTests(SdkResolverLoader resolverLoader = null _sdkResolverLoader = CachingSdkResolverLoader.Instance; } - List specificResolversManifestsRegistryPlaceholder = null; - List generalResolversManifestsRegistryPlaceholder = null; + List specificResolversManifestsRegistry = null; + List generalResolversManifestsRegistry = null; _manifestToResolvers = null; if (resolvers != null) { - specificResolversManifestsRegistryPlaceholder = new List(); - generalResolversManifestsRegistryPlaceholder = new List(); + specificResolversManifestsRegistry = new List(); + generalResolversManifestsRegistry = new List(); _manifestToResolvers = new Dictionary>(); SdkResolverManifest sdkResolverManifest = new SdkResolverManifest(DisplayName: "TestResolversManifest", Path: null, ResolvableSdkRegex: null); - generalResolversManifestsRegistryPlaceholder.Add(sdkResolverManifest); + generalResolversManifestsRegistry.Add(sdkResolverManifest); _manifestToResolvers[sdkResolverManifest] = resolvers; - _generalResolversManifestsRegistry = generalResolversManifestsRegistryPlaceholder.AsReadOnly(); - _specificResolversManifestsRegistry = specificResolversManifestsRegistryPlaceholder.AsReadOnly(); + _generalResolversManifestsRegistry = generalResolversManifestsRegistry.AsReadOnly(); + _specificResolversManifestsRegistry = specificResolversManifestsRegistry.AsReadOnly(); } } @@ -492,8 +492,8 @@ private void RegisterResolversManifests(ElementLocation location) } } - var specificResolversManifestsRegistryPlaceholder = new List(); - var generalResolversManifestsRegistryPlaceholder = new List(); + var specificResolversManifestsRegistry = new List(); + var generalResolversManifestsRegistry = new List(); // Break the list of all resolvers manifests into two parts: manifests with specific and general resolvers. // Since the collections are meant to be immutable, we have to only ever assign them when they're complete. @@ -504,16 +504,16 @@ private void RegisterResolversManifests(ElementLocation location) if (manifest.ResolvableSdkRegex == null) { - generalResolversManifestsRegistryPlaceholder.Add(manifest); + generalResolversManifestsRegistry.Add(manifest); } else { - specificResolversManifestsRegistryPlaceholder.Add(manifest); + specificResolversManifestsRegistry.Add(manifest); } } if (sdkDefaultResolversManifest != null) { - generalResolversManifestsRegistryPlaceholder.Add(sdkDefaultResolversManifest); + generalResolversManifestsRegistry.Add(sdkDefaultResolversManifest); } // Until this is set(and this is under lock), the ResolveSdkUsingResolversWithPatternsFirst will always @@ -521,8 +521,8 @@ private void RegisterResolversManifests(ElementLocation location) // Then it will wait at the lock and return after we release it since the collections we have filled them before releasing the lock. // The collections are never modified after this point. // So I've made them ReadOnly - _specificResolversManifestsRegistry = specificResolversManifestsRegistryPlaceholder.AsReadOnly(); - _generalResolversManifestsRegistry = generalResolversManifestsRegistryPlaceholder.AsReadOnly(); + _specificResolversManifestsRegistry = specificResolversManifestsRegistry.AsReadOnly(); + _generalResolversManifestsRegistry = generalResolversManifestsRegistry.AsReadOnly(); } } From ebddc30f00984b714beb53febefc29119643ded1 Mon Sep 17 00:00:00 2001 From: YuliiaKovalova <95473390+YuliiaKovalova@users.noreply.github.com> Date: Mon, 31 Mar 2025 17:11:26 +0200 Subject: [PATCH 10/10] Update Versions.props to 17.12.34 --- eng/Versions.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eng/Versions.props b/eng/Versions.props index ff7ce20085b..ab30465a5ee 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -2,7 +2,7 @@ - 17.12.33 + 17.12.34 release 17.11.4 15.1.0.0