Address PR feedback

eduardo-vp · eduardo-vp · commit 20157b545d36 · 2025-07-30T20:56:06.000-07:00
diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
@@ -3411,7 +3411,7 @@
     <value>The timeout must represent a value between -1 and Int32.MaxValue, inclusive.</value>
   </data>
   <data name="SemaphoreSlim_Wait_TimeSpanTimeoutWrong" xml:space="preserve">
-    <value>The timeout must represent a value between -1 and the maximum allowed timer duration.</value>
+    <value>The timeout must be greater than or equal to -1.</value>
   </data>
   <data name="Serialization_BadParameterInfo" xml:space="preserve">
     <value>Non existent ParameterInfo. Position bigger than member's parameters length.</value>
diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ManualResetEventSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ManualResetEventSlim.cs
@@ -520,7 +520,7 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken)
                     // period of time.  The timeout adjustments only take effect when and if we actually
                     // decide to block in the kernel below.
 
-                    startTime = TimeoutHelper.GetTime();
+                    startTime = (uint)Environment.TickCount;
                     bNeedTimeoutAdjustment = true;
                 }
 
@@ -558,7 +558,8 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken)
                             // update timeout (delays in wait commencement are due to spinning and/or spurious wakeups from other waits being canceled)
                             if (bNeedTimeoutAdjustment)
                             {
-                                realMillisecondsTimeout = TimeoutHelper.UpdateTimeOut(startTime, millisecondsTimeout);
+                                // TimeoutHelper.UpdateTimeOut returns a long but the value is capped as millisecondsTimeout is an int.
+                                realMillisecondsTimeout = (int)TimeoutHelper.UpdateTimeOut(startTime, millisecondsTimeout);
                                 if (realMillisecondsTimeout <= 0)
                                     return false;
                             }
diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs
@@ -178,7 +178,7 @@ public void Wait()
             if (OperatingSystem.IsWasi()) throw new PlatformNotSupportedException(); // TODO remove with https://github.com/dotnet/runtime/pull/107185
 #endif
             // Call wait with infinite timeout
-            Wait(Timeout.UnsignedInfinite, CancellationToken.None);
+            Wait(Timeout.Infinite, CancellationToken.None);
         }
 
         /// <summary>
@@ -198,7 +198,7 @@ public void Wait(CancellationToken cancellationToken)
             if (OperatingSystem.IsWasi()) throw new PlatformNotSupportedException(); // TODO remove with https://github.com/dotnet/runtime/pull/107185
 #endif
             // Call wait with infinite timeout
-            Wait(Timeout.UnsignedInfinite, cancellationToken);
+            Wait(Timeout.Infinite, cancellationToken);
         }
 
         /// <summary>
@@ -211,8 +211,7 @@ public void Wait(CancellationToken cancellationToken)
         /// <returns>true if the current thread successfully entered the <see cref="SemaphoreSlim"/>;
         /// otherwise, false.</returns>
         /// <exception cref="ArgumentOutOfRangeException"><paramref name="timeout"/> is a negative
-        /// number other than -1 milliseconds, which represents an infinite time-out -or- timeout is greater
-        /// than maximum allowed timer duration.</exception>
+        /// number other than -1 milliseconds, which represents an infinite time-out.</exception>
         [UnsupportedOSPlatform("browser")]
         public bool Wait(TimeSpan timeout)
         {
@@ -221,14 +220,14 @@ public bool Wait(TimeSpan timeout)
 #endif
             // Validate the timeout
             long totalMilliseconds = (long)timeout.TotalMilliseconds;
-            if (totalMilliseconds < -1 || totalMilliseconds > Timer.MaxSupportedTimeout)
+            if (totalMilliseconds < -1)
             {
                 throw new ArgumentOutOfRangeException(
                     nameof(timeout), timeout, SR.SemaphoreSlim_Wait_TimeSpanTimeoutWrong);
             }
 
             // Call wait with the timeout milliseconds
-            return Wait(totalMilliseconds == Timeout.Infinite ? Timeout.UnsignedInfinite : (uint)totalMilliseconds, CancellationToken.None);
+            return Wait(totalMilliseconds, CancellationToken.None);
         }
 
         /// <summary>
@@ -244,8 +243,7 @@ public bool Wait(TimeSpan timeout)
         /// <returns>true if the current thread successfully entered the <see cref="SemaphoreSlim"/>;
         /// otherwise, false.</returns>
         /// <exception cref="ArgumentOutOfRangeException"><paramref name="timeout"/> is a negative
