From 4acbd6e8137aed34592e420c12e98ce8f88152bb Mon Sep 17 00:00:00 2001 From: Brant Burnett Date: Mon, 26 Aug 2024 21:32:55 -0400 Subject: [PATCH 1/2] Check CancellationToken before TimeSpan.Zero in TimeProviderTaskExtensions This makes the down-level behavior for Delay and WaitAsync match the behavior in .NET 8 and later, prefering to return a Canceled task over a Completed task or TimeoutException when the TimeSpan is Zero and the CancellationToken is already Canceled. Fixes #106996 --- .../Common/tests/System/TimeProviderTests.cs | 21 ++++++++++++++++--- .../Tasks/TimeProviderTaskExtensions.cs | 16 +++++++------- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/libraries/Common/tests/System/TimeProviderTests.cs b/src/libraries/Common/tests/System/TimeProviderTests.cs index 126dfda8b233b..1a8301971f299 100644 --- a/src/libraries/Common/tests/System/TimeProviderTests.cs +++ b/src/libraries/Common/tests/System/TimeProviderTests.cs @@ -316,12 +316,26 @@ public static void RunDelayTests(TimeProvider provider, ITestTaskFactory taskFac Assert.True(task1.Status == TaskStatus.RanToCompletion, " > FAILED. Expected Delay(TimeSpan(0), timeProvider) to run to completion"); Assert.True(task2.Status == TaskStatus.RanToCompletion, " > FAILED. Expected Delay(TimeSpan(0), timeProvider, uncanceledToken) to run to completion"); + // This should complete quickly with a CANCELED status. + Task task3 = taskFactory.Delay(provider, new TimeSpan(0), new CancellationToken(true)); + try + { + var e = Assert.Throws(task3.Wait); + Assert.IsAssignableFrom(e.InnerException); + } + catch (Exception e) + { + Assert.Fail(string.Format("RunDelayTests: > FAILED. Unexpected exception on canceled task Wait(): {0}", e)); + } + + Assert.True(task3.Status == TaskStatus.Canceled, " > FAILED. Expected Delay(timeProvider, TimeSpan(0), canceledToken) to be canceled"); + // This should take some time - Task task3 = taskFactory.Delay(provider, TimeSpan.FromMilliseconds(20000)); + Task task4 = taskFactory.Delay(provider, TimeSpan.FromMilliseconds(20000)); - Assert.False(task3.IsCompleted, "RunDelayTests: > FAILED. Delay(20000) appears to have completed too soon(1)."); + Assert.False(task4.IsCompleted, "RunDelayTests: > FAILED. Delay(20000) appears to have completed too soon(1)."); Task t2 = Task.Delay(TimeSpan.FromMilliseconds(10)); - Assert.False(task3.IsCompleted, "RunDelayTests: > FAILED. Delay(10000) appears to have completed too soon(2)."); + Assert.False(task4.IsCompleted, "RunDelayTests: > FAILED. Delay(10000) appears to have completed too soon(2)."); } [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] @@ -359,6 +373,7 @@ public static async Task RunWaitAsyncTests(TimeProvider provider, ITestTaskFacto cts.Cancel(); await Assert.ThrowsAsync(() => taskFactory.WaitAsync(tcs5.Task, TimeSpan.FromMilliseconds(10), provider, cts.Token)); + await Assert.ThrowsAsync(() => taskFactory.WaitAsync(tcs5.Task, TimeSpan.Zero, provider, cts.Token)); using CancellationTokenSource cts1 = new CancellationTokenSource(); Task task5 = Task.Run(() => { while (!cts1.Token.IsCancellationRequested) { Thread.Sleep(10); } }); diff --git a/src/libraries/Microsoft.Bcl.TimeProvider/src/System/Threading/Tasks/TimeProviderTaskExtensions.cs b/src/libraries/Microsoft.Bcl.TimeProvider/src/System/Threading/Tasks/TimeProviderTaskExtensions.cs index 2153d859c00e8..a5ccccde11f31 100644 --- a/src/libraries/Microsoft.Bcl.TimeProvider/src/System/Threading/Tasks/TimeProviderTaskExtensions.cs +++ b/src/libraries/Microsoft.Bcl.TimeProvider/src/System/Threading/Tasks/TimeProviderTaskExtensions.cs @@ -66,14 +66,14 @@ public static Task Delay(this TimeProvider timeProvider, TimeSpan delay, Cancell throw new ArgumentOutOfRangeException(nameof(delay)); } - if (delay == TimeSpan.Zero) + if (cancellationToken.IsCancellationRequested) { - return Task.CompletedTask; + return Task.FromCanceled(cancellationToken); } - if (cancellationToken.IsCancellationRequested) + if (delay == TimeSpan.Zero) { - return Task.FromCanceled(cancellationToken); + return Task.CompletedTask; } DelayState state = new(cancellationToken); @@ -161,14 +161,14 @@ public static Task WaitAsync(this Task task, TimeSpan timeout, TimeProvider time return task; } - if (timeout == TimeSpan.Zero) + if (cancellationToken.IsCancellationRequested) { - return Task.FromException(new TimeoutException()); + return Task.FromCanceled(cancellationToken); } - if (cancellationToken.IsCancellationRequested) + if (timeout == TimeSpan.Zero) { - return Task.FromCanceled(cancellationToken); + return Task.FromException(new TimeoutException()); } WaitAsyncState state = new(cancellationToken); From da0c94795408754c54fd0e8c2daec15854ec763f Mon Sep 17 00:00:00 2001 From: Brant Burnett Date: Wed, 4 Sep 2024 16:48:30 -0400 Subject: [PATCH 2/2] Test improvements based on feedback --- .../Common/tests/System/TimeProviderTests.cs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/libraries/Common/tests/System/TimeProviderTests.cs b/src/libraries/Common/tests/System/TimeProviderTests.cs index 1a8301971f299..119a35610f1f5 100644 --- a/src/libraries/Common/tests/System/TimeProviderTests.cs +++ b/src/libraries/Common/tests/System/TimeProviderTests.cs @@ -310,7 +310,7 @@ public static void RunDelayTests(TimeProvider provider, ITestTaskFactory taskFac } catch (Exception e) { - Assert.Fail(string.Format("RunDelayTests: > FAILED. Unexpected exception on WaitAll(simple tasks): {0}", e)); + Assert.Fail($"RunDelayTests: > FAILED. Unexpected exception on WaitAll(simple tasks): {e}"); } Assert.True(task1.Status == TaskStatus.RanToCompletion, " > FAILED. Expected Delay(TimeSpan(0), timeProvider) to run to completion"); @@ -318,16 +318,9 @@ public static void RunDelayTests(TimeProvider provider, ITestTaskFactory taskFac // This should complete quickly with a CANCELED status. Task task3 = taskFactory.Delay(provider, new TimeSpan(0), new CancellationToken(true)); - try - { - var e = Assert.Throws(task3.Wait); - Assert.IsAssignableFrom(e.InnerException); - } - catch (Exception e) - { - Assert.Fail(string.Format("RunDelayTests: > FAILED. Unexpected exception on canceled task Wait(): {0}", e)); - } - + var canceledException = Record.Exception(task3.Wait); + Assert.True(canceledException is AggregateException { InnerException: OperationCanceledException }, + $"RunDelayTests: > FAILED. Unexpected exception on canceled task Wait(): {canceledException}"); Assert.True(task3.Status == TaskStatus.Canceled, " > FAILED. Expected Delay(timeProvider, TimeSpan(0), canceledToken) to be canceled"); // This should take some time