Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions client.go
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,23 @@ func LinearJitterBackoff(min, max time.Duration, attemptNum int, resp *http.Resp
return time.Duration(jitterMin * int64(attemptNum))
}

// RateLimitLinearJitterBackoff wraps the retryablehttp.LinearJitterBackoff.
// It first checks if the response status code is http.StatusTooManyRequests
// (HTTP Code 429) or http.StatusServiceUnavailable (HTTP Code 503). If it is
// and the response contains a Retry-After response header, it will wait the
// amount of time specified by the header. Otherwise, this calls
// LinearJitterBackoff.
Comment on lines +641 to +646

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After removing the status codes checks, the documentation is no longer correct. Let's either fix this or undo the commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted the commit

func RateLimitLinearJitterBackoff(min, max time.Duration, attemptNum int, resp *http.Response) time.Duration {
if resp != nil {
if resp.StatusCode == http.StatusTooManyRequests || resp.StatusCode == http.StatusServiceUnavailable {
if sleep, ok := parseRetryAfterHeader(resp.Header["Retry-After"]); ok {
return sleep
}
}
}
Comment on lines +648 to +654

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it makes sene to ignore Retry-After if it isn't 503 or 429? From what I've understood, the spec doesn't specify the status codes for which this header is valid but instead identifies what the header means for specific status codes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the only other case where a Retry-After header should be sent is with a redirect status. The HTTP client will do redirecting as part of c.HTTPClient.Do, before we get to any retrying, so it's unlikely that we'll end up here with a 3xx status code, but I suppose it's possible.

I can update this to parse the header regardless of status code

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh, I'm not really sure what is the correct way to handle the header. If there is a req to handle other status codes, we'll deal with it then. I'll approve the PR for now. Thanks!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you've already push a commit to ignore the status code. I'm okay with either way of handling the header.

return LinearJitterBackoff(min, max, attemptNum, resp)
}

// PassthroughErrorHandler is an ErrorHandler that directly passes through the
// values from the net/http library for the final request. The body is not
// closed.
Expand Down
107 changes: 107 additions & 0 deletions client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1143,6 +1143,113 @@ func TestClient_PostForm(t *testing.T) {
resp.Body.Close()
}

func TestBackoff_RateLimitLinearJitterBackoff(t *testing.T) {
testCases := []struct {
name string
min time.Duration
max time.Duration
headers http.Header
responseCode int
expect time.Duration
}{
{
name: "429 no retry header",
min: time.Second,
max: time.Second,
headers: http.Header{},
responseCode: http.StatusTooManyRequests,
expect: time.Second,
},
{
name: "503 no retry header",
min: time.Second,
max: time.Second,
headers: http.Header{},
responseCode: http.StatusServiceUnavailable,
expect: time.Second,
},
{
name: "429 retry header",
min: time.Second,
max: time.Second,
headers: http.Header{
"Retry-After": []string{"2"},
},
responseCode: http.StatusTooManyRequests,
expect: 2 * time.Second,
},
{
name: "503 retry header",
min: time.Second,
max: time.Second,
headers: http.Header{
"Retry-After": []string{"2"},
},
responseCode: http.StatusServiceUnavailable,
expect: 2 * time.Second,
},
{
name: "502 ignore retry header",
min: time.Second,
max: time.Second,
headers: http.Header{
"Retry-After": []string{"2"},
},
responseCode: http.StatusBadGateway,
expect: time.Second,
},
{
name: "502 no retry header",
min: time.Second,
max: time.Second,
headers: http.Header{},
responseCode: http.StatusBadGateway,
expect: time.Second,
},
{
name: "429 retry header with jitter",
min: time.Second,
max: 5 * time.Second,
headers: http.Header{
"Retry-After": []string{"2"},
},
responseCode: http.StatusTooManyRequests,
expect: 2 * time.Second,
},
{
name: "429 retry header less than min",
min: 5 * time.Second,
max: 10 * time.Second,
headers: http.Header{
"Retry-After": []string{"2"},
},
responseCode: http.StatusTooManyRequests,
expect: 2 * time.Second,
},
{
name: "429 retry header in range",
min: time.Second,
max: 10 * time.Second,
headers: http.Header{
"Retry-After": []string{"2"},
},
responseCode: http.StatusTooManyRequests,
expect: 2 * time.Second,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
got := RateLimitLinearJitterBackoff(tc.min, tc.max, 0, &http.Response{
StatusCode: tc.responseCode,
Header: tc.headers,
})
if got != tc.expect {
t.Fatalf("expected %s, got %s", tc.expect, got)
}
})
}
}

func TestBackoff(t *testing.T) {
type tcase struct {
min time.Duration
Expand Down
Loading