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

Improve implementation of respect_retry_after_header #1607

Merged
merged 8 commits into from Jun 4, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/urllib3/util/retry.py
Expand Up @@ -210,6 +210,7 @@ def new(self, **kw):
raise_on_status=self.raise_on_status,
history=self.history,
remove_headers_on_redirect=self.remove_headers_on_redirect,
respect_retry_after_header=self.respect_retry_after_header,
)
params.update(kw)
return type(self)(**params)
Expand Down Expand Up @@ -294,7 +295,7 @@ def sleep(self, response=None):
this method will return immediately.
"""

if response:
if self.respect_retry_after_header and response:
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a new unit test that hits this condition?

slept = self.sleep_for_retry(response)
if slept:
return
Expand Down
47 changes: 47 additions & 0 deletions test/test_retry.py
@@ -1,10 +1,13 @@
from mock import patch
import pytest

from urllib3.response import HTTPResponse
from urllib3.packages import six
from urllib3.packages.six.moves import xrange
from urllib3.util.retry import Retry, RequestHistory
from urllib3.exceptions import (
ConnectTimeoutError,
InvalidHeader,
MaxRetryError,
ReadTimeoutError,
ResponseError,
Expand Down Expand Up @@ -271,3 +274,47 @@ def test_retry_set_remove_headers_on_redirect(self):
retry = Retry(remove_headers_on_redirect=["X-API-Secret"])

assert list(retry.remove_headers_on_redirect) == ["x-api-secret"]

@pytest.mark.parametrize("value", ["-1", "+1", "1.0", six.u("\xb2")]) # \xb2 = ^2
def test_parse_retry_after_invalid(self, value):
retry = Retry()
with pytest.raises(InvalidHeader):
retry.parse_retry_after(value)

@pytest.mark.parametrize(
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a test case for an http-date formatted Retry-After value? May require mocking time.time() to get a consistent result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added four cases here (whether the time is in the future or not X whether we're respecting the header or not).

"value, expected", [("0", 0), ("1000", 1000), ("\t42 ", 42)]
)
def test_parse_retry_after(self, value, expected):
retry = Retry()
assert retry.parse_retry_after(value) == expected

@pytest.mark.parametrize("respect_retry_after_header", [True, False])
def test_respect_retry_after_header_propagated(self, respect_retry_after_header):

retry = Retry(respect_retry_after_header=respect_retry_after_header)
new_retry = retry.new()
assert new_retry.respect_retry_after_header == respect_retry_after_header

@pytest.mark.parametrize(
"retry_after_header,respect_retry_after_header,sleep_duration",
[("3600", True, 3600), ("3600", False, None)],
)
def test_respect_retry_after_header_sleep(
self, retry_after_header, respect_retry_after_header, sleep_duration
):
retry = Retry(respect_retry_after_header=respect_retry_after_header)

with patch("time.sleep") as sleep_mock:
# for the default behavior, it must be in RETRY_AFTER_STATUS_CODES
response = HTTPResponse(
status=503, headers={"Retry-After": retry_after_header}
)

retry.sleep(response)

# The expected behavior is that we'll only sleep if respecting
# this header (since we won't have any backoff sleep attempts)
if respect_retry_after_header:
sleep_mock.assert_called_with(sleep_duration)
else:
sleep_mock.assert_not_called()
15 changes: 0 additions & 15 deletions test/test_util.py
Expand Up @@ -13,7 +13,6 @@
from urllib3 import add_stderr_logger, disable_warnings
from urllib3.util.request import make_headers, rewind_body, _FAILEDTELL
from urllib3.util.response import assert_header_parsing
from urllib3.util.retry import Retry
from urllib3.util.timeout import Timeout
from urllib3.util.url import get_host, parse_url, split_first, Url
from urllib3.util.ssl_ import (
Expand All @@ -27,7 +26,6 @@
TimeoutStateError,
InsecureRequestWarning,
SNIMissingWarning,
InvalidHeader,
UnrewindableBodyError,
)
from urllib3.util.connection import allowed_gai_family, _has_ipv6
Expand Down Expand Up @@ -782,19 +780,6 @@ def test_ip_family_ipv6_disabled(self):
with patch("urllib3.util.connection.HAS_IPV6", False):
assert allowed_gai_family() == socket.AF_INET

@pytest.mark.parametrize("value", ["-1", "+1", "1.0", six.u("\xb2")]) # \xb2 = ^2
def test_parse_retry_after_invalid(self, value):
retry = Retry()
with pytest.raises(InvalidHeader):
retry.parse_retry_after(value)

@pytest.mark.parametrize(
"value, expected", [("0", 0), ("1000", 1000), ("\t42 ", 42)]
)
def test_parse_retry_after(self, value, expected):
retry = Retry()
assert retry.parse_retry_after(value) == expected

@pytest.mark.parametrize("headers", [b"foo", None, object])
def test_assert_header_parsing_throws_typeerror_with_non_headers(self, headers):
with pytest.raises(TypeError):
Expand Down