Improve implementation of respect_retry_after_header#1607
Improve implementation of respect_retry_after_header#1607sethmlarson merged 8 commits intourllib3:masterfrom
Conversation
|
I took a glance at the test failures, looks spurious? |
|
Right, they're not related to your changes and happen in other pull requests too. |
Codecov Report
@@ Coverage Diff @@
## master #1607 +/- ##
==========================================
- Coverage 99.47% 99.47% -0.01%
==========================================
Files 22 22
Lines 1915 1900 -15
==========================================
- Hits 1905 1890 -15
Misses 10 10
Continue to review full report at Codecov.
|
|
Cycling to get an automatic rebase for CI |
|
Looks like it passed this time. Is this mergeable? Do I need any additional tests, contributor agreement, signature in blood, etc.? :) |
sethmlarson
left a comment
There was a problem hiding this comment.
Thanks for this, just a few comments on this PR. Could we add a changelog entry for this change? Also looks like you need to resolve some conflicts after some other PRs were merged.
| history=self.history, | ||
| remove_headers_on_redirect=self.remove_headers_on_redirect | ||
| remove_headers_on_redirect=self.remove_headers_on_redirect, | ||
| respect_retry_after_header=self.respect_retry_after_header |
| """ | ||
|
|
||
| if response: | ||
| if self.respect_retry_after_header and response: |
There was a problem hiding this comment.
Could we add a new unit test that hits this condition?
|
@sethmlarson I did this:
What's the appropriate way for a changelog bump? Does this become a new tagged release by itself, or is there a "on master but not released" place to put changelogs? |
sethmlarson
left a comment
There was a problem hiding this comment.
This is looking good, one more comment!
For unreleased changes we typically have this header:
dev (master)
------------
along with the change, PR, issue number etc. Until we prepare a release it'll be unreleased on master. :) If you haven't added yourself already to CONTRIBUTORS.txt you should.
| with pytest.raises(InvalidHeader): | ||
| retry.parse_retry_after(value) | ||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
Can we add a test case for an http-date formatted Retry-After value? May require mocking time.time() to get a consistent result.
There was a problem hiding this comment.
Added four cases here (whether the time is in the future or not X whether we're respecting the header or not).
|
@sethmlarson added the above, how's that look? |
| ) | ||
| ) | ||
|
|
||
| with mock.patch("time.sleep") as sleep_mock, mock.patch( |
There was a problem hiding this comment.
TIL about this construction! Thanks for showing me this. :)
|
This PR is only failing for Python 3.8 due to reasons not related to this PR. Once I resolve those issues I'll cycle CI and merge. Thank you so much for this! :) If you've got more time and want to help out more there are plenty of issues marked "Contributor Friendly" that need eyes on them. |
|
Thanks for this! |
respect_retry_after_headeris accepted as an argument toRetry, but it isn't propagated onRetry.new(). This means that after the first retry is incremented, this setting will be lost.Additionally, this setting isn't properly applied in cases where the
retry_afterheader is present, but the intent is not to respect it. This edge case can occur if you explicitly allow retries for the same codes asRETRY_AFTER_STATUS_CODES = frozenset([413, 429, 503]), but don't respect the value in the header, and instead fall back to yourRetry's exponential backoff settings. In our case, we're dealing with an API that returns a large static value in this header even though we can actually try again much sooner because rate limits are governed by a token bucket.I've attached a patch that addresses these two cases and adds a test case for one of them. However, I can't get the test suite to run locally (at all), I'm afraid.