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

handle repeated 429s from Observable API with exponential back off #1761

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mythmon
Copy link
Member

@mythmon mythmon commented Oct 16, 2024

Framework already respects HTTP 429 rate limit responses from the Observable API. However, it will only retry once. This can sometimes not be enough, especially in cases where there are more than one Framework instances on the same IP, or Framework is not the only thing making request to the Observable API.

This PR changes the retry logic to retry up to 3 times, each time multiplying the Retry-After time the server instructed us to use by successive powers of two. After 3 times, Framework gives up and just lets the request fail.

In the worst case scenario, this would mean Framework waits in total 7 times longer than the server's "Retry-After" header. I've seen this Retry-After header as high as 60 seconds on our API, so this could mean a 7 minute delay.

I'm happy to bike shed on the number of retries or the way in which the time grows. We're also implementing some server side changes to help the situation.

@mythmon mythmon requested a review from mbostock October 16, 2024 20:47
if (response.status === 429) {
let rateLimitCount = 0;
while (response.status === 429) {
if (rateLimitCount >= MAX_RATE_LIMIT_ATTEMPTS) {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like rateLimitCount is only incremented once when this._rateLimit === null; am I reading it wrong? Normally I like to combine the increment and check into the same statement so that it’s more obvious that it can never infinite loop.

Suggested change
if (rateLimitCount >= MAX_RATE_LIMIT_ATTEMPTS) {
if (rateLimitCount++ >= MAX_RATE_LIMIT_ATTEMPTS) {

Copy link
Member Author

@mythmon mythmon Oct 21, 2024

Choose a reason for hiding this comment

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

You're right that the rateLimitCount is only incremented when this._rateLimit is null. However, the bug is something different. this._rateLimit is intended to be used for multiple concurrent callers of this function to wait collaboratively. That's not working with this new approach either though. I'll fix it up.

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.

3 participants