diff --git a/src/Polly/Timeout/AsyncTimeoutEngine.cs b/src/Polly/Timeout/AsyncTimeoutEngine.cs index c161463497b..d451964ec28 100644 --- a/src/Polly/Timeout/AsyncTimeoutEngine.cs +++ b/src/Polly/Timeout/AsyncTimeoutEngine.cs @@ -37,7 +37,6 @@ internal static async Task ImplementationAsync( actionTask = action(context, combinedToken); return await (await Task.WhenAny(actionTask, timeoutTask).ConfigureAwait(continueOnCapturedContext)).ConfigureAwait(continueOnCapturedContext); - } catch (Exception ex) { @@ -51,6 +50,16 @@ internal static async Task ImplementationAsync( throw; } + finally + { + // 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(); + } + } } private static Task AsTask(this CancellationToken cancellationToken) @@ -60,11 +69,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; } diff --git a/test/Polly.Specs/Timeout/TimeoutAsyncSpecs.cs b/test/Polly.Specs/Timeout/TimeoutAsyncSpecs.cs index 6e7428f3747..44168f0c767 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,29 @@ 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); + bool isCancelled = false; + + var act = () => policy.ExecuteAsync(async (combinedToken) => + { + combinedToken.Register(() => isCancelled = true); + await SystemClock.SleepAsync(TimeSpan.FromMilliseconds(1000), combinedToken); + }, CancellationToken.None); + + await act.Should().ThrowAsync(); + + isCancelled.Should().BeTrue(); + } + #endregion #region Timeout operation - optimistic