-        /// number other than -1 milliseconds, which represents an infinite time-out -or- timeout is greater
-        /// than maximum allowed timer duration.</exception>
+        /// number other than -1 milliseconds, which represents an infinite time-out.</exception>
         /// <exception cref="OperationCanceledException"><paramref name="cancellationToken"/> was canceled.</exception>
         [UnsupportedOSPlatform("browser")]
         public bool Wait(TimeSpan timeout, CancellationToken cancellationToken)
@@ -255,14 +253,14 @@ public bool Wait(TimeSpan timeout, CancellationToken cancellationToken)
 #endif
             // Validate the timeout
             long totalMilliseconds = (long)timeout.TotalMilliseconds;
-            if (totalMilliseconds < -1 || totalMilliseconds > Timer.MaxSupportedTimeout)
+            if (totalMilliseconds < -1)
             {
                 throw new ArgumentOutOfRangeException(
                     nameof(timeout), timeout, SR.SemaphoreSlim_Wait_TimeSpanTimeoutWrong);
             }
 
             // Call wait with the timeout milliseconds
-            return Wait(totalMilliseconds == Timeout.Infinite ? Timeout.UnsignedInfinite : (uint)totalMilliseconds, cancellationToken);
+            return Wait(totalMilliseconds, cancellationToken);
         }
 
         /// <summary>
@@ -309,7 +307,7 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken)
                     nameof(millisecondsTimeout), millisecondsTimeout, SR.SemaphoreSlim_Wait_TimeoutWrong);
             }
 
-            return Wait(millisecondsTimeout == Timeout.Infinite ? Timeout.UnsignedInfinite : (uint)millisecondsTimeout, cancellationToken);
+            return Wait(millisecondsTimeout, cancellationToken);
         }
 
         /// <summary>
@@ -323,7 +321,7 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken)
         /// <returns>true if the current thread successfully entered the <see cref="SemaphoreSlim"/>; otherwise, false.</returns>
         /// <exception cref="OperationCanceledException"><paramref name="cancellationToken"/> was canceled.</exception>
         [UnsupportedOSPlatform("browser")]
-        private bool Wait(uint millisecondsTimeout, CancellationToken cancellationToken)
+        private bool Wait(long millisecondsTimeout, CancellationToken cancellationToken)
         {
             CheckDispose();
 #if FEATURE_WASM_MANAGED_THREADS
@@ -339,9 +337,9 @@ private bool Wait(uint millisecondsTimeout, CancellationToken cancellationToken)
             }
 
             long startTime = 0;
-            if (millisecondsTimeout != Timeout.UnsignedInfinite && millisecondsTimeout > 0)
+            if (millisecondsTimeout != Timeout.Infinite && millisecondsTimeout > 0)
             {
-                startTime = TimeoutHelper.GetTime64();
+                startTime = Environment.TickCount64;
             }
 
             bool waitSuccessful = false;
@@ -465,7 +463,7 @@ private bool Wait(uint millisecondsTimeout, CancellationToken cancellationToken)
         /// <param name="cancellationToken">The CancellationToken to observe.</param>
         /// <returns>true if the monitor received a signal, false if the timeout expired</returns>
         [UnsupportedOSPlatform("browser")]
