Skip to content

Protect against retry delay overflows #1522

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

Merged
merged 1 commit into from
Aug 30, 2023
Merged

Protect against retry delay overflows #1522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions src/Polly.Core/Retry/RetryHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,18 @@ internal static class RetryHelper
public static bool IsValidDelay(TimeSpan delay) => delay >= TimeSpan.Zero;

public static TimeSpan GetRetryDelay(RetryBackoffType type, bool jitter, int attempt, TimeSpan baseDelay, ref double state, Func<double> randomizer)
{
try
{
return GetRetryDelayCore(type, jitter, attempt, baseDelay, ref state, randomizer);
}
catch (OverflowException)
{
return TimeSpan.MaxValue;
}
Comment on lines +17 to +20
Copy link
Member

@martincostello martincostello Aug 29, 2023

Choose a reason for hiding this comment

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

Just wondering what's the use case for trapping the overflow, rather than throwing an exception and the user fixing their code/configuration?

Copy link
Contributor Author

@martintmk martintmk Aug 29, 2023

Choose a reason for hiding this comment

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

Good point, maybe this really should be handled outside of Polly?

For example similar to how done here:

https://github.com/dotnet/extensions/blob/f1175430d64364b2a90e0188ba5caa8042c339f3/src/Libraries/Microsoft.Extensions.Http.Resilience/Internal/ValidationHelper.cs#L49

One use-case is really high number of retries, at some point the retry strategy will will fail with overflow.
Is it an user-error or Polly error?

Copy link
Contributor

@peter-csala peter-csala Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@martintmk
A kinda weird use case just bumped into my mind. (It is slightly related to this overflow problem) On StackOverflow once I have seen a question where the OP wanted to use the retry policy to schedule jobs to run them periodically (say once in a day).

I highly discouraged the OP to use Polly V7 for that. But what's the case for V8? Is the new implementation suitable for this use case? Or should be people directed to dedicated job scheduler solution?

Copy link
Contributor Author

@martintmk martintmk Aug 29, 2023

Choose a reason for hiding this comment

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

It does not seems to be the use-case of Polly. Yes, you can misuse retries to do "periodical" executions, but eyebrows would be raised :)

Copy link
Contributor

@peter-csala peter-csala Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What do you think would it make sense to have an "anti-patterns" section on the wiki/readme?

Copy link
Contributor Author

@martintmk martintmk Aug 29, 2023

Choose a reason for hiding this comment

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

If there are enough of those, I would say yes.

Copy link
Member

@martincostello martincostello Aug 29, 2023

Choose a reason for hiding this comment

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

I'd agree - using Polly to do that sounds like a case of "when you have a hammer, everything looks like a nail".

We could maybe document anti-patterns, but I would only do so for things that are common mistakes or there's subtlety as to why they are - otherwise the number of things you shouldn't use Polly for is basically infinite.

Copy link
Contributor

@peter-csala peter-csala Aug 29, 2023

Choose a reason for hiding this comment

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

I will collect anti-patterns that I have seen on SO in the past 3 years and I'll create an issue to discuss them and come up with suggested alternatives for V8

}

private static TimeSpan GetRetryDelayCore(RetryBackoffType type, bool jitter, int attempt, TimeSpan baseDelay, ref double state, Func<double> randomizer)
{
if (baseDelay == TimeSpan.Zero)
{
Expand Down
8 changes: 8 additions & 0 deletions test/Polly.Core.Tests/Retry/RetryHelperTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,14 @@ public void Exponential_Ok()
RetryHelper.GetRetryDelay(RetryBackoffType.Exponential, false, 2, TimeSpan.FromSeconds(1), ref state, _randomizer).Should().Be(TimeSpan.FromSeconds(4));
}

[Fact]
public void GetRetryDelay_Overflow_ReturnsMaxTimeSpan()
{
double state = 0;

RetryHelper.GetRetryDelay(RetryBackoffType.Exponential, false, 1000, TimeSpan.FromDays(1), ref state, _randomizer).Should().Be(TimeSpan.MaxValue);
}

[InlineData(1)]
[InlineData(2)]
[InlineData(3)]
Expand Down