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

retry requests when rate limit reached #315

Merged
merged 5 commits into from Aug 18, 2023

Conversation

bentranter
Copy link
Member

@bentranter bentranter commented Aug 11, 2023

Adds support for retrying requests that fail with 429 using the Retry-After header, similar to what we do in godo.

Adds support for retrying requests that fail with 429 using the
`Retry-After` header, similar to what we do in godo.

This is still WIP, since the README hasn't yet been update to describe
the change in behaviour. I also still need to run some manual tests.
Comment on lines 38 to 43
# faraday-retry supports both the Retry-After and RateLimit-Reset
# headers, however, it favours the RateLimit-Reset one. To force it
# to use the Retry-After header, we override the header that it
# expects for the RateLimit-Reset header to something that we know
# we don't set.
rate_limit_reset_header: 'undefined'
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Looks like we're providing a Unix timestamp and it's expecting seconds until?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've since spent some time testing this manually, and while you're right, I think this is still the best approach for the library.

I've restored the previous 429 status code handling if the Retry-After header is present, so that when the burst limit is reached, the client will wait, but if it's not, a rate limit error is raised. For this to work, this line is needed, since the faraday-retry middleware executes before the error handling logic. If it's not present, faraday-retry takes over and tries to wait until the RateLimit-Reset header, which can (in the absolute worst possible case) wait for an hour. The client being unresponsive for an hour makes it feel like the client is broken, so I opted to return an error in that case.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to override this altogether? Should this be conditional on retry_max being set to greater than 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Update to be conditional on retry_max being set greater than 0.

@bentranter bentranter marked this pull request as ready for review August 16, 2023 17:32
@bentranter
Copy link
Member Author

The tests for Ruby 2.5 were failing in the setup stage when trying to install rubocop. Ruby 2.5 has been eol for 2.5yrs, so I figured it's okay to drop it from the test matrix. If we're okay with that, I'll remove that check from the branch protection rules.

Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

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

👍 LGTM!

@bentranter bentranter merged commit a43a704 into digitalocean:main Aug 18, 2023
12 checks passed
@bentranter bentranter deleted the rate-limit-retry branch August 18, 2023 21:52
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 this pull request may close these issues.

None yet

2 participants