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:
|
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)):
}
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:
go-retryablehttp/client.go
Lines 771 to 791 in 867c4f5
Before the
selectblock, 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.Timerand.Stop()is unnecessary and can just betime.After(...)in modern go, and even if backwards compatibility is an issue thendefer timer.Stop()should be preferred...So, how about this?