Protect against retry delay overflows #1522
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1522 +/- ##
=======================================
Coverage 84.11% 84.12%
=======================================
Files 278 278
Lines 6586 6590 +4
Branches 1026 1026
=======================================
+ Hits 5540 5544 +4
Misses 837 837
Partials 209 209
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| catch (OverflowException) | ||
| { | ||
| return TimeSpan.MaxValue; | ||
| } |
There was a problem hiding this comment.
Just wondering what's the use case for trapping the overflow, rather than throwing an exception and the user fixing their code/configuration?
There was a problem hiding this comment.
Good point, maybe this really should be handled outside of Polly?
For example similar to how done here:
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?
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
What do you think would it make sense to have an "anti-patterns" section on the wiki/readme?
There was a problem hiding this comment.
If there are enough of those, I would say yes.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
@martincostello Is it ok to merge this one or should I abandon it? My thinking is that even when user sets some crazy configuration, Polly should be able to handle it (by limiting the maximum delay to max, for example). |
Details on the issue fix or feature implementation
If the calculated delay overflows just return
TimeSpan.MaxValue.Confirm the following