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.

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

Remove context deadline checking in backOffContext #113

Merged
merged 2 commits into from
May 31, 2021
Merged

Remove context deadline checking in backOffContext #113

merged 2 commits into from
May 31, 2021

Conversation

swithek
Copy link
Contributor

@swithek swithek commented May 26, 2021

The current implementation of backOffContext checks the context's deadline and marks it as cancelled even when that deadline is not reached yet. This leads to confusion on the RetryNotifyWithTimer caller's side, because there are no clear indications whether the execution was stopped due to the context reaching its deadline (context.DeadlineExceeded is not returned) or some other error. Even checking whether the context is cancelled after the retry func returns, does not work because the context itself might not have been cancelled (only backOffContext marked it internally as being so):

b := backoff.WithContext(backoff.NewConstantBackOff(time.Second), ctx)
err := backoff.Retry(operation, b)
if err != nil {
      if cerr := ctx.Err(); cerr != nil { // there might be no error returned from the context because backOffContext only 'predicts' its cancellation
            //...
      }
}

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 88.987% when pulling aac57c9 on swithek:remove-ctx-deadline into c2975ff on cenkalti:v4.

@cenkalti cenkalti merged commit a78d380 into cenkalti:v4 May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants