cockroachdb / cockroachdb/cockroach

retry: the `StartWithCtx` API makes it too easy to ignore cancellation

Open
#100,659 3 comments 0 reactions 0 assignees View on GitHub
C-bug
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

This is the general pattern we recommend callers use the `retry` package:
```go
retryOpts := base.DefaultRetryOptions()

var lastErr error
for retryable := retry.StartWithCtx(ctx, retryOpts); retryable.Next(); {
lastErr := fn(ctx)
if lastErr == nil {
return nil
}
}
return lastErr
```

This is more-or-less what every caller currently does.

Alas, this fails to properly work if the context is cancelled _before the first iteration_. In that case, `lastErr` remains `nil` but the function is never caller. Then the `return lastErr` at the end will make the caller of this code believe the function was called successfully where, instead, it was not called at all!

**Expected behavior** `retry` should ensure an error is returned in case of premature cancellation of the retry loop; and the API defined in such a way that call points notice this case properly.

Jira issue: CRDB-26537

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.