Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle CancellationToken for Retry #2396

Closed
wants to merge 4 commits into from
Closed

Handle CancellationToken for Retry #2396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1401b0e
Handle CancellationToken for Retry
peter-csala Nov 18, 2024
a77284a
Handle CancellationToken for Retry
peter-csala Nov 18, 2024
b852765
Handle CancellationToken for Retry
peter-csala Nov 18, 2024
802eb93
Handle CancellationToken for Retry
peter-csala Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Polly.Core/Retry/RetryResilienceStrategy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ protected internal override async ValueTask<Outcome<T>> ExecuteCore<TState>(Func
{
var startTimestamp = _timeProvider.GetTimestamp();
var outcome = await StrategyHelper.ExecuteCallbackSafeAsync(callback, context, state).ConfigureAwait(context.ContinueOnCapturedContext);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to be aware. If the outcome with disposable result is produced and then you are replacing it with exception, it might lead to a memory leak.

I think it might be good idea to encapsulate all this handling into ExecuteCallbackSafeAsync method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we would move the code inside the ExecuteCallbackSafeAsync then we don't have access to the isLastAttempt. @kmcclellan suggested to respect the cancellation request only under certain circumstances.

Please see the other comment section for more details.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we would move the code inside the ExecuteCallbackSafeAsync then we don't have access to the isLastAttempt.

The ExecuteCallbackSafeAsync is internal, we could pass that information to the function. Wdyt?

Copy link
Contributor Author

@peter-csala peter-csala Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That function is used by almost all strategies, where the "retry" concept may or may not make any sense. Of course we can pass true as a default for isLastAttempt but it feels a bit awkward to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you still suggesting to add a new isLastAttempt parameter with default value? I'm fine with either approach, I just want to close this PR this year if possible 😁

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaah sorry about this, got busy with other stuff. I would say meh, we can add it, it's internal detail anyway. At least this logic will be encapsulated there.

So a new default value with isLastAttempt = true. Retry strategy will provide its own value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I am the reporter of the bug. Hoping I can help move this forward to the right solution!

The concern about disposable outcomes being thrown away is not really relevant to this change. Abiding by the rules of cancellation, we should not replace an outcome unless we have more work to do (in which case it was probably an exception and won't have a result to dispose).

Don’t throw OperationCanceledException after you’ve completed the work, just because the token was signaled

It is only handled outcomes that are not the last outcome which could have a disposable result thrown away. This is true whether we attempt another execution or acknowledge cancellation. It's fair to say that retry policies currently don't support handling disposable outcomes.

I'm not even sure what ExecuteCallbackSafeAsync would supposedly do with isLastAttempt. In order to dispose, it would also need to know that the strategy would handle the outcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The strategy requires the operation's outcome to decide whether to perform a retry attempt or not.

If we want to respect the cancellation request after the strategy has decided to handle the outcome or not and whether this was a last attempt or not then we have the following situation:

  • ShouldHandle's args receives the outcome of the operation
  • Then we might replace the outcome with an OCE depending on the conditions

That means inside the ShouldHandle delegate users would see the operation's outcome but inside the telemetry and at the Execute{Async}'s result they might see an OCE. This also feels a bit inconsistent behavior for me.

if (context.CancellationToken.IsCancellationRequested)
{
outcome = Outcome.FromException<T>(new OperationCanceledException(context.CancellationToken));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to acknowledge cancellation only when the retry policy would handle the outcome. It's important that a valid outcome be surfaced by the policy, since the choice to continue processing may well have been deliberate (cf. item three of MS recommendations).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this PR:

  • If the cancellation is requested before the execution of a given attempt (either the original or any handled retry attempt) then it will short cut the execution with an OCE.
  • If the cancellation is requested during the OnRetry then it will short cut the execution with an OCE.
  • If the cancellation is requested during the waiting of the retry delay then it will short cut the execution with an OCE.

This PR handles the following case:

  • If the cancellation is requested during the execution of the user callback then regardless of the outcome then it will short cut the execution with an OCE.

Just to clarify: are you asking to

  • return the outcome of the user callback regardless of the cancellation was requested (or not) if the strategy won't handle the outcome of the user callback
  • return the outcome of the user callback if the strategy would handle the outcome and it was the last attempt
  • short cut the execution with an OCE if the strategy would handle the outcome it was not the last attempt

Is my understanding correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. Only a handled outcome that is not the last outcome should throw, since this is the only scenario where more work remains to be done by the policy. Under no circumstances should a handled outcome that is not the last outcome be returned (as is currently possible).

}

var shouldRetryArgs = new RetryPredicateArguments<T>(context, outcome, attempt);
var handle = await ShouldHandle(shouldRetryArgs).ConfigureAwait(context.ContinueOnCapturedContext);
var executionTime = _timeProvider.GetElapsedTime(startTimestamp);
Expand Down
75 changes: 71 additions & 4 deletions test/Polly.Core.Tests/Retry/RetryResilienceStrategyTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,13 @@ public void ExecuteAsync_EnsureResultNotDisposed()
}

[Fact]
public async Task ExecuteAsync_CancellationRequested_EnsureNotRetried()
public async Task ExecuteAsync_CancellationRequestedBeforeCallback_EnsureNoAttempt()
{
SetupNoDelay();
var sut = CreateSut();
using var cancellationToken = new CancellationTokenSource();
_options.ShouldHandle = _ => PredicateResult.True();
var sut = CreateSut();

cancellationToken.Cancel();
var context = ResilienceContextPool.Shared.Get();
context.CancellationToken = cancellationToken.Token;
Expand All @@ -47,10 +49,36 @@ public async Task ExecuteAsync_CancellationRequested_EnsureNotRetried()
}

[Fact]
public async Task ExecuteAsync_CancellationRequestedAfterCallback_EnsureNotRetried()
public async Task ExecuteAsync_CancellationRequestedDuringCallback_EnsureNotRetried()
{
SetupNoDelay();
using var cancellationToken = new CancellationTokenSource();
_options.ShouldHandle = _ => PredicateResult.True();
var sut = CreateSut();

var context = ResilienceContextPool.Shared.Get();
context.CancellationToken = cancellationToken.Token;
var executed = false;
var attemptCounter = 0;

var result = await sut.ExecuteOutcomeAsync((_, _) =>
{
executed = true;
++attemptCounter;
cancellationToken.Cancel();
return Outcome.FromResultAsValueTask("dummy");
}, context, "state");

result.Exception.Should().BeOfType<OperationCanceledException>();
executed.Should().BeTrue();
attemptCounter.Should().Be(1);
}

[Fact]
public async Task ExecuteAsync_CancellationRequestedAfterCallback_EnsureNotRetried()
{
SetupNoDelay();
using var cancellationToken = new CancellationTokenSource();
_options.ShouldHandle = _ => PredicateResult.True();
_options.OnRetry = _ =>
{
Expand All @@ -62,10 +90,49 @@ public async Task ExecuteAsync_CancellationRequestedAfterCallback_EnsureNotRetri
var context = ResilienceContextPool.Shared.Get();
context.CancellationToken = cancellationToken.Token;
var executed = false;
var attemptCounter = 0;

var result = await sut.ExecuteOutcomeAsync((_, _) =>
{
executed = true;
++attemptCounter;
return Outcome.FromResultAsValueTask("dummy");
}, context, "state");

var result = await sut.ExecuteOutcomeAsync((_, _) => { executed = true; return Outcome.FromResultAsValueTask("dummy"); }, context, "state");
result.Exception.Should().BeOfType<OperationCanceledException>();
executed.Should().BeTrue();
attemptCounter.Should().Be(1);
}

[Fact]
public async Task ExecuteAsync_CancellationRequestedDuringDelay_EnsureNotRetried()
{
using var cancellationToken = new CancellationTokenSource();
_options.Delay = TimeSpan.FromMilliseconds(100);
_options.ShouldHandle = _ => PredicateResult.True();
_options.OnRetry = _ =>
{
cancellationToken.Cancel();
return default;
};

var sut = CreateSut(TimeProvider.System);
var context = ResilienceContextPool.Shared.Get();
context.CancellationToken = cancellationToken.Token;
var executed = false;
var attemptCounter = 0;

var result = await sut.ExecuteOutcomeAsync((_, _) =>
{
executed = true;
++attemptCounter;
return Outcome.FromResultAsValueTask("dummy");
}, context, "state");

result.Exception.Should().BeAssignableTo<OperationCanceledException>();
cancellationToken.Token.IsCancellationRequested.Should().BeTrue();
executed.Should().BeTrue();
attemptCounter.Should().Be(1);
}

[Fact]
Expand Down
Loading