Skip to content

Backoff wait sleep should check that it wouldn't exceed the context Deadline #272

@croepha

Description

@croepha

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

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

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions