-
Notifications
You must be signed in to change notification settings - Fork 154
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
retry requests when rate limit reached #315
Merged
Merged
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
fb179a8
retry requests when rate limit reached
bentranter 01032e2
restore rate limit err when retry-after missing
bentranter 7c85584
remove support for ruby 2.5 (eol 31/03/2021)
bentranter 5aff9fa
only retry rate limits when enabled
bentranter 4264327
document rate limit handling and config
bentranter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
# frozen_string_literal: true | ||
|
||
shared_examples_for 'resource that handles rate limit retries' do | ||
let(:arguments) { {} } | ||
|
||
it 'handles rate limit' do | ||
response_body = { id: :rate_limit, message: 'example' } | ||
stub_do_api(path, method).to_return( | ||
[ | ||
{ | ||
body: nil, | ||
status: 429, | ||
headers: { | ||
'RateLimit-Limit' => 1200, | ||
'RateLimit-Remaining' => 1193, | ||
'RateLimit-Reset' => 1_402_425_459, | ||
'Retry-After' => 0 # Retry immediately in tests. | ||
} | ||
}, | ||
{ | ||
body: response_body.to_json, | ||
status: 200, | ||
headers: { | ||
'RateLimit-Limit' => 1200, | ||
'RateLimit-Remaining' => 1192, | ||
'RateLimit-Reset' => 1_402_425_459 | ||
} | ||
} | ||
] | ||
) | ||
|
||
expect { resource.send(action, arguments) }.not_to raise_error | ||
end | ||
end |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/lostisland/faraday-retry/blob/41b7ea27e30d99ebfed958abfa11d12b01f6b6d1/spec/faraday/retry/middleware_spec.rb#L269-L274 for confirmation, this is surprising behaviour.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 thefaraday-retry
middleware executes before the error handling logic. If it's not present,faraday-retry
takes over and tries to wait until theRateLimit-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.There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.