Fix timer firing bug introduced in commit e652d4d (#6905 / #6898)

src/System.Private.CoreLib/src/System/Threading/Timer.Unix.cs

@@ -19,12 +19,12 @@ internal partial class TimerQueue
         /// This event is used by the timer thread to wait for timer expiration. It is also
         /// used to notify the timer thread that a new timer has been set.
         /// </summary>
-        private static AutoResetEvent s_timerEvent;
+        private AutoResetEvent _timerEvent;
 
         /// <summary>
         /// This field stores the value of next timer that the timer thread should install.
         /// </summary>
-        private static volatile int s_nextTimerDuration;
+        private volatile int _nextTimerDuration;
 
         private TimerQueue(int id)
         {
@@ -35,21 +35,21 @@ private bool SetTimer(uint actualDuration)
             // Note: AutoResetEvent.WaitOne takes an Int32 value as a timeout.
             // The TimerQueue code ensures that timer duration is not greater than max Int32 value
             Debug.Assert(actualDuration <= (uint)int.MaxValue);
-            s_nextTimerDuration = (int)actualDuration;
+            _nextTimerDuration = (int)actualDuration;
 
             // If this is the first time the timer is set then we need to create a thread that
             // will manage and respond to timer requests. Otherwise, simply signal the timer thread
             // to notify it that the timer duration has changed.
-            if (s_timerEvent == null)
+            if (_timerEvent == null)
             {
-                s_timerEvent = new AutoResetEvent(false);
+                _timerEvent = new AutoResetEvent(false);
                 RuntimeThread thread = RuntimeThread.Create(TimerThread, 0);
                 thread.IsBackground = true; // Keep this thread from blocking process shutdown
                 thread.Start();
             }
             else
             {
-                s_timerEvent.Set();
+                _timerEvent.Set();
             }
 
             return true;
@@ -65,15 +65,15 @@ private void TimerThread()
             int currentTimerInterval;
 
             // Get wait time for the next timer
-            currentTimerInterval = Interlocked.Exchange(ref s_nextTimerDuration, Timeout.Infinite);
+            currentTimerInterval = Interlocked.Exchange(ref _nextTimerDuration, Timeout.Infinite);
 
             for (;;)
             {
                 // Wait for the current timer to expire.
                 // We will be woken up because either 1) the wait times out, which will indicate that
                 // the current timer has expired and/or 2) the TimerQueue installs a new (earlier) timer.
                 int startWait = TickCount;
-                bool timerHasExpired = !s_timerEvent.WaitOne(currentTimerInterval);
+                bool timerHasExpired = !_timerEvent.WaitOne(currentTimerInterval);
                 uint elapsedTime = (uint)(TickCount - startWait);
 
                 // The timer event can be set after this thread reads the new timer interval but before it enters
@@ -99,11 +99,11 @@ private void TimerThread()
 
                     // When FireNextTimers() installs a new timer, it also sets the timer event.
                     // Reset the event so the timer thread is not woken up right away unnecessary.
-                    s_timerEvent.Reset();
+                    _timerEvent.Reset();
                     currentTimerInterval = Timeout.Infinite;
                 }
 
-                int nextTimerInterval = Interlocked.Exchange(ref s_nextTimerDuration, Timeout.Infinite);
+                int nextTimerInterval = Interlocked.Exchange(ref _nextTimerDuration, Timeout.Infinite);
                 if (nextTimerInterval != Timeout.Infinite)
                 {
                     currentTimerInterval = nextTimerInterval;

tests/src/Simple/Threading/Threading.cs

@@ -38,6 +38,9 @@ public static int Main()
         //Console.WriteLine("    WaitSubsystemTests.MutexMaximumReacquireCountTest");
         //WaitSubsystemTests.MutexMaximumReacquireCountTest();
 
+        Console.WriteLine("    TimersCreatedConcurrentlyOnDifferentThreadsAllFire");
+        TimerTests.TimersCreatedConcurrentlyOnDifferentThreadsAllFire();
+
         Console.WriteLine("    ThreadPoolTests.RunProcessorCountItemsInParallel");
         ThreadPoolTests.RunProcessorCountItemsInParallel();
 
@@ -837,6 +840,62 @@ public static void MutexMaximumReacquireCountTest()
     }
 }
 
+internal static class TimerTests
+{
+    [Fact]
+    public static void TimersCreatedConcurrentlyOnDifferentThreadsAllFire()
+    {
+        int processorCount = Environment.ProcessorCount;
+
+        int timerTickCount = 0;
+        TimerCallback timerCallback = data => Interlocked.Increment(ref timerTickCount);
+
+        var threadStarted = new AutoResetEvent(false);
+        var createTimers = new ManualResetEvent(false);
+        var timers = new Timer[processorCount];
+        Action<object> createTimerThreadStart = data =>
+        {
+            int i = (int)data;
+            var sw = new Stopwatch();
+            threadStarted.Set();
+            createTimers.WaitOne();
+
+            // Use the CPU a bit around creating the timer to try to have some of these threads run concurrently
+            sw.Restart();
+            do
+            {
+                Thread.SpinWait(1000);
+            } while (sw.ElapsedMilliseconds < 10);
+
+            timers[i] = new Timer(timerCallback, null, 1, Timeout.Infinite);
+
+            // Use the CPU a bit around creating the timer to try to have some of these threads run concurrently
+            sw.Restart();
+            do
+            {
+                Thread.SpinWait(1000);
+            } while (sw.ElapsedMilliseconds < 10);
+        };
+
+        var waitsForThread = new Action[timers.Length];
+        for (int i = 0; i < timers.Length; ++i)
+        {
+            var t = ThreadTestHelpers.CreateGuardedThread(out waitsForThread[i], createTimerThreadStart);
+            t.IsBackground = true;
+            t.Start(i);
+            threadStarted.CheckedWait();
+        }
+
+        createTimers.Set();
+        ThreadTestHelpers.WaitForCondition(() => timerTickCount == timers.Length);
+
+        foreach (var waitForThread in waitsForThread)
+        {
+            waitForThread();
+        }
+    }
+}
+
 internal static class ThreadPoolTests
 {
     [Fact]
@@ -1353,6 +1412,70 @@ internal static class ThreadTestHelpers
     public const int ExpectedMeasurableTimeoutMilliseconds = 500;
     public const int UnexpectedTimeoutMilliseconds = 1000 * 30;
 
+    // Wait longer for a thread to time out, so that an unexpected timeout in the thread is more likely to expire first and
+    // provide a better stack trace for the failure
+    public const int UnexpectedThreadTimeoutMilliseconds = UnexpectedTimeoutMilliseconds * 2;
+
+    public static Thread CreateGuardedThread(out Action waitForThread, Action<object> start)
+    {
+        Action checkForThreadErrors;
+        return CreateGuardedThread(out checkForThreadErrors, out waitForThread, start);
+    }
+
+    public static Thread CreateGuardedThread(out Action checkForThreadErrors, out Action waitForThread, Action<object> start)
+    {
+        Exception backgroundEx = null;
+        var t =
+            new Thread(parameter =>
+            {
+                try
+                {
+                    start(parameter);
+                }
+                catch (Exception ex)
+                {
+                    backgroundEx = ex;
+                    Interlocked.MemoryBarrier();
+                }
+            });
+        Action localCheckForThreadErrors = checkForThreadErrors = // cannot use ref or out parameters in lambda
+            () =>
+            {
+                Interlocked.MemoryBarrier();
+                if (backgroundEx != null)
+                {
+                    throw new AggregateException(backgroundEx);
+                }
+            };
+        waitForThread =
+            () =>
+            {
+                Assert.True(t.Join(UnexpectedThreadTimeoutMilliseconds));
+                localCheckForThreadErrors();
+            };
+        return t;
+    }
+
+    public static void WaitForCondition(Func<bool> condition)
+    {
+        WaitForConditionWithCustomDelay(condition, () => Thread.Sleep(1));
+    }
+
+    public static void WaitForConditionWithCustomDelay(Func<bool> condition, Action delay)
+    {
+        if (condition())
+        {
+            return;
+        }
+
+        var startTime = Environment.TickCount;
+        do
+        {
+            delay();
+            Assert.True(Environment.TickCount - startTime < UnexpectedTimeoutMilliseconds);
+        } while (!condition());
+    }
+
     public static void CheckedWait(this WaitHandle wh)
     {
         Assert.True(wh.WaitOne(UnexpectedTimeoutMilliseconds));
@@ -1365,12 +1488,23 @@ internal sealed class InvalidWaitHandle : WaitHandle
 
 internal sealed class Stopwatch
 {
-    private DateTime _startTimeTicks;
-    private DateTime _endTimeTicks;
+    private int _startTimeMs;
+    private int _endTimeMs;
+    private bool _isStopped;
+
+    public void Restart()
+    {
+        _isStopped = false;
+        _startTimeMs = Environment.TickCount;
+    }
+
+    public void Stop()
+    {
+        _endTimeMs = Environment.TickCount;
+        _isStopped = true;
+    }
 
-    public void Restart() => _startTimeTicks = DateTime.UtcNow;
-    public void Stop() => _endTimeTicks = DateTime.UtcNow;
-    public long ElapsedMilliseconds => (long)(_endTimeTicks - _startTimeTicks).TotalMilliseconds;
+    public long ElapsedMilliseconds => (_isStopped ? _endTimeMs : Environment.TickCount) - _startTimeMs;
 }
 
 internal static class Assert