Fix a thread pool deadlock issue (dotnet#6822)

Fixes dotnet#6780:
- `ShouldStopProcessingWorkNow` was checking the count of existing threads and decrementing the count of processing threads. Due to differences from CoreCLR, the condition needs to be different. The effect was that when hill climbing decides to crease the thread count goal, `ShouldStop...` stops all threads that were processing work if there are enough existing threads, and even though there are several thread requests, the now-all-waiting threads are not released to process more work, leading to deadlock. The condition in `ShouldAdjustMaxWorkersActive` was also incorrect, fixed both.
- Fixed a few other small things that I saw

Author: kouvel

src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.HillClimbing.cs

@@ -23,19 +23,19 @@ private partial class HillClimbing
             private static HillClimbing CreateHillClimber()
             {
                 // Default values pulled from CoreCLR
-                return new HillClimbing(wavePeriod: AppContextConfigHelper.GetInt32Config("HillClimbing_WavePeriod", 4),
-                    maxWaveMagnitude: AppContextConfigHelper.GetInt32Config("HillClimbing_MaxWaveMagnitude", 20),
-                    waveMagnitudeMultiplier: AppContextConfigHelper.GetInt32Config("HillClimbing_WaveMagnitudeMultiplier", 100) / 100.0,
-                    waveHistorySize: AppContextConfigHelper.GetInt32Config("HillClimbing_WaveHistorySize", 8),
-                    targetThroughputRatio: AppContextConfigHelper.GetInt32Config("HillClimbing_Bias", 15) / 100.0,
-                    targetSignalToNoiseRatio: AppContextConfigHelper.GetInt32Config("HillClimbing_TargetSignalToNoiseRatio", 300) / 100.0,
-                    maxChangePerSecond: AppContextConfigHelper.GetInt32Config("HillClimbing_MaxChangePerSecond", 4),
-                    maxChangePerSample: AppContextConfigHelper.GetInt32Config("HillClimbing_MaxChangePerSample", 20),
-                    sampleIntervalMsLow: AppContextConfigHelper.GetInt32Config("HillClimbing_SampleIntervalLow", DefaultSampleIntervalMsLow, allowNegative: false),
-                    sampleIntervalMsHigh: AppContextConfigHelper.GetInt32Config("HillClimbing_SampleIntervalHigh", DefaultSampleIntervalMsHigh, allowNegative: false),
-                    errorSmoothingFactor: AppContextConfigHelper.GetInt32Config("HillClimbing_ErrorSmoothingFactor", 1) / 100.0,
-                    gainExponent: AppContextConfigHelper.GetInt32Config("HillClimbing_GainExponent", 200) / 100.0,
-                    maxSampleError: AppContextConfigHelper.GetInt32Config("HillClimbing_MaxSampleErrorPercent", 15) / 100.0
+                return new HillClimbing(wavePeriod: AppContextConfigHelper.GetInt32Config("HillClimbing_WavePeriod", 4, false),
+                    maxWaveMagnitude: AppContextConfigHelper.GetInt32Config("HillClimbing_MaxWaveMagnitude", 20, false),
+                    waveMagnitudeMultiplier: AppContextConfigHelper.GetInt32Config("HillClimbing_WaveMagnitudeMultiplier", 100, false) / 100.0,
+                    waveHistorySize: AppContextConfigHelper.GetInt32Config("HillClimbing_WaveHistorySize", 8, false),
+                    targetThroughputRatio: AppContextConfigHelper.GetInt32Config("HillClimbing_Bias", 15, false) / 100.0,
+                    targetSignalToNoiseRatio: AppContextConfigHelper.GetInt32Config("HillClimbing_TargetSignalToNoiseRatio", 300, false) / 100.0,
+                    maxChangePerSecond: AppContextConfigHelper.GetInt32Config("HillClimbing_MaxChangePerSecond", 4, false),
+                    maxChangePerSample: AppContextConfigHelper.GetInt32Config("HillClimbing_MaxChangePerSample", 20, false),
+                    sampleIntervalMsLow: AppContextConfigHelper.GetInt32Config("HillClimbing_SampleIntervalLow", DefaultSampleIntervalMsLow, false),
+                    sampleIntervalMsHigh: AppContextConfigHelper.GetInt32Config("HillClimbing_SampleIntervalHigh", DefaultSampleIntervalMsHigh, false),
+                    errorSmoothingFactor: AppContextConfigHelper.GetInt32Config("HillClimbing_ErrorSmoothingFactor", 1, false) / 100.0,
+                    gainExponent: AppContextConfigHelper.GetInt32Config("HillClimbing_GainExponent", 200, false) / 100.0,
+                    maxSampleError: AppContextConfigHelper.GetInt32Config("HillClimbing_MaxSampleErrorPercent", 15, false) / 100.0
                 );
             }
             private const int LogCapacity = 200;

src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.WorkerThread.cs

@@ -23,7 +23,7 @@ private static class WorkerThread
             private static void WorkerThreadStart()
             {
                 ClrThreadPoolEventSource.Log.WorkerThreadStart(ThreadCounts.VolatileReadCounts(ref ThreadPoolInstance._separated.counts).numExistingThreads);
-                RuntimeThread currentThread = RuntimeThread.CurrentThread;
+
                 while (true)
                 {
                     while (WaitForRequest())
@@ -189,7 +189,14 @@ internal static bool ShouldStopProcessingWorkNow()
                 ThreadCounts counts = ThreadCounts.VolatileReadCounts(ref ThreadPoolInstance._separated.counts);
                 while (true)
                 {
-                    if (counts.numExistingThreads <= counts.numThreadsGoal)
+                    /// When there are more threads processing work than the thread count goal, hill climbing must have decided
+                    /// to decrease the number of threads. Stop processing if the counts can be updated. We may have more
+                    /// threads existing than the thread count goal and that is ok, the cold ones will eventually time out if
+                    /// the thread count goal is not increased again. This logic is a bit different from the original CoreCLR
+                    /// code from which this implementation was ported, which turns a processing thread into a retired thread
+                    /// and checks for pending requests like <see cref="RemoveWorkingWorker"/>. In this implementation there are
+                    /// no retired threads, so only the count of threads processing work is considered.
+                    if (counts.numProcessingWork <= counts.numThreadsGoal)
                     {
                         return false;
                     }

src/System.Private.CoreLib/src/System/Threading/ClrThreadPool.cs

@@ -9,26 +9,31 @@ namespace System.Threading
     /// <summary>
     /// A thread-pool run and managed on the CLR.
     /// </summary>
-    internal partial class ClrThreadPool
+    internal sealed partial class ClrThreadPool
     {
 #pragma warning disable IDE1006 // Naming Styles
         public static readonly ClrThreadPool ThreadPoolInstance = new ClrThreadPool();
 #pragma warning restore IDE1006 // Naming Styles
 
         private const int ThreadPoolThreadTimeoutMs = 20 * 1000; // If you change this make sure to change the timeout times in the tests.
-      
+
+#if BIT64
         private const short MaxPossibleThreadCount = short.MaxValue;
+#elif BIT32
+        private const short MaxPossibleThreadCount = 1023;
+#else
+    #error Unknown platform
+#endif
 
         private const int CpuUtilizationHigh = 95;
         private const int CpuUtilizationLow = 80;
         private int _cpuUtilization = 0;
 
+        private static readonly short s_forcedMinWorkerThreads = AppContextConfigHelper.GetInt16Config("System.Threading.ThreadPool.MinThreads", 0, false);
+        private static readonly short s_forcedMaxWorkerThreads = AppContextConfigHelper.GetInt16Config("System.Threading.ThreadPool.MaxThreads", 0, false);
 
-        private static readonly short s_forcedMinWorkerThreads = AppContextConfigHelper.GetInt16Config("System.Threading.ThreadPool.MinThreads", 0);
-        private static readonly short s_forcedMaxWorkerThreads = AppContextConfigHelper.GetInt16Config("System.Threading.ThreadPool.MaxThreads", 0);
-
-        private short _minThreads = (short)ThreadPoolGlobals.processorCount;
-        private short _maxThreads = MaxPossibleThreadCount;
+        private short _minThreads;
+        private short _maxThreads;
         private readonly LowLevelLock _maxMinThreadLock = new LowLevelLock();
 
         [StructLayout(LayoutKind.Explicit, Size = CacheLineSize * 5)]
@@ -62,11 +67,23 @@ private struct CacheLineSeparated
 
         private ClrThreadPool()
         {
+            _minThreads = s_forcedMinWorkerThreads > 0 ? s_forcedMinWorkerThreads : (short)ThreadPoolGlobals.processorCount;
+            if (_minThreads > MaxPossibleThreadCount)
+            {
+                _minThreads = MaxPossibleThreadCount;
+            }
+
+            _maxThreads = s_forcedMaxWorkerThreads > 0 ? s_forcedMaxWorkerThreads : MaxPossibleThreadCount;
+            if (_maxThreads < _minThreads)
+            {
+                _maxThreads = _minThreads;
+            }
+
             _separated = new CacheLineSeparated
             {
                 counts = new ThreadCounts
                 {
-                    numThreadsGoal = s_forcedMinWorkerThreads > 0 ? s_forcedMinWorkerThreads : _minThreads
+                    numThreadsGoal = _minThreads
                 }
             };
         }
@@ -181,23 +198,16 @@ internal bool NotifyWorkItemComplete()
             Interlocked.Increment(ref _completionCount);
             Volatile.Write(ref _separated.lastDequeueTime, Environment.TickCount);
 
-            if (ShouldAdjustMaxWorkersActive())
+            if (ShouldAdjustMaxWorkersActive() && _hillClimbingThreadAdjustmentLock.TryAcquire())
             {
-                bool acquiredLock = _hillClimbingThreadAdjustmentLock.TryAcquire();
                 try
                 {
-                    if (acquiredLock)
-                    {
-                        AdjustMaxWorkersActive();
-                    }
+                    AdjustMaxWorkersActive();
                 }
                 finally
                 {
-                    if (acquiredLock)
-                    {
-                        _hillClimbingThreadAdjustmentLock.Release();
-                    }
-                } 
+                    _hillClimbingThreadAdjustmentLock.Release();
+                }
             }
 
             return !WorkerThread.ShouldStopProcessingWorkNow();
@@ -272,8 +282,15 @@ private bool ShouldAdjustMaxWorkersActive()
             int elapsedInterval = Environment.TickCount - priorTime;
             if(elapsedInterval >= requiredInterval)
             {
+                /// Avoid trying to adjust the thread count goal if there are already more threads than the thread count goal.
+                /// In that situation, hill climbing must have previously decided to decrease the thread count goal, so let's
+                /// wait until the system responds to that change before calling into hill climbing again. This condition should
+                /// be the opposite of the condition in <see cref="WorkerThread.ShouldStopProcessingWorkNow"/> that causes
+                /// threads processing work to stop in response to a decreased thread count goal. The logic here is a bit
+                /// different from the original CoreCLR code from which this implementation was ported because in this
+                /// implementation there are no retired threads, so only the count of threads processing work is considered.
                 ThreadCounts counts = ThreadCounts.VolatileReadCounts(ref _separated.counts);
-                return counts.numExistingThreads >= counts.numThreadsGoal;
+                return counts.numProcessingWork <= counts.numThreadsGoal;
             }
             return false;
         }

src/System.Private.CoreLib/src/System/Threading/LowLevelLifoSemaphore.Windows.cs

@@ -22,7 +22,11 @@ internal sealed class LowLevelLifoSemaphore : IDisposable
         public LowLevelLifoSemaphore(int initialSignalCount, int maximumSignalCount)
         {
             Debug.Assert(initialSignalCount >= 0, "Windows LowLevelLifoSemaphore does not support a negative signal count"); // TODO: Track actual signal count to enable this
-            _completionPort = Interop.Kernel32.CreateIoCompletionPort(new IntPtr(-1), IntPtr.Zero, UIntPtr.Zero, 1);
+            Debug.Assert(maximumSignalCount > 0);
+            Debug.Assert(initialSignalCount <= maximumSignalCount);
+
+            _completionPort =
+                Interop.Kernel32.CreateIoCompletionPort(new IntPtr(-1), IntPtr.Zero, UIntPtr.Zero, maximumSignalCount);
             if (_completionPort == IntPtr.Zero)
             {
                 var error = Marshal.GetLastWin32Error();
@@ -33,15 +37,27 @@ public LowLevelLifoSemaphore(int initialSignalCount, int maximumSignalCount)
             Release(initialSignalCount);
         }
 
+        ~LowLevelLifoSemaphore()
+        {
+            if (_completionPort != IntPtr.Zero)
+            {
+                Dispose();
+            }
+        }
+
         public bool Wait(int timeoutMs)
         {
+            Debug.Assert(timeoutMs >= -1);
+
             bool success = Interop.Kernel32.GetQueuedCompletionStatus(_completionPort, out var numberOfBytes, out var completionKey, out var pointerToOverlapped, timeoutMs);
             Debug.Assert(success || (Marshal.GetLastWin32Error() == WaitHandle.WaitTimeout));
             return success;
         }
 
         public int Release(int count)
         {
+            Debug.Assert(count > 0);
+
             for (int i = 0; i < count; i++)
             {
                 if(!Interop.Kernel32.PostQueuedCompletionStatus(_completionPort, 1, UIntPtr.Zero, IntPtr.Zero))
@@ -57,7 +73,11 @@ public int Release(int count)
 
         public void Dispose()
         {
+            Debug.Assert(_completionPort != IntPtr.Zero);
+
             Interop.Kernel32.CloseHandle(_completionPort);
+            _completionPort = IntPtr.Zero;
+            GC.SuppressFinalize(this);
         }
     }
 }

src/System.Private.CoreLib/src/System/Threading/ThreadPool.Portable.cs

@@ -207,7 +207,11 @@ private void SignalUserWaitHandle()
                 if (handleValue != IntPtr.Zero && handleValue != (IntPtr)(-1))
                 {
                     Debug.Assert(handleValue == handle.DangerousGetHandle());
+#if PLATFORM_WINDOWS
+                    Interop.Kernel32.SetEvent(handle);
+#else
                     WaitSubsystem.SetEvent(handleValue);
+#endif
                 }
             }
             finally

tests/src/Simple/Threading/Threading.cs

@@ -50,6 +50,9 @@ public static int Main()
         Console.WriteLine("    ThreadPoolTests.ThreadPoolCanPickUpMultipleJobsWhenThreadsAreAvailable");
         ThreadPoolTests.ThreadPoolCanPickUpMultipleJobsWhenThreadsAreAvailable();
 
+        Console.WriteLine("    ThreadPoolTests.ThreadPoolCanProcessManyWorkItemsInParallelWithoutDeadlocking");
+        ThreadPoolTests.ThreadPoolCanProcessManyWorkItemsInParallelWithoutDeadlocking();
+
         // This test takes a long time to run (min 42 seconds sleeping). Enable for manual testing.
         // Console.WriteLine("    ThreadPoolTests.RunJobsAfterThreadTimeout");
         // ThreadPoolTests.RunJobsAfterThreadTimeout();
@@ -950,6 +953,34 @@ void Job(object _)
         e0.Set();
     }
 
+    // See https://github.com/dotnet/corert/issues/6780
+    [Fact]
+    public static void ThreadPoolCanProcessManyWorkItemsInParallelWithoutDeadlocking()
+    {
+        int iterationCount = 100_000;
+        var done = new ManualResetEvent(false);
+
+        WaitCallback wc = null;
+        wc = data =>
+        {
+            int n = Interlocked.Decrement(ref iterationCount);
+            if (n == 0)
+            {
+                done.Set();
+            }
+            else if (n > 0)
+            {
+                ThreadPool.QueueUserWorkItem(wc);
+            }
+        };
+
+        for (int i = 0, n = Environment.ProcessorCount; i < n; ++i)
+        {
+            ThreadPool.QueueUserWorkItem(wc);
+        }
+        done.WaitOne();
+    }
+
     private static WaitCallback CreateRecursiveJob(int jobCount, int targetJobCount, AutoResetEvent testJobCompleted)
     {
         return _ =>