From ab786b99ad3a703917ab06b0e3e55e5d1353f40f Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Sun, 13 Aug 2023 09:54:26 -0700 Subject: [PATCH 1/8] Make TimeProvider Delay Continuations Synchronously --- .../Common/tests/System/TimeProviderTests.cs | 30 +++++++++++++++++++ .../Tasks/TimeProviderTaskExtensions.cs | 2 +- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/libraries/Common/tests/System/TimeProviderTests.cs b/src/libraries/Common/tests/System/TimeProviderTests.cs index a4209e83f5236c..a8dcc764eed422 100644 --- a/src/libraries/Common/tests/System/TimeProviderTests.cs +++ b/src/libraries/Common/tests/System/TimeProviderTests.cs @@ -638,6 +638,36 @@ public override ITimer CreateTimer(TimerCallback callback, object? state, TimeSp } } + public static IEnumerable TaskFactoryData() + { + yield return new object[] { taskFactory }; + +#if TESTEXTENSIONS + yield return new object[] { extensionsTaskFactory }; +#endif // TESTEXTENSIONS + } + + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] + [MemberData(nameof(TaskFactoryData))] + public void TestDelayTaskContinuation(ITestTaskFactory taskFactory) + { + ManualTimeProvider manualTimeProvider = new ManualTimeProvider(); + var callbackCount = 0; + _ = Continuation(manualTimeProvider, () => callbackCount++); + + Assert.NotNull(manualTimeProvider.Timer); + manualTimeProvider.Timer.Fire(); + + // Delay should be completed and the continuation should be called. + Assert.Equal(1, callbackCount); + + async Task Continuation(TimeProvider timeProvider, Action callback) + { + await taskFactory.Delay(timeProvider, TimeSpan.FromSeconds(10)); + callback(); + } + } + [Fact] // 1- Creates the CTS with a delay that we control via the time provider. // 2- Disposes the CTS. 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 e410f4a5c9d32b..55f77f88408166 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 @@ -15,7 +15,7 @@ public static class TimeProviderTaskExtensions #if !NET8_0_OR_GREATER private sealed class DelayState : TaskCompletionSource { - public DelayState(CancellationToken cancellationToken) : base(TaskCreationOptions.RunContinuationsAsynchronously) + public DelayState(CancellationToken cancellationToken) : base() { CancellationToken = cancellationToken; } From cd1931a6191ccf0d4c93a7fa52344e035d71036e Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Sun, 13 Aug 2023 19:31:11 -0700 Subject: [PATCH 2/8] Address the feedback. Thanks to Stephen Toub for help with it :-) --- .../Common/tests/System/TimeProviderTests.cs | 29 +++++++++++++++---- .../Tasks/TimeProviderTaskExtensions.cs | 9 +++++- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/src/libraries/Common/tests/System/TimeProviderTests.cs b/src/libraries/Common/tests/System/TimeProviderTests.cs index a8dcc764eed422..f353c9deee9bfa 100644 --- a/src/libraries/Common/tests/System/TimeProviderTests.cs +++ b/src/libraries/Common/tests/System/TimeProviderTests.cs @@ -651,19 +651,36 @@ public static IEnumerable TaskFactoryData() [MemberData(nameof(TaskFactoryData))] public void TestDelayTaskContinuation(ITestTaskFactory taskFactory) { + // + // Test time expiration and validate continuation is called synchronously. + // + ManualTimeProvider manualTimeProvider = new ManualTimeProvider(); var callbackCount = 0; - _ = Continuation(manualTimeProvider, () => callbackCount++); - Assert.NotNull(manualTimeProvider.Timer); - manualTimeProvider.Timer.Fire(); + _ = Continuation(manualTimeProvider, default, () => callbackCount++); - // Delay should be completed and the continuation should be called. + Assert.NotNull(manualTimeProvider.Timer); + manualTimeProvider.Timer.Fire(); + + // Delay should be completed and the continuation should be called synchronously. Assert.Equal(1, callbackCount); - async Task Continuation(TimeProvider timeProvider, Action callback) + // + // Test cancellation and validate continuation is called asynchronously. + // + + manualTimeProvider = new ManualTimeProvider(); + CancellationTokenSource cts = new CancellationTokenSource(); + + Task task = Continuation(manualTimeProvider, cts.Token, () => callbackCount++); + cts.Cancel(); + + Assert.Throws(() => task.GetAwaiter().GetResult()); + + async Task Continuation(TimeProvider timeProvider, CancellationToken token, Action callback) { - await taskFactory.Delay(timeProvider, TimeSpan.FromSeconds(10)); + await taskFactory.Delay(timeProvider, TimeSpan.FromSeconds(10), token); callback(); } } 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 55f77f88408166..a8595455445415 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 @@ -89,7 +89,14 @@ public static Task Delay(this TimeProvider timeProvider, TimeSpan delay, Cancell state.Registration = cancellationToken.Register(static delayState => { DelayState s = (DelayState)delayState!; - s.TrySetCanceled(s.CancellationToken); + + // When cancellation is requested, we need to force the task continuation to run asynchronously + ThreadPool.UnsafeQueueUserWorkItem(static state => + { + DelayState theState = (DelayState)state; + theState.TrySetCanceled(theState.CancellationToken); + }, s); + s.Registration.Dispose(); s.Timer?.Dispose(); }, state); From a3cc03f0cbf312cfe92b63b876d46fa254736887 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Mon, 14 Aug 2023 08:35:40 -0700 Subject: [PATCH 3/8] Update src/libraries/Microsoft.Bcl.TimeProvider/src/System/Threading/Tasks/TimeProviderTaskExtensions.cs Co-authored-by: Stephen Toub --- .../src/System/Threading/Tasks/TimeProviderTaskExtensions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 a8595455445415..935c301a7d08a7 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 @@ -15,7 +15,7 @@ public static class TimeProviderTaskExtensions #if !NET8_0_OR_GREATER private sealed class DelayState : TaskCompletionSource { - public DelayState(CancellationToken cancellationToken) : base() + public DelayState(CancellationToken cancellationToken) { CancellationToken = cancellationToken; } From 54947b4622cd554bec55e28ecece20a529642e70 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Mon, 14 Aug 2023 08:36:23 -0700 Subject: [PATCH 4/8] Apply suggestions from code review Co-authored-by: Stephen Toub --- src/libraries/Common/tests/System/TimeProviderTests.cs | 2 +- .../src/System/Threading/Tasks/TimeProviderTaskExtensions.cs | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/Common/tests/System/TimeProviderTests.cs b/src/libraries/Common/tests/System/TimeProviderTests.cs index f353c9deee9bfa..4d4be2ba080c1b 100644 --- a/src/libraries/Common/tests/System/TimeProviderTests.cs +++ b/src/libraries/Common/tests/System/TimeProviderTests.cs @@ -656,7 +656,7 @@ public void TestDelayTaskContinuation(ITestTaskFactory taskFactory) // ManualTimeProvider manualTimeProvider = new ManualTimeProvider(); - var callbackCount = 0; + int callbackCount = 0; _ = Continuation(manualTimeProvider, default, () => callbackCount++); 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 935c301a7d08a7..1497b84c4aa3a6 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 @@ -91,6 +91,7 @@ public static Task Delay(this TimeProvider timeProvider, TimeSpan delay, Cancell DelayState s = (DelayState)delayState!; // When cancellation is requested, we need to force the task continuation to run asynchronously + // to avoid doing arbitrary amounts of work as part of a call to CancellationTokenSource.Cancel. ThreadPool.UnsafeQueueUserWorkItem(static state => { DelayState theState = (DelayState)state; From eebe805016cef402d2717d62d052af586ceb0eb3 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Mon, 14 Aug 2023 10:34:35 -0700 Subject: [PATCH 5/8] Updated the test --- .../Common/tests/System/TimeProviderTests.cs | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/libraries/Common/tests/System/TimeProviderTests.cs b/src/libraries/Common/tests/System/TimeProviderTests.cs index 4d4be2ba080c1b..3a6c96e2ea4976 100644 --- a/src/libraries/Common/tests/System/TimeProviderTests.cs +++ b/src/libraries/Common/tests/System/TimeProviderTests.cs @@ -649,7 +649,7 @@ public static IEnumerable TaskFactoryData() [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] [MemberData(nameof(TaskFactoryData))] - public void TestDelayTaskContinuation(ITestTaskFactory taskFactory) + public async void TestDelayTaskContinuation(ITestTaskFactory taskFactory) { // // Test time expiration and validate continuation is called synchronously. @@ -673,14 +673,30 @@ public void TestDelayTaskContinuation(ITestTaskFactory taskFactory) manualTimeProvider = new ManualTimeProvider(); CancellationTokenSource cts = new CancellationTokenSource(); - Task task = Continuation(manualTimeProvider, cts.Token, () => callbackCount++); + var tl = new ThreadLocal(); + tl.Value = 10; + int t1Value = 0; + + Task task = Continuation(manualTimeProvider, cts.Token, () => t1Value = tl.Value); cts.Cancel(); - Assert.Throws(() => task.GetAwaiter().GetResult()); + // rest the thread local value as the continuation callback could end up running on the this thread pool thread. + tl.Value = 0; + await task; + + Assert.NotEqual(10, t1Value); async Task Continuation(TimeProvider timeProvider, CancellationToken token, Action callback) { - await taskFactory.Delay(timeProvider, TimeSpan.FromSeconds(10), token); + try + { + await taskFactory.Delay(timeProvider, TimeSpan.FromSeconds(10), token); + } + catch (TaskCanceledException) + { + // Ignore + } + callback(); } } From 87247e5ee24f4c105bb4d5cfda4b2d3e7400bcfe Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Mon, 14 Aug 2023 10:36:55 -0700 Subject: [PATCH 6/8] Update src/libraries/Common/tests/System/TimeProviderTests.cs Co-authored-by: Stephen Toub --- src/libraries/Common/tests/System/TimeProviderTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Common/tests/System/TimeProviderTests.cs b/src/libraries/Common/tests/System/TimeProviderTests.cs index 3a6c96e2ea4976..09dc1b6bdd6f68 100644 --- a/src/libraries/Common/tests/System/TimeProviderTests.cs +++ b/src/libraries/Common/tests/System/TimeProviderTests.cs @@ -649,7 +649,7 @@ public static IEnumerable TaskFactoryData() [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] [MemberData(nameof(TaskFactoryData))] - public async void TestDelayTaskContinuation(ITestTaskFactory taskFactory) + public async Task TestDelayTaskContinuation(ITestTaskFactory taskFactory) { // // Test time expiration and validate continuation is called synchronously. From 60b19fc6252d0da363514d165b85a79f002f700f Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Mon, 14 Aug 2023 10:39:08 -0700 Subject: [PATCH 7/8] Update src/libraries/Common/tests/System/TimeProviderTests.cs Co-authored-by: Stephen Toub --- src/libraries/Common/tests/System/TimeProviderTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Common/tests/System/TimeProviderTests.cs b/src/libraries/Common/tests/System/TimeProviderTests.cs index 09dc1b6bdd6f68..acfa1d51f8ca4d 100644 --- a/src/libraries/Common/tests/System/TimeProviderTests.cs +++ b/src/libraries/Common/tests/System/TimeProviderTests.cs @@ -692,7 +692,7 @@ async Task Continuation(TimeProvider timeProvider, CancellationToken token, Acti { await taskFactory.Delay(timeProvider, TimeSpan.FromSeconds(10), token); } - catch (TaskCanceledException) + catch (OperationCanceledException) { // Ignore } From d4f73ea844f3b266edaa569bcbb5c5c5eb40e3ed Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Mon, 14 Aug 2023 10:39:51 -0700 Subject: [PATCH 8/8] Update src/libraries/Common/tests/System/TimeProviderTests.cs Co-authored-by: Stephen Toub --- src/libraries/Common/tests/System/TimeProviderTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Common/tests/System/TimeProviderTests.cs b/src/libraries/Common/tests/System/TimeProviderTests.cs index acfa1d51f8ca4d..45148afbcbb4fc 100644 --- a/src/libraries/Common/tests/System/TimeProviderTests.cs +++ b/src/libraries/Common/tests/System/TimeProviderTests.cs @@ -680,7 +680,7 @@ public async Task TestDelayTaskContinuation(ITestTaskFactory taskFactory) Task task = Continuation(manualTimeProvider, cts.Token, () => t1Value = tl.Value); cts.Cancel(); - // rest the thread local value as the continuation callback could end up running on the this thread pool thread. + // reset the thread local value as the continuation callback could end up running on this thread pool thread. tl.Value = 0; await task;