From 728d9244665ef5b03103cb74d7b409ebe4f23b43 Mon Sep 17 00:00:00 2001 From: James Meickle Date: Tue, 4 Jun 2019 14:32:54 -0400 Subject: [PATCH] Improve implementation of respect_retry_after_header (#1607) --- CHANGES.rst | 9 +++++ CONTRIBUTORS.txt | 3 ++ src/urllib3/util/retry.py | 3 +- test/test_retry.py | 70 +++++++++++++++++++++++++++++++++++++++ test/test_util.py | 15 --------- 5 files changed, 84 insertions(+), 16 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index a772447134..55a9c0c93a 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -1,6 +1,15 @@ Changes ======= +dev (master) +------------ + +* Propagate Retry-After header settings to subsequent retries. (Pull #1607) + +* Fix edge case where Retry-After header was still respected even when + explicitly opted out of. (Pull #1607) + + 1.25.3 (2019-05-23) ------------------- diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index 3da291e594..6f0362c373 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -275,5 +275,8 @@ In chronological order: * Katsuhiko YOSHIDA * Remove Authorization header regardless of case when redirecting to cross-site +* James Meickle + * Improve handling of Retry-After header + * [Your name or handle] <[email or website]> * [Brief summary of your changes] diff --git a/src/urllib3/util/retry.py b/src/urllib3/util/retry.py index c26f48c959..5a049fe65e 100644 --- a/src/urllib3/util/retry.py +++ b/src/urllib3/util/retry.py @@ -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) @@ -294,7 +295,7 @@ def sleep(self, response=None): this method will return immediately. """ - if response: + if self.respect_retry_after_header and response: slept = self.sleep_for_retry(response) if slept: return diff --git a/test/test_retry.py b/test/test_retry.py index 7b228d75d9..c36476f8fa 100644 --- a/test/test_retry.py +++ b/test/test_retry.py @@ -1,10 +1,15 @@ +import datetime +import mock import pytest +import time 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, @@ -271,3 +276,68 @@ 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( + "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), + # Will sleep due to header is 1 hour in future + ("Mon, 3 Jun 2019 12:00:00 UTC", True, 3600), + # Won't sleep due to not respecting header + ("Mon, 3 Jun 2019 12:00:00 UTC", False, None), + # Won't sleep due to current time reached + ("Mon, 3 Jun 2019 11:00:00 UTC", True, None), + # Won't sleep due to current time reached + not respecting header + ("Mon, 3 Jun 2019 11:00:00 UTC", 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) + + # Date header syntax can specify an absolute date; compare this to the + # time in the parametrized inputs above. + current_time = mock.MagicMock( + return_value=time.mktime( + datetime.datetime(year=2019, month=6, day=3, hour=11).timetuple() + ) + ) + + with mock.patch("time.sleep") as sleep_mock, mock.patch( + "time.time", current_time + ): + # 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 and sleep_duration is not None: + sleep_mock.assert_called_with(sleep_duration) + else: + sleep_mock.assert_not_called() diff --git a/test/test_util.py b/test/test_util.py index b2906f225e..37c09ab20e 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -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 ( @@ -27,7 +26,6 @@ TimeoutStateError, InsecureRequestWarning, SNIMissingWarning, - InvalidHeader, UnrewindableBodyError, ) from urllib3.util.connection import allowed_gai_family, _has_ipv6 @@ -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):