From 66b8bdb332ec2741749a3702a92df595c5a08022 Mon Sep 17 00:00:00 2001 From: Peter Lorimer Date: Fri, 13 Oct 2023 14:01:40 -0600 Subject: [PATCH 1/5] Update to extend the lifetime of the combinedTokenSource so that it will cancel downstream operations in TimeoutStrategy.Pessimistic. --- src/Polly/Timeout/AsyncTimeoutEngine.cs | 17 +++++++++-- test/Polly.Specs/Timeout/TimeoutAsyncSpecs.cs | 29 ++++++++++++++++++- 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/src/Polly/Timeout/AsyncTimeoutEngine.cs b/src/Polly/Timeout/AsyncTimeoutEngine.cs index c161463497b..962a2a6e397 100644 --- a/src/Polly/Timeout/AsyncTimeoutEngine.cs +++ b/src/Polly/Timeout/AsyncTimeoutEngine.cs @@ -15,17 +15,28 @@ internal static async Task ImplementationAsync( TimeSpan timeout = timeoutProvider(context); using var timeoutCancellationTokenSource = new CancellationTokenSource(); - using var combinedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCancellationTokenSource.Token); + // Do not use a using here, the exception will exit the scope before we have time to + // notify the downstream of the cancellation. + var combinedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCancellationTokenSource.Token); Task actionTask = null; CancellationToken combinedToken = combinedTokenSource.Token; try { + + var actionTaskFn = async () => + { + var result = await action(context, combinedToken).ConfigureAwait(continueOnCapturedContext); + // dispose of the token source after we've waited on the task to finish. + combinedTokenSource.Dispose(); + return result; + }; + if (timeoutStrategy == TimeoutStrategy.Optimistic) { SystemClock.CancelTokenAfter(timeoutCancellationTokenSource, timeout); - return await action(context, combinedToken).ConfigureAwait(continueOnCapturedContext); + return await actionTaskFn().ConfigureAwait(continueOnCapturedContext); } // else: timeoutStrategy == TimeoutStrategy.Pessimistic @@ -34,7 +45,7 @@ internal static async Task ImplementationAsync( SystemClock.CancelTokenAfter(timeoutCancellationTokenSource, timeout); - actionTask = action(context, combinedToken); + actionTask = actionTaskFn(); return await (await Task.WhenAny(actionTask, timeoutTask).ConfigureAwait(continueOnCapturedContext)).ConfigureAwait(continueOnCapturedContext); diff --git a/test/Polly.Specs/Timeout/TimeoutAsyncSpecs.cs b/test/Polly.Specs/Timeout/TimeoutAsyncSpecs.cs index 6e7428f3747..5c0009945ad 100644 --- a/test/Polly.Specs/Timeout/TimeoutAsyncSpecs.cs +++ b/test/Polly.Specs/Timeout/TimeoutAsyncSpecs.cs @@ -1,4 +1,6 @@ -namespace Polly.Specs.Timeout; +using System; + +namespace Polly.Specs.Timeout; [Collection(Constants.SystemClockDependentTestCollection)] public class TimeoutAsyncSpecs : TimeoutSpecsBase @@ -248,6 +250,31 @@ public async Task Should_rethrow_exception_from_inside_delegate__pessimistic() await policy.Awaiting(p => p.ExecuteAsync(() => throw new NotImplementedException())).Should().ThrowAsync(); } + [Fact] + public async Task Should_cancel_downstream_token_on_timeout__pessimistic() + { + // It seems that there's a difference in the mocked clock vs. the real time clock. + // This test does not fail when using the mocked timer. + // In the TimeoutSpecsBase we actually cancel the combined token. Which hides + // the fact that it doesn't actually cancel irl. + SystemClock.Reset(); + + var policy = Policy.TimeoutAsync(TimeSpan.FromMilliseconds(200), TimeoutStrategy.Pessimistic); + var upstreamToken = new CancellationToken(); + bool isCancelled = false; + + var act = () => policy.ExecuteAsync(async (combinedToken) => + { + combinedToken.Register(() => isCancelled = true); + await SystemClock.SleepAsync(TimeSpan.FromMilliseconds(300), combinedToken); + }, upstreamToken); + + await act.Should().ThrowAsync(); + + await SystemClock.SleepAsync(TimeSpan.FromMilliseconds(400), default); + isCancelled.Should().BeTrue(); + } + #endregion #region Timeout operation - optimistic From 43a0d0d9f2dfeab9286422d6a87285a8e2de9ff6 Mon Sep 17 00:00:00 2001 From: Peter Lorimer Date: Mon, 16 Oct 2023 09:22:38 -0600 Subject: [PATCH 2/5] Refactor as per PR comments; cancel in finally if token hasn't been canceled. --- src/Polly/Timeout/AsyncTimeoutEngine.cs | 35 ++++++++++++------------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/src/Polly/Timeout/AsyncTimeoutEngine.cs b/src/Polly/Timeout/AsyncTimeoutEngine.cs index 962a2a6e397..ba07e06fc24 100644 --- a/src/Polly/Timeout/AsyncTimeoutEngine.cs +++ b/src/Polly/Timeout/AsyncTimeoutEngine.cs @@ -17,26 +17,17 @@ internal static async Task ImplementationAsync( using var timeoutCancellationTokenSource = new CancellationTokenSource(); // Do not use a using here, the exception will exit the scope before we have time to // notify the downstream of the cancellation. - var combinedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCancellationTokenSource.Token); + using var combinedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCancellationTokenSource.Token); Task actionTask = null; CancellationToken combinedToken = combinedTokenSource.Token; try { - - var actionTaskFn = async () => - { - var result = await action(context, combinedToken).ConfigureAwait(continueOnCapturedContext); - // dispose of the token source after we've waited on the task to finish. - combinedTokenSource.Dispose(); - return result; - }; - if (timeoutStrategy == TimeoutStrategy.Optimistic) { SystemClock.CancelTokenAfter(timeoutCancellationTokenSource, timeout); - return await actionTaskFn().ConfigureAwait(continueOnCapturedContext); + return await action(context, combinedToken).ConfigureAwait(continueOnCapturedContext); } // else: timeoutStrategy == TimeoutStrategy.Pessimistic @@ -45,10 +36,9 @@ internal static async Task ImplementationAsync( SystemClock.CancelTokenAfter(timeoutCancellationTokenSource, timeout); - actionTask = actionTaskFn(); + actionTask = action(context, combinedToken); return await (await Task.WhenAny(actionTask, timeoutTask).ConfigureAwait(continueOnCapturedContext)).ConfigureAwait(continueOnCapturedContext); - } catch (Exception ex) { @@ -62,6 +52,15 @@ internal static async Task ImplementationAsync( throw; } + finally + { + // If the timeoutCancellation was canceled & our combined token hasn't been signaled, signal it. + // This avoids the exception propagating before the linked token can signal the downstream to cancel. + if (!combinedTokenSource.IsCancellationRequested && timeoutCancellationTokenSource.IsCancellationRequested) + { + combinedTokenSource.Cancel(); + } + } } private static Task AsTask(this CancellationToken cancellationToken) @@ -71,11 +70,11 @@ private static Task AsTask(this CancellationToken cancellation // A generalised version of this method would include a hotpath returning a canceled task (rather than setting up a registration) if (cancellationToken.IsCancellationRequested) on entry. This is omitted, since we only start the timeout countdown in the token _after calling this method. IDisposable registration = null; - registration = cancellationToken.Register(() => - { - tcs.TrySetCanceled(); - registration?.Dispose(); - }, useSynchronizationContext: false); + registration = cancellationToken.Register(() => + { + tcs.TrySetCanceled(); + registration?.Dispose(); + }, useSynchronizationContext: false); return tcs.Task; } From 9f5ed1648ffab5ecf3603dd1026d7c992fa2d51e Mon Sep 17 00:00:00 2001 From: Peter Lorimer Date: Mon, 16 Oct 2023 09:53:18 -0600 Subject: [PATCH 3/5] Update as per PR comments. --- src/Polly/Timeout/AsyncTimeoutEngine.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Polly/Timeout/AsyncTimeoutEngine.cs b/src/Polly/Timeout/AsyncTimeoutEngine.cs index ba07e06fc24..0b22d724924 100644 --- a/src/Polly/Timeout/AsyncTimeoutEngine.cs +++ b/src/Polly/Timeout/AsyncTimeoutEngine.cs @@ -15,8 +15,6 @@ internal static async Task ImplementationAsync( TimeSpan timeout = timeoutProvider(context); using var timeoutCancellationTokenSource = new CancellationTokenSource(); - // Do not use a using here, the exception will exit the scope before we have time to - // notify the downstream of the cancellation. using var combinedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCancellationTokenSource.Token); Task actionTask = null; @@ -54,7 +52,7 @@ internal static async Task ImplementationAsync( } finally { - // If the timeoutCancellation was canceled & our combined token hasn't been signaled, signal it. + // If the timeoutCancellation was canceled & our combined token hasn't been signaled, cancel it. // This avoids the exception propagating before the linked token can signal the downstream to cancel. if (!combinedTokenSource.IsCancellationRequested && timeoutCancellationTokenSource.IsCancellationRequested) { From e249c03d357ab8b24b29a91ddce590d7fa2aeb23 Mon Sep 17 00:00:00 2001 From: Peter Lorimer Date: Mon, 16 Oct 2023 10:23:02 -0600 Subject: [PATCH 4/5] Adjust as per PR comments; remove unnecessary wait time. --- test/Polly.Specs/Timeout/TimeoutAsyncSpecs.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/Polly.Specs/Timeout/TimeoutAsyncSpecs.cs b/test/Polly.Specs/Timeout/TimeoutAsyncSpecs.cs index 5c0009945ad..44168f0c767 100644 --- a/test/Polly.Specs/Timeout/TimeoutAsyncSpecs.cs +++ b/test/Polly.Specs/Timeout/TimeoutAsyncSpecs.cs @@ -260,18 +260,16 @@ public async Task Should_cancel_downstream_token_on_timeout__pessimistic() SystemClock.Reset(); var policy = Policy.TimeoutAsync(TimeSpan.FromMilliseconds(200), TimeoutStrategy.Pessimistic); - var upstreamToken = new CancellationToken(); bool isCancelled = false; var act = () => policy.ExecuteAsync(async (combinedToken) => { combinedToken.Register(() => isCancelled = true); - await SystemClock.SleepAsync(TimeSpan.FromMilliseconds(300), combinedToken); - }, upstreamToken); + await SystemClock.SleepAsync(TimeSpan.FromMilliseconds(1000), combinedToken); + }, CancellationToken.None); await act.Should().ThrowAsync(); - await SystemClock.SleepAsync(TimeSpan.FromMilliseconds(400), default); isCancelled.Should().BeTrue(); } From 9a36f3865a5954918122f8e2953570ff35e909db Mon Sep 17 00:00:00 2001 From: Martin Costello Date: Tue, 17 Oct 2023 10:54:17 +0100 Subject: [PATCH 5/5] Update comment Fix wording and add link to issue. --- src/Polly/Timeout/AsyncTimeoutEngine.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Polly/Timeout/AsyncTimeoutEngine.cs b/src/Polly/Timeout/AsyncTimeoutEngine.cs index 0b22d724924..d451964ec28 100644 --- a/src/Polly/Timeout/AsyncTimeoutEngine.cs +++ b/src/Polly/Timeout/AsyncTimeoutEngine.cs @@ -52,8 +52,9 @@ internal static async Task ImplementationAsync( } finally { - // If the timeoutCancellation was canceled & our combined token hasn't been signaled, cancel it. + // If timeoutCancellationTokenSource was canceled & our combined token hasn't been signaled, cancel it. // This avoids the exception propagating before the linked token can signal the downstream to cancel. + // See https://github.com/App-vNext/Polly/issues/722. if (!combinedTokenSource.IsCancellationRequested && timeoutCancellationTokenSource.IsCancellationRequested) { combinedTokenSource.Cancel();