-
Notifications
You must be signed in to change notification settings - Fork 29
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.
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
Dealing with a canceled Context #8
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, but can you move the test into an existing test file (maybe retry_test.go)?
@@ -66,6 +73,13 @@ func Do(ctx context.Context, b Backoff, f RetryFunc) error { | |||
return rerr.Unwrap() | |||
} | |||
|
|||
// ctx.Done() has priority, so we test it alone first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If only Go had a priority for select
...
Can you tell me more about this? |
WithJitter may produce a duration 0 when the magnitude of the jitter exceeds the duration of the "next backoff": val = 0 This case does not produce a hot loop though, because statistically there is more delay added by positive jitter, than subtracted by negative jitter. |
Test moved to retry_test.go |
This pull request has been automatically locked since there has not |
Hi Seth, I spotted a small bug with a straightforward fix so I'm taking the liberty to make a PR without creating an issue first.
Current behaviour
NewConstant
rejects a duration of0
Backoff.Next()
returns a duration of0
, e.g. when usingWithJitter
Do
executes the user func at least once, even if the provided Context is already canceledProblems
0
, it is possible that the user func gets retried, bypassing the Context cancelation check. This is because Go'sselect
statement doesn't let us specify case priority, when several cases are ready to proceed.Do
should not execute the user func at all.Fix
ctx.Done()
alone, before theselect
with 2 cases.ctx.Done()
before the very first call to the user func.For readability, please find 3 commits. The 1st commit is a test that usually fails, before the code fix.