Fix regression in Timers with long timeouts (#25604)

* Fix regression in Timers with long timeouts

Early in .NET Core 3.0, we added an important optimization to significantly reduce lock contention and CPU overheads in situations where lots of timers were firing.  The optimization worked by splitting the set of timers into those expected to fire in the very near future and then everything else.  However, for really long timers (e.g. > 24 days), due to integer overflows we can accidentally put a long timer onto the short list and cause a timer that shouldn’t fire for days to instead fire in milliseconds.  This is not the first such bug we’ve had in Timer, and there is a fair amount of complicated casting (that we sometimes get wrong, obviously) in order to keep the tick count usage within the Int32 domain.  This PR addresses the problem (and hopefully others in the future) by switching over to using Int64.  We’re already paying to get 64-bit tick counts on both Windows and Unix, so this just removes truncation that was being performed, does a little more addition/comparison with 64-bit values instead of with 32-bit, and stores an Int64 instead of Int32 in each TimerQueueTimer.  On 64-bit, this has a 0 increase in memory consumption, as it simply ends up utilizing padding space that was previously in TimerQueueTimer.  On 32-bit, it increases memory allocation when creating a Timer by 4 bytes from 80 to 84 (still 3 allocations), but I believe it’s the right trade-off for reduced complexity and bug likelihood.

* Address PR feedback

diff --git a/src/System.Private.CoreLib/shared/System/Threading/Timer.cs b/src/System.Private.CoreLib/shared/System/Threading/Timer.cs
index 355fc0a..dd21c42 100644 (file)
--- a/src/System.Private.CoreLib/shared/System/Threading/Timer.cs
+++ b/src/System.Private.CoreLib/shared/System/Threading/Timer.cs
@@ -55,14 +55,13 @@ namespace System.Threading
         #region interface to native timer
 
         private bool _isTimerScheduled;
-        private int _currentTimerStartTicks;
+        private long _currentTimerStartTicks;
         private uint _currentTimerDuration;
 
         private bool EnsureTimerFiresBy(uint requestedDuration)
         {
             // The VM's timer implementation does not work well for very long-duration timers.
-            // See kb 950807.
-            // So we'll limit our native timer duration to a "small" value.
+            // So we limit our native timer duration to a "small" value.
             // This may cause us to attempt to fire timers early, but that's ok - 
             // we'll just see that none of our timers has actually reached its due time,
             // and schedule the native timer again.
@@ -71,11 +70,11 @@ namespace System.Threading
 
             if (_isTimerScheduled)
             {
-                uint elapsed = (uint)(TickCount - _currentTimerStartTicks);
+                long elapsed = TickCount64 - _currentTimerStartTicks;
                 if (elapsed >= _currentTimerDuration)
                     return true; //the timer's about to fire
 
-                uint remainingDuration = _currentTimerDuration - elapsed;
+                uint remainingDuration = _currentTimerDuration - (uint)elapsed;
                 if (actualDuration >= remainingDuration)
                     return true; //the timer will fire earlier than this request
             }
@@ -91,7 +90,7 @@ namespace System.Threading
             if (SetTimer(actualDuration))
             {
                 _isTimerScheduled = true;
-                _currentTimerStartTicks = TickCount;
+                _currentTimerStartTicks = TickCount64;
                 _currentTimerDuration = actualDuration;
                 return true;
             }
@@ -114,7 +113,7 @@ namespace System.Threading
 
         // The current threshold, an absolute time where any timers scheduled to go off at or
         // before this time must be queued to the short list.
-        private int _currentAbsoluteThreshold = ShortTimersThresholdMilliseconds;
+        private long _currentAbsoluteThreshold = TickCount64 + ShortTimersThresholdMilliseconds;
 
         // Default threshold that separates which timers target _shortTimers vs _longTimers. The threshold
         // is chosen to balance the number of timers in the small list against the frequency with which
@@ -144,7 +143,7 @@ namespace System.Threading
                 bool haveTimerToSchedule = false;
                 uint nextTimerDuration = uint.MaxValue;
 
-                int nowTicks = TickCount;
+                long nowTicks = TickCount64;
 
                 // Sweep through the "short" timers.  If the current tick count is greater than
                 // the current threshold, also sweep through the "long" timers.  Finally, as part
@@ -162,8 +161,8 @@ namespace System.Threading
                         // in our deleting or moving it; we'll continue after with this saved next timer.
                         TimerQueueTimer? next = timer._next;
 
-                        uint elapsed = (uint)(nowTicks - timer._startTicks);
-                        int remaining = (int)timer._dueTime - (int)elapsed;
+                        long elapsed = nowTicks - timer._startTicks;
+                        long remaining = timer._dueTime - elapsed;
                         if (remaining <= 0)
                         {
                             // Timer is ready to fire.
@@ -177,9 +176,9 @@ namespace System.Threading
                                 // again, meaning the timer can't keep up with the short period, have it fire 1 ms from now to
                                 // avoid spinning without a delay.
                                 timer._startTicks = nowTicks;
-                                uint elapsedForNextDueTime = elapsed - timer._dueTime;
+                                long elapsedForNextDueTime = elapsed - timer._dueTime;
                                 timer._dueTime = (elapsedForNextDueTime < timer._period) ?
-                                    timer._period - elapsedForNextDueTime :
+                                    timer._period - (uint)elapsedForNextDueTime :
                                     1;
 
                                 // Update the timer if this becomes the next timer to fire.
@@ -251,7 +250,7 @@ namespace System.Threading
                         // long list now; otherwise, most timers created in the interim would end up in the long
                         // list and we'd likely end up paying for another invocation of FireNextTimers that could
                         // have been delayed longer (to whatever is the current minimum in the long list).
-                        int remaining = _currentAbsoluteThreshold - nowTicks;
+                        long remaining = _currentAbsoluteThreshold - nowTicks;
                         if (remaining > 0)
                         {
                             if (_shortTimers == null && _longTimers != null)
@@ -294,11 +293,11 @@ namespace System.Threading
 
         public bool UpdateTimer(TimerQueueTimer timer, uint dueTime, uint period)
         {
-            int nowTicks = TickCount;
+            long nowTicks = TickCount64;
 
             // The timer can be put onto the short list if it's next absolute firing time
             // is <= the current absolute threshold.
-            int absoluteDueTime = (int)(nowTicks + dueTime);
+            long absoluteDueTime = nowTicks + dueTime;
             bool shouldBeShort = _currentAbsoluteThreshold - absoluteDueTime >= 0;
 
             if (timer._dueTime == Timeout.UnsignedInfinite)
@@ -414,7 +413,7 @@ namespace System.Threading
         internal bool _short;
 
         // The time, according to TimerQueue.TickCount, when this timer's current interval started.
-        internal int _startTicks;
+        internal long _startTicks;
 
         // Timeout.UnsignedInfinite if we are not going to fire.  Otherwise, the offset from _startTime when we will fire.
         internal uint _dueTime;

diff --git a/src/System.Private.CoreLib/shared/System/Threading/TimerQueue.Unix.cs b/src/System.Private.CoreLib/shared/System/Threading/TimerQueue.Unix.cs
index 7c45637..043b590 100644 (file)
--- a/src/System.Private.CoreLib/shared/System/Threading/TimerQueue.Unix.cs
+++ b/src/System.Private.CoreLib/shared/System/Threading/TimerQueue.Unix.cs
@@ -6,6 +6,6 @@ namespace System.Threading
 {
     internal partial class TimerQueue
     {
-        private static int TickCount => Environment.TickCount;
+        private static long TickCount64 => Environment.TickCount64;
     }
 }

diff --git a/src/System.Private.CoreLib/shared/System/Threading/TimerQueue.Windows.cs b/src/System.Private.CoreLib/shared/System/Threading/TimerQueue.Windows.cs
index 0bd0cc4..38a14a1 100644 (file)
--- a/src/System.Private.CoreLib/shared/System/Threading/TimerQueue.Windows.cs
+++ b/src/System.Private.CoreLib/shared/System/Threading/TimerQueue.Windows.cs
@@ -8,7 +8,7 @@ namespace System.Threading
 {
     internal partial class TimerQueue
     {
-        private static int TickCount
+        private static long TickCount64
         {
             get
             {
@@ -21,18 +21,14 @@ namespace System.Threading
                 // in sleep/hibernate mode.
                 if (Environment.IsWindows8OrAbove)
                 {
-                    ulong time100ns;
-
-                    bool result = Interop.Kernel32.QueryUnbiasedInterruptTime(out time100ns);
-                    if (!result)
+                    if (!Interop.Kernel32.QueryUnbiasedInterruptTime(out ulong time100ns))
                         Marshal.ThrowExceptionForHR(Marshal.GetLastWin32Error());
 
-                    // convert to 100ns to milliseconds, and truncate to 32 bits.
-                    return (int)(uint)(time100ns / 10000);
+                    return (long)(time100ns / 10_000); // convert from 100ns to milliseconds
                 }
                 else
                 {
-                    return Environment.TickCount;
+                    return Environment.TickCount64;
                 }
             }
         }