Optimize locking in ProjectCollection (low risk) (#8742)

Fixes AB#1811627 (@davkean will likely file a GH issue)

Context
ProjectCollection uses a ReaderWriterLockSlim but it never takes it just for reading with EnterReadLock. Instead it uses EnterUpgradeableReadLock, which effectively provides mutual exclusion when reading data and results in unnecessary contention. The reason cited in comments revolves around reentrancy but calling EnterReadLock while already holding the write lock is perfectly legal with LockRecursionPolicy.SupportsRecursion so there's no need to mutually exclude readers.

Changes Made
Made ProjectCollection use plain read lock instead of the upgradeable one.
Simplified the IDisposable holder structs by removing the unneeded interlocked operation.
Testing
Existing unit tests.

Notes
This is a safer version of #8728, which attempts to optimize concurrency in this class even further at the expense of readability. In particular, many readers could be converted to volatile reads to eliminate the possibility of contention with writers, some of which may be long-running. Since the reader-writer contention should not happen in VS scenarios, that PR was closed due to unfavorable risk/benefit and left just for future reference.

src/Build/Definition/ProjectCollection.cs

@@ -87,8 +87,8 @@ namespace Microsoft.Build.Evaluation
         /// <remarks>
         /// ProjectCollection is highly reentrant - project creation, toolset and logger changes, and so on
         /// all need lock protection, but there are a lot of read cases as well, and calls to create Projects
-        /// call back to the ProjectCollection under locks. Use a RW lock, but default to always using
-        /// upgradable read locks to avoid adding reentrancy bugs.
+        /// call back to the ProjectCollection under locks. Use a RW lock with recursion support to avoid
+        /// adding reentrancy bugs.
         /// </remarks>
         private readonly ReaderWriterLockSlim _locker = new ReaderWriterLockSlim(LockRecursionPolicy.SupportsRecursion);
 
@@ -508,7 +508,7 @@ namespace Microsoft.Build.Evaluation
         {
             get
             {
-                using (_locker.EnterDisposableUpgradeableReadLock())
+                using (_locker.EnterDisposableReadLock())
                 {
                     ErrorUtilities.VerifyThrow(_defaultToolsVersion != null, "Should have a default");
                     return _defaultToolsVersion;
@@ -558,7 +558,7 @@ namespace Microsoft.Build.Evaluation
             {
                 Dictionary<string, string> dictionary;
 
-                using (_locker.EnterDisposableUpgradeableReadLock())
+                using (_locker.EnterDisposableReadLock())
                 {
                     if (_globalProperties.Count == 0)
                     {
@@ -591,7 +591,7 @@ namespace Microsoft.Build.Evaluation
         {
             get
             {
-                using (_locker.EnterDisposableUpgradeableReadLock())
+                using (_locker.EnterDisposableReadLock())
                 {
                     return _loadedProjects.Count;
                 }
@@ -609,7 +609,7 @@ namespace Microsoft.Build.Evaluation
             [DebuggerStepThrough]
             get
             {
-                using (_locker.EnterDisposableUpgradeableReadLock())
+                using (_locker.EnterDisposableReadLock())
                 {
                     return _loggingService.Loggers == null
                         ? (ICollection<ILogger>)ReadOnlyEmptyCollection<ILogger>.Instance
@@ -628,7 +628,7 @@ namespace Microsoft.Build.Evaluation
         {
             get
             {
-                using (_locker.EnterDisposableUpgradeableReadLock())
+                using (_locker.EnterDisposableReadLock())
                 {
                     return new List<Toolset>(_toolsets.Values);
                 }
@@ -650,7 +650,7 @@ namespace Microsoft.Build.Evaluation
             [DebuggerStepThrough]
             get
             {
-                using (_locker.EnterDisposableUpgradeableReadLock())
+                using (_locker.EnterDisposableReadLock())
                 {
                     return _isBuildEnabled;
                 }
@@ -683,7 +683,7 @@ namespace Microsoft.Build.Evaluation
         {
             get
             {
-                using (_locker.EnterDisposableUpgradeableReadLock())
+                using (_locker.EnterDisposableReadLock())
                 {
                     return _onlyLogCriticalEvents;
                 }
@@ -720,17 +720,20 @@ namespace Microsoft.Build.Evaluation
             get
             {
                 // Avoid write lock if possible, this getter is called a lot during Project construction.
-                using (_locker.EnterDisposableUpgradeableReadLock())
+                using (_locker.EnterDisposableReadLock())
                 {
                     if (_hostServices != null)
                     {
                         return _hostServices;
                     }
-
+                }
                 using (_locker.EnterDisposableWriteLock())
                 {
-                        return _hostServices ?? (_hostServices = new HostServices());
+                    if (_hostServices == null)
+                    {
+                        _hostServices = new HostServices();
                     }
+                    return _hostServices;
                 }
             }
 
@@ -763,7 +766,7 @@ namespace Microsoft.Build.Evaluation
         {
             get
             {
-                using (_locker.EnterDisposableUpgradeableReadLock())
+                using (_locker.EnterDisposableReadLock())
                 {
                     return _skipEvaluation;
                 }
@@ -799,7 +802,7 @@ namespace Microsoft.Build.Evaluation
         {
             get
             {
-                using (_locker.EnterDisposableUpgradeableReadLock())
+                using (_locker.EnterDisposableReadLock())
                 {
                     return _disableMarkDirty;
                 }
@@ -849,7 +852,7 @@ namespace Microsoft.Build.Evaluation
             [DebuggerStepThrough]
             get
             {
-                using (_locker.EnterDisposableUpgradeableReadLock())
+                using (_locker.EnterDisposableReadLock())
                 {
                     return _loggingService;
                 }
@@ -867,7 +870,7 @@ namespace Microsoft.Build.Evaluation
             {
                 var clone = new PropertyDictionary<ProjectPropertyInstance>();
 
-                using (_locker.EnterDisposableUpgradeableReadLock())
+                using (_locker.EnterDisposableReadLock())
                 {
                     foreach (ProjectPropertyInstance property in _globalProperties)
                     {
@@ -885,24 +888,25 @@ namespace Microsoft.Build.Evaluation
         internal PropertyDictionary<ProjectPropertyInstance> EnvironmentProperties
         {
             get
-            {
-                using (_locker.EnterDisposableUpgradeableReadLock())
             {
                 // Retrieves the environment properties.
                 // This is only done once, when the project collection is created. Any subsequent
                 // environment changes will be ignored. Child nodes will be passed this set
                 // of properties in their build parameters.
-                    if (_environmentProperties == null)
+                using (_locker.EnterDisposableReadLock())
                 {
+                    if (_environmentProperties != null)
+                    {
+                        return new PropertyDictionary<ProjectPropertyInstance>(_environmentProperties);
+                    }
+                }
+
                 using (_locker.EnterDisposableWriteLock())
                 {
                     if (_environmentProperties == null)
                     {
                         _environmentProperties = Utilities.GetEnvironmentProperties();
                     }
-                        }
-                    }
-
                     return new PropertyDictionary<ProjectPropertyInstance>(_environmentProperties);
                 }
             }
@@ -917,7 +921,7 @@ namespace Microsoft.Build.Evaluation
             [DebuggerStepThrough]
             get
             {
-                using (_locker.EnterDisposableUpgradeableReadLock())
+                using (_locker.EnterDisposableReadLock())
                 {
                     return _toolsetsVersion;
                 }
@@ -931,7 +935,7 @@ namespace Microsoft.Build.Evaluation
         {
             get
             {
-                using (_locker.EnterDisposableUpgradeableReadLock())
+                using (_locker.EnterDisposableReadLock())
                 {
                     return _maxNodeCount;
                 }
@@ -1419,7 +1423,7 @@ namespace Microsoft.Build.Evaluation
         /// </summary>
         public ProjectPropertyInstance GetGlobalProperty(string name)
         {
-            using (_locker.EnterDisposableUpgradeableReadLock())
+            using (_locker.EnterDisposableReadLock())
             {
                 return _globalProperties[name];
             }

src/Build/Utilities/ReaderWriterLockSlimExtensions.cs

@@ -10,10 +10,10 @@ namespace Microsoft.Build.Internal;
 
 internal static class ReaderWriterLockSlimExtensions
 {
-    public static UpgradeableReadLockDisposer EnterDisposableUpgradeableReadLock(this ReaderWriterLockSlim rwLock)
+    public static DisposableReadLock EnterDisposableReadLock(this ReaderWriterLockSlim rwLock)
     {
-        rwLock.EnterUpgradeableReadLock();
-        return new UpgradeableReadLockDisposer(rwLock);
+        rwLock.EnterReadLock();
+        return new DisposableReadLock(rwLock);
     }
 
     public static DisposableWriteLock EnterDisposableWriteLock(this ReaderWriterLockSlim rwLock)
@@ -22,44 +22,21 @@ internal static class ReaderWriterLockSlimExtensions
         return new DisposableWriteLock(rwLock);
     }
 
-    // Officially, Dispose() being called more than once is allowable, but in this case if that were to happen
-    // that means something is very, very wrong. Since it's an internal type, better to be strict.
-
-    internal struct UpgradeableReadLockDisposer : IDisposable
+    internal readonly struct DisposableReadLock : IDisposable
     {
-        private ReaderWriterLockSlim? _rwLock;
+        private readonly ReaderWriterLockSlim _rwLock;
 
-        public UpgradeableReadLockDisposer(ReaderWriterLockSlim rwLock) => _rwLock = rwLock;
+        public DisposableReadLock(ReaderWriterLockSlim rwLock) => _rwLock = rwLock;
 
-        public void Dispose()
-        {
-            var rwLockToDispose = Interlocked.Exchange(ref _rwLock, null);
-
-            if (rwLockToDispose is null)
-            {
-                throw new ObjectDisposedException($"Somehow a {nameof(UpgradeableReadLockDisposer)} is being disposed twice.");
+        public void Dispose() => _rwLock.ExitReadLock();
     }
 
-            rwLockToDispose.ExitUpgradeableReadLock();
-        }
-    }
-
-    internal struct DisposableWriteLock : IDisposable
+    internal readonly struct DisposableWriteLock : IDisposable
     {
-        private ReaderWriterLockSlim? _rwLock;
+        private readonly ReaderWriterLockSlim _rwLock;
 
         public DisposableWriteLock(ReaderWriterLockSlim rwLock) => _rwLock = rwLock;
 
-        public void Dispose()
-        {
-            var rwLockToDispose = Interlocked.Exchange(ref _rwLock, null);
-
-            if (rwLockToDispose is null)
-            {
-                throw new ObjectDisposedException($"Somehow a {nameof(DisposableWriteLock)} is being disposed twice.");
-            }
-
-            rwLockToDispose.ExitWriteLock();
-        }
+        public void Dispose() => _rwLock.ExitWriteLock();
     }
 }