Merge pull request #118 from RatanShreshtha/merge-from-master-2019-06-05

Merge from master (Jun 05th, 2019)

Author: pquentin

CHANGES.rst

@@ -24,6 +24,14 @@ Upcoming 2.0 Changes
   useful to persist them up to the user.
 
 * Removed ``BodyNotHttplibCompatible`` and ``ResponseNotChunked`` exceptions.
+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)
 -------------------

CONTRIBUTORS.txt

@@ -275,5 +275,8 @@ In chronological order:
 * Katsuhiko YOSHIDA <https://github.com/kyoshidajp>
   * Remove Authorization header regardless of case when redirecting to cross-site
 
+* James Meickle <https://permadeath.com/>
+  * Improve handling of Retry-After header
+
 * [Your name or handle] <[email or website]>
   * [Brief summary of your changes]

docs/user-guide.rst

@@ -302,8 +302,8 @@ to the standard-library :mod:`ssl` module. You may experience
 Using timeouts
 --------------
 
-Timeouts allow you to control how long requests are allowed to run before
-being aborted. In simple cases, you can specify a timeout as a ``float``
+Timeouts allow you to control how long (in seconds) requests are allowed to run
+before being aborted. In simple cases, you can specify a timeout as a ``float``
 to :meth:`~poolmanager.PoolManager.request`::
 
     >>> http.request(

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

src/urllib3/util/timeout.py

@@ -46,19 +46,20 @@ class Timeout(object):
     :type total: integer, float, or None
 
     :param connect:
-        The maximum amount of time to wait for a connection attempt to a server
-        to succeed. Omitting the parameter will default the connect timeout to
-        the system default, probably `the global default timeout in socket.py
+        The maximum amount of time (in seconds) to wait for a connection
+        attempt to a server to succeed. Omitting the parameter will default the
+        connect timeout to the system default, probably `the global default
+        timeout in socket.py
         <http://hg.python.org/cpython/file/603b4d593758/Lib/socket.py#l535>`_.
         None will set an infinite timeout for connection attempts.
 
     :type connect: integer, float, or None
 
     :param read:
-        The maximum amount of time to wait between consecutive
-        read operations for a response from the server. Omitting
-        the parameter will default the read timeout to the system
-        default, probably `the global default timeout in socket.py
+        The maximum amount of time (in seconds) to wait between consecutive
+        read operations for a response from the server. Omitting the parameter
+        will default the read timeout to the system default, probably `the
+        global default timeout in socket.py
         <http://hg.python.org/cpython/file/603b4d593758/Lib/socket.py#l535>`_.
         None will set an infinite timeout.
 
@@ -195,7 +196,7 @@ def start_connect(self):
     def get_connect_duration(self):
         """ Gets the time elapsed since the call to :meth:`start_connect`.
 
-        :return: Elapsed time.
+        :return: Elapsed time in seconds.
         :rtype: float
         :raises urllib3.exceptions.TimeoutStateError: if you attempt
             to get duration for a timer that hasn't been started.

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()