Respect retry_after in HTTP 429 response#470
Conversation
| if status = r.GetStatus(); status == StatusTooManyRequests || status >= 500 { | ||
| status = r.GetStatus() | ||
|
|
||
| if status == StatusNirnTimeout || status >= 500 { |
There was a problem hiding this comment.
In the case of a 5xx response, I think we should do our own exponential backoff to prevent spamming Discord.
| if status = r.GetStatus(); status == StatusTooManyRequests || status >= 500 { | ||
| status = r.GetStatus() | ||
|
|
||
| if status == StatusNirnTimeout || status >= 500 { |
There was a problem hiding this comment.
Rather than adding a StatusNirnTimeout, I think we should just write status == http.StatusRequestTimeout.
| err := json.Unmarshal(buf.Bytes(), &errBody) | ||
|
|
||
| if err == nil { | ||
| time.Sleep(time.Duration(errBody.RetryAfter) * time.Second) |
There was a problem hiding this comment.
time.Sleep is not context-aware. Prefer writing:
select {
case <-ctx.Done():
doErr = ctx.Err()
continue // or break out of the loop, but this will work too
case <-time.After(...):
continue
}| RetryAfter float32 `json:"retry_after"` | ||
| } | ||
|
|
||
| err := json.Unmarshal(buf.Bytes(), &errBody) |
There was a problem hiding this comment.
We can shorten this to:
| err := json.Unmarshal(buf.Bytes(), &errBody) | |
| err := json.NewDecoder(body).Decode(&errBody) |
|
|
||
| err := json.Unmarshal(buf.Bytes(), &errBody) | ||
|
|
||
| if err == nil { |
There was a problem hiding this comment.
Happy path: put the if err != nil check first, then put the other case after the check.
|
I mainly made these changes so I can it in a project, I agree with your comments and can make the changes when I get around to it! Happy for you to also close this PR and make the changes :) |
2 participants
Arikawa does prevent most 429's by respecting the headers returned by Discord but some rate limits aren't indicated by the headers.