diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.ThreadCounts.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.ThreadCounts.cs index 43aa0fcf7102c..b3747c0061c1d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.ThreadCounts.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.ThreadCounts.cs @@ -31,61 +31,52 @@ private void SetInt16Value(short value, byte shift) => /// public short NumProcessingWork { - get => GetInt16Value(NumProcessingWorkShift); + get + { + short value = GetInt16Value(NumProcessingWorkShift); + Debug.Assert(value >= 0); + return value; + } set { Debug.Assert(value >= 0); - SetInt16Value(value, NumProcessingWorkShift); + SetInt16Value(Math.Max((short)0, value), NumProcessingWorkShift); } } - public void SubtractNumProcessingWork(short value) - { - Debug.Assert(value >= 0); - Debug.Assert(value <= NumProcessingWork); - - _data -= (ulong)(ushort)value << NumProcessingWorkShift; - } - - public void InterlockedDecrementNumProcessingWork() - { - Debug.Assert(NumProcessingWorkShift == 0); - - ThreadCounts counts = new ThreadCounts(Interlocked.Decrement(ref _data)); - Debug.Assert(counts.NumProcessingWork >= 0); - } - /// /// Number of thread pool threads that currently exist. /// public short NumExistingThreads { - get => GetInt16Value(NumExistingThreadsShift); + get + { + short value = GetInt16Value(NumExistingThreadsShift); + Debug.Assert(value >= 0); + return value; + } set { Debug.Assert(value >= 0); - SetInt16Value(value, NumExistingThreadsShift); + SetInt16Value(Math.Max((short)0, value), NumExistingThreadsShift); } } - public void SubtractNumExistingThreads(short value) - { - Debug.Assert(value >= 0); - Debug.Assert(value <= NumExistingThreads); - - _data -= (ulong)(ushort)value << NumExistingThreadsShift; - } - /// /// Max possible thread pool threads we want to have. /// public short NumThreadsGoal { - get => GetInt16Value(NumThreadsGoalShift); + get + { + short value = GetInt16Value(NumThreadsGoalShift); + Debug.Assert(value > 0); + return value; + } set { Debug.Assert(value > 0); - SetInt16Value(value, NumThreadsGoalShift); + SetInt16Value(Math.Max((short)1, value), NumThreadsGoalShift); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WorkerThread.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WorkerThread.cs index 1298d1be4121d..d42dd58200b8b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WorkerThread.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WorkerThread.cs @@ -114,16 +114,14 @@ private static void WorkerThreadStart() // Since this thread is currently registered as an existing thread, if more work comes in meanwhile, // this thread would be expected to satisfy the new work. Ensure that NumExistingThreads is not // decreased below NumProcessingWork, as that would be indicative of such a case. - short numExistingThreads = counts.NumExistingThreads; - if (numExistingThreads <= counts.NumProcessingWork) + if (counts.NumExistingThreads <= counts.NumProcessingWork) { // In this case, enough work came in that this thread should not time out and should go back to work. break; } ThreadCounts newCounts = counts; - newCounts.SubtractNumExistingThreads(1); - short newNumExistingThreads = (short)(numExistingThreads - 1); + short newNumExistingThreads = --newCounts.NumExistingThreads; short newNumThreadsGoal = Math.Max( threadPoolInstance.MinThreadsGoal, @@ -159,7 +157,23 @@ private static void WorkerThreadStart() /// private static void RemoveWorkingWorker(PortableThreadPool threadPoolInstance) { - threadPoolInstance._separated.counts.InterlockedDecrementNumProcessingWork(); + // A compare-exchange loop is used instead of Interlocked.Decrement or Interlocked.Add to defensively prevent + // NumProcessingWork from underflowing. See the setter for NumProcessingWork. + ThreadCounts counts = threadPoolInstance._separated.counts; + while (true) + { + ThreadCounts newCounts = counts; + newCounts.NumProcessingWork--; + + ThreadCounts countsBeforeUpdate = + threadPoolInstance._separated.counts.InterlockedCompareExchange(newCounts, counts); + if (countsBeforeUpdate == counts) + { + break; + } + + counts = countsBeforeUpdate; + } // It's possible that we decided we had thread requests just before a request came in, // but reduced the worker count *after* the request came in. In this case, we might @@ -221,8 +235,8 @@ internal static void MaybeAddWorkingWorker(PortableThreadPool threadPoolInstance while (true) { ThreadCounts newCounts = counts; - newCounts.SubtractNumProcessingWork((short)toCreate); - newCounts.SubtractNumExistingThreads((short)toCreate); + newCounts.NumProcessingWork -= (short)toCreate; + newCounts.NumExistingThreads -= (short)toCreate; ThreadCounts oldCounts = threadPoolInstance._separated.counts.InterlockedCompareExchange(newCounts, counts); if (oldCounts == counts) @@ -259,7 +273,7 @@ internal static bool ShouldStopProcessingWorkNow(PortableThreadPool threadPoolIn } ThreadCounts newCounts = counts; - newCounts.SubtractNumProcessingWork(1); + newCounts.NumProcessingWork--; ThreadCounts oldCounts = threadPoolInstance._separated.counts.InterlockedCompareExchange(newCounts, counts);