Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(api): no retries on error #25

Closed
mavolin opened this issue May 7, 2020 · 2 comments · Fixed by #26
Closed

fix(api): no retries on error #25

mavolin opened this issue May 7, 2020 · 2 comments · Fixed by #26

Comments

@mavolin
Copy link
Contributor

mavolin commented May 7, 2020

Regarding: httputil.Client.Request

Currently, Request retries on any HTTP Response status that is < 200 or > 299.
However, many errors won't get resolved by a retry, as only 5xx errors indicate a server error and any other status indicates either an error with the data that was sent, etc..
An exception to this is of course code 429 - too many requests which indicates rate limiting.
See also https://en.wikipedia.org/wiki/List_of_HTTP_status_codes.

I would propose to check for server or rate limit errors and retry only then, but let all others break the loop and get handled respectively.
This could be done by simply replacing the

if status = r.GetStatus(); status < 200 || status > 299 {
    continue
}

with

if status = r.GetStatus(); status == http.StatusTooManyRequests || status >= 500 {
    continue
}

.

If you create a branch for it, I can hand in a PR for it.

@diamondburned
Copy link
Owner

You can PR straight into master, that's fine.

@mavolin
Copy link
Contributor Author

mavolin commented May 7, 2020

Wow, that was quick! Will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants