addressing review comments

Author: SimaTian

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<SdkResolverManifest> manifests = new List<SdkResolverManifest>();
-            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
 

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

@@ -407,14 +407,25 @@ private bool TryResolveSdkUsingSpecifiedResolvers(
         /// </summary>
         /// <param name="resolverLoader">An <see cref="SdkResolverLoader"/> to use for loading SDK resolvers.</param>
         /// <param name="resolvers">Explicit set of SdkResolvers to use for all SDK resolution.</param>
-        /// <param name="resolverOnly"> Debug parameter for initializing only the resolver part</param>
-        internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IReadOnlyList<SdkResolver> resolvers = null, bool resolverOnly = false)
+        /// <param name="contentionConditionTest"> Debug parameter for initializing only the stuff required for the Contention Condition check test.</param>
+        internal void InitializeForTests(SdkResolverLoader resolverLoader = null, IReadOnlyList<SdkResolver> resolvers = null, bool contentionConditionTest = false)
         {
             if (resolverLoader != null)
             {
                 _sdkResolverLoader = resolverLoader;
-                if (resolverOnly)
+
+                if (contentionConditionTest)
                 {
+                    _fake_initialization = true;
+                    List<SdkResolverManifest> manifests = new List<SdkResolverManifest>();
+                    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<SdkResolverManifest> _specificResolversManifestsRegistryPlaceholder = null;
-            List<SdkResolverManifest> _generalResolversManifestsRegistryPlaceholder = null;
+            List<SdkResolverManifest> specificResolversManifestsRegistryPlaceholder = null;
+            List<SdkResolverManifest> generalResolversManifestsRegistryPlaceholder = null;
             _manifestToResolvers = null;
 
             if (resolvers != null)
             {
-                _specificResolversManifestsRegistryPlaceholder = new List<SdkResolverManifest>();
-                _generalResolversManifestsRegistryPlaceholder = new List<SdkResolverManifest>();
+                specificResolversManifestsRegistryPlaceholder = new List<SdkResolverManifest>();
+                generalResolversManifestsRegistryPlaceholder = new List<SdkResolverManifest>();
                 _manifestToResolvers = new Dictionary<SdkResolverManifest, IReadOnlyList<SdkResolver>>();
 
                 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<SdkResolverManifest>();
-                var _generalResolversManifestsRegistryPlaceholder = new List<SdkResolverManifest>();
+                var specificResolversManifestsRegistryPlaceholder = new List<SdkResolverManifest>();
+                var generalResolversManifestsRegistryPlaceholder = new List<SdkResolverManifest>();
 
                 // 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,25 +542,25 @@ 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
                 // 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();
+                _specificResolversManifestsRegistry = specificResolversManifestsRegistryPlaceholder.AsReadOnly();
+                _generalResolversManifestsRegistry = generalResolversManifestsRegistryPlaceholder.AsReadOnly();
             }
         }