From 0e959bef48363f762ec7189db9b317d0a6d029b5 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 21 Oct 2020 22:13:12 -0400 Subject: [PATCH] Extend allowed Task.Delay/CTS TimeSpan values to match Timer (#43708) For some reason, Task.Delay(TimeSpan, ...) and CancellationTokenSource.CancelAfter(TimeSpan) cut off the max allowed timeout at int.MaxValue milliseconds, whereas Timer's TimeSpan support (which is used under the covers) goes all the way to UInt32.MaxValue - 2. This changes Task/CancellationTokenSource to match, effectively doubling the allowed length of the timeouts. --- .../src/Resources/Strings.resx | 2 +- .../System/Threading/CancellationTokenSource.cs | 34 ++++++++++--------- .../src/System/Threading/Tasks/Task.cs | 38 ++++++++++------------ .../src/System/Threading/Timer.cs | 14 ++++---- .../tests/CancellationTokenTests.cs | 35 ++++++++++---------- .../tests/Task/TaskRtTests.cs | 23 +++++++++++++ 6 files changed, 85 insertions(+), 61 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx index 0fd5380..79a510f 100644 --- a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx +++ b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx @@ -3359,7 +3359,7 @@ The specified TaskContinuationOptions excluded all continuation kinds. - The value needs to translate in milliseconds to -1 (signifying an infinite timeout), 0 or a positive integer less than or equal to Int32.MaxValue. + The value needs to translate in milliseconds to -1 (signifying an infinite timeout), 0 or a positive integer less than or equal to the maximum allowed timer duration. The value needs to be either -1 (signifying an infinite timeout), 0 or a positive integer. diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs index c83625f..9fb58d4 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs @@ -151,7 +151,7 @@ namespace System.Threading /// /// The time span to wait before canceling this /// - /// The exception that is thrown when is less than -1 or greater than int.MaxValue. + /// The is less than -1 or greater than the maximum allowed timer duration. /// /// /// @@ -168,12 +168,12 @@ namespace System.Threading public CancellationTokenSource(TimeSpan delay) { long totalMilliseconds = (long)delay.TotalMilliseconds; - if (totalMilliseconds < -1 || totalMilliseconds > int.MaxValue) + if (totalMilliseconds < -1 || totalMilliseconds > Timer.MaxSupportedTimeout) { throw new ArgumentOutOfRangeException(nameof(delay)); } - InitializeWithTimer((int)totalMilliseconds); + InitializeWithTimer((uint)totalMilliseconds); } /// @@ -202,14 +202,14 @@ namespace System.Threading throw new ArgumentOutOfRangeException(nameof(millisecondsDelay)); } - InitializeWithTimer(millisecondsDelay); + InitializeWithTimer((uint)millisecondsDelay); } /// /// Common initialization logic when constructing a CTS with a delay parameter. /// A zero delay will result in immediate cancellation. /// - private void InitializeWithTimer(int millisecondsDelay) + private void InitializeWithTimer(uint millisecondsDelay) { if (millisecondsDelay == 0) { @@ -218,7 +218,7 @@ namespace System.Threading else { _state = NotCanceledState; - _timer = new TimerQueueTimer(s_timerCallback, this, (uint)millisecondsDelay, Timeout.UnsignedInfinite, flowExecutionContext: false); + _timer = new TimerQueueTimer(s_timerCallback, this, millisecondsDelay, Timeout.UnsignedInfinite, flowExecutionContext: false); // The timer roots this CTS instance while it's scheduled. That is by design, so // that code like: @@ -286,8 +286,7 @@ namespace System.Threading /// cref="CancellationTokenSource"/> has been disposed. /// /// - /// The exception thrown when is less than -1 or - /// greater than int.MaxValue. + /// The is less than -1 or greater than maximum allowed timer duration. /// /// /// @@ -303,12 +302,12 @@ namespace System.Threading public void CancelAfter(TimeSpan delay) { long totalMilliseconds = (long)delay.TotalMilliseconds; - if (totalMilliseconds < -1 || totalMilliseconds > int.MaxValue) + if (totalMilliseconds < -1 || totalMilliseconds > Timer.MaxSupportedTimeout) { - throw new ArgumentOutOfRangeException(nameof(delay)); + ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.delay); } - CancelAfter((int)totalMilliseconds); + CancelAfter((uint)totalMilliseconds); } /// @@ -337,13 +336,18 @@ namespace System.Threading /// public void CancelAfter(int millisecondsDelay) { - ThrowIfDisposed(); - if (millisecondsDelay < -1) { - throw new ArgumentOutOfRangeException(nameof(millisecondsDelay)); + ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.millisecondsDelay); } + CancelAfter((uint)millisecondsDelay); + } + + private void CancelAfter(uint millisecondsDelay) + { + ThrowIfDisposed(); + if (IsCancellationRequested) { return; @@ -379,7 +383,7 @@ namespace System.Threading // the following in a try/catch block. try { - timer.Change((uint)millisecondsDelay, Timeout.UnsignedInfinite); + timer.Change(millisecondsDelay, Timeout.UnsignedInfinite); } catch (ObjectDisposedException) { diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs index c6cbb14..b0b1db2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs @@ -5278,15 +5278,12 @@ namespace System.Threading.Tasks /// The time span to wait before completing the returned Task /// A Task that represents the time delay /// - /// The is less than -1 or greater than int.MaxValue. + /// The is less than -1 or greater than the maximum allowed timer duration. /// /// /// After the specified time delay, the Task is completed in RanToCompletion state. /// - public static Task Delay(TimeSpan delay) - { - return Delay(delay, default); - } + public static Task Delay(TimeSpan delay) => Delay(delay, default); /// /// Creates a Task that will complete after a time delay. @@ -5295,7 +5292,7 @@ namespace System.Threading.Tasks /// The cancellation token that will be checked prior to completing the returned Task /// A Task that represents the time delay /// - /// The is less than -1 or greater than int.MaxValue. + /// The is less than -1 or greater than the maximum allowed timer duration. /// /// /// The provided has already been disposed. @@ -5308,12 +5305,12 @@ namespace System.Threading.Tasks public static Task Delay(TimeSpan delay, CancellationToken cancellationToken) { long totalMilliseconds = (long)delay.TotalMilliseconds; - if (totalMilliseconds < -1 || totalMilliseconds > int.MaxValue) + if (totalMilliseconds < -1 || totalMilliseconds > Timer.MaxSupportedTimeout) { ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.delay, ExceptionResource.Task_Delay_InvalidDelay); } - return Delay((int)totalMilliseconds, cancellationToken); + return Delay((uint)totalMilliseconds, cancellationToken); } /// @@ -5327,10 +5324,7 @@ namespace System.Threading.Tasks /// /// After the specified time delay, the Task is completed in RanToCompletion state. /// - public static Task Delay(int millisecondsDelay) - { - return Delay(millisecondsDelay, default); - } + public static Task Delay(int millisecondsDelay) => Delay(millisecondsDelay, default); /// /// Creates a Task that will complete after a time delay. @@ -5357,19 +5351,21 @@ namespace System.Threading.Tasks ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.millisecondsDelay, ExceptionResource.Task_Delay_InvalidMillisecondsDelay); } - return - cancellationToken.IsCancellationRequested ? FromCanceled(cancellationToken) : - millisecondsDelay == 0 ? CompletedTask : - cancellationToken.CanBeCanceled ? new DelayPromiseWithCancellation(millisecondsDelay, cancellationToken) : - new DelayPromise(millisecondsDelay); + return Delay((uint)millisecondsDelay, cancellationToken); } + private static Task Delay(uint millisecondsDelay, CancellationToken cancellationToken) => + cancellationToken.IsCancellationRequested ? FromCanceled(cancellationToken) : + millisecondsDelay == 0 ? CompletedTask : + cancellationToken.CanBeCanceled ? new DelayPromiseWithCancellation(millisecondsDelay, cancellationToken) : + new DelayPromise(millisecondsDelay); + /// Task that also stores the completion closure and logic for Task.Delay implementation. private class DelayPromise : Task { private readonly TimerQueueTimer? _timer; - internal DelayPromise(int millisecondsDelay) + internal DelayPromise(uint millisecondsDelay) { Debug.Assert(millisecondsDelay != 0); @@ -5379,9 +5375,9 @@ namespace System.Threading.Tasks if (s_asyncDebuggingEnabled) AddToActiveTasks(this); - if (millisecondsDelay != Timeout.Infinite) // no need to create the timer if it's an infinite timeout + if (millisecondsDelay != Timeout.UnsignedInfinite) // no need to create the timer if it's an infinite timeout { - _timer = new TimerQueueTimer(state => ((DelayPromise)state!).CompleteTimedOut(), this, (uint)millisecondsDelay, Timeout.UnsignedInfinite, flowExecutionContext: false); + _timer = new TimerQueueTimer(state => ((DelayPromise)state!).CompleteTimedOut(), this, millisecondsDelay, Timeout.UnsignedInfinite, flowExecutionContext: false); if (IsCompleted) { // Handle rare race condition where completion occurs prior to our having created and stored the timer, in which case @@ -5414,7 +5410,7 @@ namespace System.Threading.Tasks { private readonly CancellationTokenRegistration _registration; - internal DelayPromiseWithCancellation(int millisecondsDelay, CancellationToken token) : base(millisecondsDelay) + internal DelayPromiseWithCancellation(uint millisecondsDelay, CancellationToken token) : base(millisecondsDelay) { Debug.Assert(token.CanBeCanceled); diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Timer.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Timer.cs index 6486053..6f79993 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/Timer.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Timer.cs @@ -693,7 +693,7 @@ namespace System.Threading public sealed class Timer : MarshalByRefObject, IDisposable, IAsyncDisposable { - private const uint MAX_SUPPORTED_TIMEOUT = (uint)0xfffffffe; + internal const uint MaxSupportedTimeout = 0xfffffffe; private TimerHolder _timer; @@ -727,13 +727,13 @@ namespace System.Threading long dueTm = (long)dueTime.TotalMilliseconds; if (dueTm < -1) throw new ArgumentOutOfRangeException(nameof(dueTime), SR.ArgumentOutOfRange_NeedNonNegOrNegative1); - if (dueTm > MAX_SUPPORTED_TIMEOUT) + if (dueTm > MaxSupportedTimeout) throw new ArgumentOutOfRangeException(nameof(dueTime), SR.ArgumentOutOfRange_TimeoutTooLarge); long periodTm = (long)period.TotalMilliseconds; if (periodTm < -1) throw new ArgumentOutOfRangeException(nameof(period), SR.ArgumentOutOfRange_NeedNonNegOrNegative1); - if (periodTm > MAX_SUPPORTED_TIMEOUT) + if (periodTm > MaxSupportedTimeout) throw new ArgumentOutOfRangeException(nameof(period), SR.ArgumentOutOfRange_PeriodTooLarge); TimerSetup(callback, state, (uint)dueTm, (uint)periodTm); @@ -757,9 +757,9 @@ namespace System.Threading throw new ArgumentOutOfRangeException(nameof(dueTime), SR.ArgumentOutOfRange_NeedNonNegOrNegative1); if (period < -1) throw new ArgumentOutOfRangeException(nameof(period), SR.ArgumentOutOfRange_NeedNonNegOrNegative1); - if (dueTime > MAX_SUPPORTED_TIMEOUT) + if (dueTime > MaxSupportedTimeout) throw new ArgumentOutOfRangeException(nameof(dueTime), SR.ArgumentOutOfRange_TimeoutTooLarge); - if (period > MAX_SUPPORTED_TIMEOUT) + if (period > MaxSupportedTimeout) throw new ArgumentOutOfRangeException(nameof(period), SR.ArgumentOutOfRange_PeriodTooLarge); TimerSetup(callback, state, (uint)dueTime, (uint)period); } @@ -814,9 +814,9 @@ namespace System.Threading throw new ArgumentOutOfRangeException(nameof(dueTime), SR.ArgumentOutOfRange_NeedNonNegOrNegative1); if (period < -1) throw new ArgumentOutOfRangeException(nameof(period), SR.ArgumentOutOfRange_NeedNonNegOrNegative1); - if (dueTime > MAX_SUPPORTED_TIMEOUT) + if (dueTime > MaxSupportedTimeout) throw new ArgumentOutOfRangeException(nameof(dueTime), SR.ArgumentOutOfRange_TimeoutTooLarge); - if (period > MAX_SUPPORTED_TIMEOUT) + if (period > MaxSupportedTimeout) throw new ArgumentOutOfRangeException(nameof(period), SR.ArgumentOutOfRange_PeriodTooLarge); return _timer._timer.Change((uint)dueTime, (uint)period); diff --git a/src/libraries/System.Threading.Tasks/tests/CancellationTokenTests.cs b/src/libraries/System.Threading.Tasks/tests/CancellationTokenTests.cs index 26c9ae4..8d4029c 100644 --- a/src/libraries/System.Threading.Tasks/tests/CancellationTokenTests.cs +++ b/src/libraries/System.Threading.Tasks/tests/CancellationTokenTests.cs @@ -1052,27 +1052,28 @@ namespace System.Threading.Tasks.Tests [Fact] public static void CancellationTokenSourceWithTimer_Negative() { - TimeSpan bigTimeSpan = new TimeSpan(2000, 0, 0, 0, 0); + TimeSpan bigTimeSpan = TimeSpan.FromMilliseconds(uint.MaxValue); TimeSpan reasonableTimeSpan = new TimeSpan(0, 0, 1); - // - // Test exception logic - // - Assert.Throws( - () => { new CancellationTokenSource(-2); }); - Assert.Throws( - () => { new CancellationTokenSource(bigTimeSpan); }); - CancellationTokenSource cts = new CancellationTokenSource(); - Assert.Throws( - () => { cts.CancelAfter(-2); }); - Assert.Throws( - () => { cts.CancelAfter(bigTimeSpan); }); + Assert.Throws(() => { new CancellationTokenSource(-2); }); + Assert.Throws(() => { new CancellationTokenSource(bigTimeSpan); }); + var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(uint.MaxValue - 1)); + Assert.False(cts.IsCancellationRequested); cts.Dispose(); - Assert.Throws( - () => { cts.CancelAfter(1); }); - Assert.Throws( - () => { cts.CancelAfter(reasonableTimeSpan); }); + + cts = new CancellationTokenSource(); + Assert.Throws(() => { cts.CancelAfter(-2); }); + Assert.Throws(() => { cts.CancelAfter(bigTimeSpan); }); + + cts = new CancellationTokenSource(); + cts.CancelAfter(TimeSpan.FromMilliseconds(uint.MaxValue - 1)); + Assert.False(cts.IsCancellationRequested); + cts.Dispose(); + + cts.Dispose(); + Assert.Throws(() => { cts.CancelAfter(1); }); + Assert.Throws(() => { cts.CancelAfter(reasonableTimeSpan); }); } [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] diff --git a/src/libraries/System.Threading.Tasks/tests/Task/TaskRtTests.cs b/src/libraries/System.Threading.Tasks/tests/Task/TaskRtTests.cs index 5ca7e5a..b86df9c 100644 --- a/src/libraries/System.Threading.Tasks/tests/Task/TaskRtTests.cs +++ b/src/libraries/System.Threading.Tasks/tests/Task/TaskRtTests.cs @@ -495,6 +495,29 @@ namespace System.Threading.Tasks.Tests Assert.True((bool)isHandledField.GetValue(holderObject), "Expected FromException task to be observed after accessing Exception"); } + [Theory] + [InlineData(-2L)] + [InlineData((long)int.MinValue)] + [InlineData((long)uint.MaxValue)] + public static void TaskDelay_OutOfBounds_ThrowsException(long delay) + { + AssertExtensions.Throws("delay", () => { Task.Delay(TimeSpan.FromMilliseconds(delay)); }); + if (delay >= int.MinValue && delay <= int.MaxValue) + { + AssertExtensions.Throws("millisecondsDelay", () => { Task.Delay((int)delay); }); + } + } + + [Fact] + public static void TaskDelay_MaxSupported_Success() + { + var cts = new CancellationTokenSource(); + Task t = Task.Delay(TimeSpan.FromMilliseconds(uint.MaxValue - 2), cts.Token); + Assert.False(t.IsCompleted); + cts.Cancel(); + Assert.True(t.IsCanceled); + } + [Fact] public static void RunDelayTests() { -- 2.7.4