-
Notifications
You must be signed in to change notification settings - Fork 282
Open
Description
For example, if the backoff is such that the next attempt is in 10 minutes, and the context's deadline is in 5 minutes, then the code will wait for 5 minutes for no reason.
Refer to this code:
Lines 771 to 791 in 867c4f5
| wait := c.Backoff(c.RetryWaitMin, c.RetryWaitMax, i, resp) | |
| if logger != nil { | |
| desc := fmt.Sprintf("%s %s", req.Method, redactURL(req.URL)) | |
| if resp != nil { | |
| desc = fmt.Sprintf("%s (status: %d)", desc, resp.StatusCode) | |
| } | |
| switch v := logger.(type) { | |
| case LeveledLogger: | |
| v.Debug("retrying request", "request", desc, "timeout", wait, "remaining", remain) | |
| case Logger: | |
| v.Printf("[DEBUG] %s: retrying in %s (%d left)", desc, wait, remain) | |
| } | |
| } | |
| timer := time.NewTimer(wait) | |
| select { | |
| case <-req.Context().Done(): | |
| timer.Stop() | |
| c.HTTPClient.CloseIdleConnections() | |
| return nil, req.Context().Err() | |
| case <-timer.C: | |
| } |
Before the select block, the future retry time should be checked to see if it is beyond the current context's deadline, and return early error in that case.
Also side note: the use of time.Timer and .Stop() is unnecessary and can just be time.After(...) in modern go, and even if backwards compatibility is an issue then defer timer.Stop() should be preferred...
So, how about this?
nextTime := time.Now().Add(wait)
if cd, ok := ctx.Deadline(); ok && nextTime.After(cd) {
return context.DeadlineExceeded // Its the same error we would have returned, we're just returning it earlier...
}
select {
case <-req.Context().Done():
c.HTTPClient.CloseIdleConnections()
return nil, req.Context().Err()
case <-time.After(time.Until(nextTime)):
}
Metadata
Metadata
Assignees
Labels
No labels