-        private bool WaitUntilCountOrTimeout(uint millisecondsTimeout, long startTime, CancellationToken cancellationToken)
+        private bool WaitUntilCountOrTimeout(long millisecondsTimeout, long startTime, CancellationToken cancellationToken)
         {
 #if TARGET_WASI
             if (OperatingSystem.IsWasi()) throw new PlatformNotSupportedException(); // TODO remove with https://github.com/dotnet/runtime/pull/107185
@@ -481,7 +479,7 @@ private bool WaitUntilCountOrTimeout(uint millisecondsTimeout, long startTime, C
                 // Since Monitor.Wait will handle the actual wait and it accepts an int timeout,
                 // we may need to cap the timeout to int.MaxValue.
                 bool timeoutIsCapped = false;
-                if (millisecondsTimeout != Timeout.UnsignedInfinite)
+                if (millisecondsTimeout != Timeout.Infinite)
                 {
                     long remainingWaitMilliseconds = TimeoutHelper.UpdateTimeOut(startTime, millisecondsTimeout);
                     if (remainingWaitMilliseconds <= 0)
@@ -531,7 +529,7 @@ private bool WaitUntilCountOrTimeout(uint millisecondsTimeout, long startTime, C
         /// <returns>A task that will complete when the semaphore has been entered.</returns>
         public Task WaitAsync()
         {
-            return WaitAsync(Timeout.UnsignedInfinite, default);
+            return WaitAsync(Timeout.Infinite, default);
         }
 
         /// <summary>
@@ -547,7 +545,7 @@ public Task WaitAsync()
         /// </exception>
         public Task WaitAsync(CancellationToken cancellationToken)
         {
-            return WaitAsync(Timeout.UnsignedInfinite, cancellationToken);
+            return WaitAsync(Timeout.Infinite, cancellationToken);
         }
 
         /// <summary>
@@ -619,14 +617,14 @@ public Task<bool> WaitAsync(TimeSpan timeout, CancellationToken cancellationToke
         {
             // Validate the timeout
             long totalMilliseconds = (long)timeout.TotalMilliseconds;
-            if (totalMilliseconds < -1 || totalMilliseconds > Timer.MaxSupportedTimeout)
+            if (totalMilliseconds < -1)
             {
                 throw new ArgumentOutOfRangeException(
                     nameof(timeout), timeout, SR.SemaphoreSlim_Wait_TimeSpanTimeoutWrong);
             }
 
             // Call wait with the timeout milliseconds
-            return WaitAsync(totalMilliseconds == Timeout.Infinite ? Timeout.UnsignedInfinite : (uint)totalMilliseconds, cancellationToken);
+            return WaitAsync(totalMilliseconds, cancellationToken);
         }
 
         /// <summary>
@@ -655,7 +653,7 @@ public Task<bool> WaitAsync(int millisecondsTimeout, CancellationToken cancellat
                     nameof(millisecondsTimeout), millisecondsTimeout, SR.SemaphoreSlim_Wait_TimeoutWrong);
             }
 
-            return WaitAsync(millisecondsTimeout == Timeout.Infinite ? Timeout.UnsignedInfinite : (uint)millisecondsTimeout, cancellationToken);
+            return WaitAsync(millisecondsTimeout, cancellationToken);
         }
 
         /// <summary>
@@ -673,7 +671,7 @@ public Task<bool> WaitAsync(int millisecondsTimeout, CancellationToken cancellat
         /// </returns>
         /// <exception cref="ObjectDisposedException">The current instance has already been
         /// disposed.</exception>
-        private Task<bool> WaitAsync(uint millisecondsTimeout, CancellationToken cancellationToken)
+        private Task<bool> WaitAsync(long millisecondsTimeout, CancellationToken cancellationToken)
         {
             CheckDispose();
 
@@ -702,7 +700,7 @@ private Task<bool> WaitAsync(uint millisecondsTimeout, CancellationToken cancell
                 {
                     Debug.Assert(m_currentCount == 0, "m_currentCount should never be negative");
                     TaskNode asyncWaiter = CreateAndAddAsyncWaiter();
-                    return (millisecondsTimeout == Timeout.UnsignedInfinite && !cancellationToken.CanBeCanceled) ?
+                    return (millisecondsTimeout == Timeout.Infinite && !cancellationToken.CanBeCanceled) ?
                         asyncWaiter :
                         WaitUntilCountOrTimeoutAsync(asyncWaiter, millisecondsTimeout, cancellationToken);
                 }
@@ -767,13 +765,13 @@ private bool RemoveAsyncWaiter(TaskNode task)
         /// <param name="millisecondsTimeout">The timeout.</param>
         /// <param name="cancellationToken">The cancellation token.</param>
         /// <returns>The task to return to the caller.</returns>
-        private async Task<bool> WaitUntilCountOrTimeoutAsync(TaskNode asyncWaiter, uint millisecondsTimeout, CancellationToken cancellationToken)
+        private async Task<bool> WaitUntilCountOrTimeoutAsync(TaskNode asyncWaiter, long millisecondsTimeout, CancellationToken cancellationToken)
         {
             Debug.Assert(asyncWaiter is not null, "Waiter should have been constructed");
             Debug.Assert(Monitor.IsEntered(m_lockObjAndDisposed), "Requires the lock be held");
 
             await ((Task)asyncWaiter.WaitAsync(
-                TimeSpan.FromMilliseconds(millisecondsTimeout == Timeout.UnsignedInfinite ? (long)Timeout.Infinite : (long)millisecondsTimeout),
+                TimeSpan.FromMilliseconds(millisecondsTimeout),
                 cancellationToken)).ConfigureAwait(ConfigureAwaitOptions.SuppressThrowing);
 
             if (cancellationToken.IsCancellationRequested)
diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SpinLock.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SpinLock.cs
@@ -290,7 +290,7 @@ private void ContinueTryEnter(int millisecondsTimeout, ref bool lockTaken)
             uint startTime = 0;
             if (millisecondsTimeout != Timeout.Infinite && millisecondsTimeout != 0)
             {
-                startTime = TimeoutHelper.GetTime();
+                startTime = (uint)Environment.TickCount;
             }
 
             if (IsThreadOwnerTrackingEnabled)
diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/SpinWait.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/SpinWait.cs
@@ -315,7 +315,7 @@ public static bool SpinUntil(Func<bool> condition, int millisecondsTimeout)
             uint startTime = 0;
             if (millisecondsTimeout != 0 && millisecondsTimeout != Timeout.Infinite)
             {
-                startTime = TimeoutHelper.GetTime();
+                startTime = (uint)Environment.TickCount;
             }
             SpinWait spinner = default;
             while (!condition())
@@ -329,7 +329,7 @@ public static bool SpinUntil(Func<bool> condition, int millisecondsTimeout)
 
                 if (millisecondsTimeout != Timeout.Infinite && spinner.NextSpinWillYield)
                 {
-                    if (millisecondsTimeout <= (TimeoutHelper.GetTime() - startTime))
+                    if (millisecondsTimeout <= (uint)Environment.TickCount - startTime)
                     {
                         return false;
                     }
diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/TimeoutHelper.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/TimeoutHelper.cs
@@ -6,77 +6,35 @@
 namespace System.Threading
 {
     /// <summary>
-    /// A helper class to capture a start time using <see cref="Environment.TickCount"/> as a time in milliseconds.
-    /// Also updates a given timeout by subtracting the current time from the start time.
+    /// A helper class to update a given timeout by subtracting the current time from the start time.
     /// </summary>
     internal static class TimeoutHelper
     {
-        /// <summary>
-        /// Returns <see cref="Environment.TickCount"/> as a start time in milliseconds as a <see cref="uint"/>.
-        /// <see cref="Environment.TickCount"/> rolls over from positive to negative every ~25 days, then ~25 days to back to positive again.
-        /// <see cref="uint"/> is used to ignore the sign and double the range to 50 days.
-        /// </summary>
-        public static uint GetTime()
-        {
-            return (uint)Environment.TickCount;
-        }
-
-        /// <summary>
-        /// Returns <see cref="Environment.TickCount64"/> as a start time in milliseconds.
-        /// </summary>
-        public static long GetTime64()
-        {
-            return Environment.TickCount64;
-        }
-
         /// <summary>
         /// Helper function to measure and update the elapsed time
         /// </summary>
         /// <param name="startTime"> The first time (in milliseconds) observed when the wait started</param>
         /// <param name="originalWaitMillisecondsTimeout">The original wait timeout in milliseconds</param>
         /// <returns>The new wait time in milliseconds</returns>
-        public static int UpdateTimeOut(uint startTime, int originalWaitMillisecondsTimeout)
+        public static long UpdateTimeOut(long startTime, long originalWaitMillisecondsTimeout)
         {
             // The function must be called in case the time out is not infinite
             Debug.Assert(originalWaitMillisecondsTimeout != Timeout.Infinite);
 
-            uint elapsedMilliseconds = (GetTime() - startTime);
+            ulong elapsedMilliseconds = (ulong)(Environment.TickCount64 - startTime);
 
-            // Check the elapsed milliseconds is greater than max int because this property is uint
-            if (elapsedMilliseconds > int.MaxValue)
+            if (elapsedMilliseconds > long.MaxValue)
             {
                 return 0;
             }
 
-            // Subtract the elapsed time from the current wait time
-            int currentWaitTimeout = originalWaitMillisecondsTimeout - (int)elapsedMilliseconds;
+            long currentWaitTimeout = originalWaitMillisecondsTimeout - (long)elapsedMilliseconds;
             if (currentWaitTimeout <= 0)
             {
                 return 0;
             }
 
             return currentWaitTimeout;
         }
-
-        /// <summary>
-        /// Helper function to measure and update the elapsed time
-        /// </summary>
-        /// <param name="startTime"> The first time (in milliseconds) observed when the wait started</param>
-        /// <param name="originalWaitMillisecondsTimeout">The original wait timeout in milliseconds</param>
-        /// <returns>The new wait time in milliseconds</returns>
-        public static long UpdateTimeOut(long startTime, uint originalWaitMillisecondsTimeout)
-        {
-            // The function must be called in case the time out is not infinite
-            Debug.Assert(originalWaitMillisecondsTimeout != Timeout.UnsignedInfinite);
-
-            long elapsedMilliseconds = GetTime64() - startTime;
-
-            if (originalWaitMillisecondsTimeout <= elapsedMilliseconds)
-            {
-                return 0;
-            }
-
-            return originalWaitMillisecondsTimeout - elapsedMilliseconds;
-        }
     }
 }
diff --git a/src/libraries/System.Threading/tests/SemaphoreSlimTests.cs b/src/libraries/System.Threading/tests/SemaphoreSlimTests.cs

Original file line number	Diff line number	Diff line change
`@@ -520,7 +520,7 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken)`
`520`	`520`	`// period of time. The timeout adjustments only take effect when and if we actually`
`521`	`521`	`// decide to block in the kernel below.`
`522`	`522`
`523`		`- startTime = TimeoutHelper.GetTime();`
	`523`	`+ startTime = (uint)Environment.TickCount;`
`524`	`524`	`bNeedTimeoutAdjustment = true;`
`525`	`525`	`}`
`526`	`526`
`@@ -558,7 +558,8 @@ public bool Wait(int millisecondsTimeout, CancellationToken cancellationToken)`
`558`	`558`	`// update timeout (delays in wait commencement are due to spinning and/or spurious wakeups from other waits being canceled)`
`559`	`559`	`if (bNeedTimeoutAdjustment)`
`560`	`560`	`{`
`561`		`- realMillisecondsTimeout = TimeoutHelper.UpdateTimeOut(startTime, millisecondsTimeout);`
	`561`	`+ // TimeoutHelper.UpdateTimeOut returns a long but the value is capped as millisecondsTimeout is an int.`
	`562`	`+ realMillisecondsTimeout = (int)TimeoutHelper.UpdateTimeOut(startTime, millisecondsTimeout);`
`562`	`563`	`if (realMillisecondsTimeout <= 0)`
`563`	`564`	`return false;`
`564`	`565`	`}`
Original file line number	Diff line number	Diff line change
`@@ -290,7 +290,7 @@ private void ContinueTryEnter(int millisecondsTimeout, ref bool lockTaken)`
`290`	`290`	`uint startTime = 0;`
`291`	`291`	`if (millisecondsTimeout != Timeout.Infinite && millisecondsTimeout != 0)`
`292`	`292`	`{`
`293`		`- startTime = TimeoutHelper.GetTime();`
	`293`	`+ startTime = (uint)Environment.TickCount;`
`294`	`294`	`}`
`295`	`295`
`296`	`296`	`if (IsThreadOwnerTrackingEnabled)`
Original file line number	Diff line number	Diff line change
`@@ -315,7 +315,7 @@ public static bool SpinUntil(Func<bool> condition, int millisecondsTimeout)`
`315`	`315`	`uint startTime = 0;`
`316`	`316`	`if (millisecondsTimeout != 0 && millisecondsTimeout != Timeout.Infinite)`
`317`	`317`	`{`
`318`		`- startTime = TimeoutHelper.GetTime();`
	`318`	`+ startTime = (uint)Environment.TickCount;`
`319`	`319`	`}`
`320`	`320`	`SpinWait spinner = default;`
`321`	`321`	`while (!condition())`
`@@ -329,7 +329,7 @@ public static bool SpinUntil(Func<bool> condition, int millisecondsTimeout)`
`329`	`329`
`330`	`330`	`if (millisecondsTimeout != Timeout.Infinite && spinner.NextSpinWillYield)`
`331`	`331`	`{`
`332`		`- if (millisecondsTimeout <= (TimeoutHelper.GetTime() - startTime))`
	`332`	`+ if (millisecondsTimeout <= (uint)Environment.TickCount - startTime)`
`333`	`333`	`{`
`334`	`334`	`return false;`
`335`	`335`	`}`