Merge pull request SUSE#172 from SUSE/urllib3-retry

Use `urllib3.Retry` instead of custom implementation

Author: der-gabe

doc/source/api.rst

@@ -85,5 +85,8 @@ Utilities
 .. automodule:: osctiny.utils.mapping
     :members:
 
+.. automodule:: osctiny.utils.session
+    :members:
+
 .. automodule:: osctiny.utils.xml
     :members:

osctiny/osc.py

@@ -4,20 +4,15 @@
 """
 from __future__ import unicode_literals
 
-from base64 import b64encode
 import typing
 import errno
 from http.cookiejar import CookieJar, LWPCookieJar
 from io import BufferedReader, BytesIO, StringIO
 import gc
-import logging
-import os
 from pathlib import Path
 import re
-from ssl import get_default_verify_paths
-import time
 import threading
-from urllib.parse import quote, parse_qs, urlparse
+from urllib.parse import quote, urlparse
 import warnings
 
 from lxml.etree import tostring
@@ -45,6 +40,7 @@
 from .utils.conf import BOOLEAN_PARAMS, get_credentials
 from .utils.cookies import CookieManager
 from .utils.errors import OscError
+from .utils.session import generate_session_id, init_session, RetryPolicy
 from .utils.xml import get_xml_parser, get_objectified_xml
 
 
@@ -132,6 +128,10 @@ class Osc:
     .. versionchanged:: 0.9.0
         * Added the ``staging`` extension
 
+    .. versionchanged:: {{ NEXT_RELEASE }}
+        * Deprecated ``default_connection_retries`` and ``default_retry_timeout``
+        * Introduced :py:class:`osctiny.utils.session.RetryPolicy`
+
     .. _SSL Cert Verification:
         http://docs.python-requests.org/en/master/user/advanced/
         #ssl-cert-verification
@@ -142,6 +142,8 @@ class Osc:
     default_timeout = (60, 300)
     default_connection_retries = 5
     default_retry_timeout = 5
+    retry_policy = RetryPolicy(max_attempts=default_connection_retries,
+                               backoff_max=default_retry_timeout)
 
     def __init__(self, url: typing.Optional[str] = None, username: typing.Optional[str] = None,
                  password: typing.Optional[str] = None, verify: typing.Optional[str] = None,
@@ -180,29 +182,21 @@ def __del__(self):
         # Just in case ;-)
         gc.collect()
 
-    @property
-    def _session_id(self) -> str:
-        session_hash = b64encode(f'{self.username}@{self.url}'.encode()).decode()
-        return f"session_{session_hash}_{os.getpid()}_{threading.get_ident()}"
-
     @property
     def session(self) -> Session:
         """
         Session object
         """
-        session = getattr(THREAD_LOCAL, self._session_id, None)
+        session_id = generate_session_id(username=self.username, url=self.url)
+        session = getattr(THREAD_LOCAL, session_id, None)
         if not session:
-            session = Session()
-            session.verify = self.verify or get_default_verify_paths().capath
-            session.cookies = CookieManager.get_jar()
-
             if self.ssh_key is not None:
-                session.auth = HttpSignatureAuth(username=self.username, password=self.password,
-                                                 ssh_key_file=self.ssh_key)
+                auth = HttpSignatureAuth(username=self.username, password=self.password,
+                                         ssh_key_file=self.ssh_key)
             else:
-                session.auth = HTTPBasicAuth(self.username, self.password)
-
-            setattr(THREAD_LOCAL, self._session_id, session)
+                auth = HTTPBasicAuth(self.username, self.password)
+            session = init_session(auth=auth, policy=self.retry_policy, verify=self.verify)
+            setattr(THREAD_LOCAL, session_id, session)
 
         return session
 
@@ -305,39 +299,14 @@ def request(self, url: str, method: str = "GET", stream: bool = False,
         if timeout:
             settings["timeout"] = timeout
 
-        logger = logging.getLogger("osctiny.request")
-
-        for i in range(self.default_connection_retries, -1, -1):
-            logger.info("Requested URL: %s", prepped_req.url)
-            logger.debug("Sent data:\n%s\n---",
-                         "\n".join(f"{k}: {v}" for k, v in req.data.items())
-                         if isinstance(req.data, dict) else req.data)
-            logger.debug("Sent parameters:\n%s\n---",
-                         "\n".join(f"{k}: {v}" for k, v in (
-                             req.params
-                             if isinstance(req.params, dict)
-                             else parse_qs(req.params, keep_blank_values=True)
-                         ).items()))
-            try:
-                response = self.session.send(prepped_req, **settings)
-            except _ConnectionError as error:
-                warnings.warn("Problem connecting to server: {}".format(error))
-                log_method = logger.error if i < 1 else logger.warning
-                log_method("Request failed: %s", error)
-                if i < 1:
-                    raise
-                logger.debug("Retrying request in %d seconds", self.default_retry_timeout)
-                time.sleep(self.default_retry_timeout)
-            else:
-                logger.info("Server replied with status %d", response.status_code)
-                logger.debug("Response headers:\n%s\n---",
-                             "\n".join(f"{k}: {v}" for k, v in response.headers.items()))
-                if not stream:
-                    # The response content must not be accessed when streaming
-                    logger.debug("Response content:\n%s\n---", response.text)
-                if raise_for_status:
-                    response.raise_for_status()
-                return response
+        try:
+            response = self.session.send(prepped_req, **settings)
+        except _ConnectionError as error:
+            warnings.warn("Problem connecting to server: {}".format(error))
+        else:
+            if raise_for_status:
+                response.raise_for_status()
+            return response
 
         return None
 

osctiny/tests/base.py

@@ -61,8 +61,7 @@ def setUpClass(cls):
             username="foobar",
             password="helloworld",
         )
-        cls.osc.default_connection_retries = 0
-        cls.osc.default_retry_timeout = 0
+        cls.osc.retry_policy = None
 
     @staticmethod
     def mock_request(**kwargs):

osctiny/tests/test_utils.py

@@ -27,6 +27,7 @@
 from ..utils.cookies import CookieManager
 from ..utils.mapping import Mappable
 from ..utils.errors import get_http_error_details
+from ..utils.session import generate_session_id
 
 sys.path.append(os.path.dirname(__file__))
 
@@ -405,8 +406,7 @@ def test_get_credentials(self, *_):
 class TestAuth(TestCase):
     def _clear_thread_local(self):
         try:
-            delattr(THREAD_LOCAL, self.osc._session_id)
-
+            delattr(THREAD_LOCAL, generate_session_id(username=self.osc.username, url=self.osc.url))
         except AttributeError:
             pass
 
@@ -499,6 +499,7 @@ def test_handle_401(self, *_):
 
 class TestError(TestCase):
     url = "http://example.com"
+
     @property
     def osc(self) -> Osc:
         return Osc(url=self.url, username="nemo", password="password")
@@ -507,9 +508,9 @@ def osc(self) -> Osc:
     def test_get_http_error_details(self):
         status = 400
         summary = "Bla Bla Bla"
-        responses.add(
+        rsp_mock = responses.add(
             responses.GET,
-            "http://example.com",
+            self.url,
             body=f"""<status code="foo"><summary>{summary}</summary></status>""",
             status=status
         )
@@ -528,12 +529,17 @@ def test_get_http_error_details(self):
             else:
                 self.fail("No exception was raised")
 
+        with self.subTest("Request count"):
+            if rsp_mock is not None:
+                # In the last version of `responses` supporting Python 3.6, `rsp_mock` is None
+                self.assertEqual(1, rsp_mock.call_count)
+
     @responses.activate
     def test_get_http_error_details__bad_response(self):
         status = 502
-        responses.add(
+        rsp_mock = responses.add(
             responses.GET,
-            "http://example.com",
+            self.url,
             body=f"""Bad Gateway HTML message""",
             status=status
         )
@@ -558,6 +564,11 @@ def test_get_http_error_details__bad_response(self):
             else:
                 self.fail("No exception was raised")
 
+        with self.subTest("Request count"):
+            if rsp_mock is not None:
+                # In the last version of `responses` supporting Python 3.6, `rsp_mock` is None
+                self.assertEqual(self.osc.retry_policy.max_attempts + 1, rsp_mock.call_count)
+
 
 @mock.patch("osctiny.utils.cookies._conf", new=None)
 class TestCookies(TestCase):

osctiny/utils/session.py

@@ -0,0 +1,85 @@
+"""
+HTTP Session
+^^^^^^^^^^^^
+
+.. versionadded:: {{ NEXT_RELEASE }}
+"""
+from base64 import b64encode
+import os
+from ssl import get_default_verify_paths
+import threading
+import typing
+
+from requests import Session
+from requests.adapters import HTTPAdapter
+from requests.auth import AuthBase
+import urllib3
+
+from .cookies import CookieManager
+
+
+class RetryPolicy(typing.NamedTuple):
+    """
+    Parameters for governing request retries
+    """
+    max_attempts: int = 6
+    backoff_factor: float = 0.125
+    backoff_max: float = 5.
+
+
+def generate_session_id(username: str, url: str) -> str:
+    """
+    Generate a session ID unique to the user, remote host, process and thread.
+    """
+    session_hash = b64encode(f'{username}@{url}'.encode()).decode()
+    return f"session_{session_hash}_{os.getpid()}_{threading.get_ident()}"
+
+
+def generate_retry_policy(policy: RetryPolicy) -> urllib3.Retry:
+    """
+    Generate retry policy for HTTP requests
+    """
+    version = tuple(int(x) for x in urllib3.__version__.split("."))
+    kwargs = {
+        "connect": policy.max_attempts,
+        "read": policy.max_attempts,
+        "status": policy.max_attempts,
+        "status_forcelist": [500, 502, 503, 504],
+        "redirect": False,        # Don't limit redirects.
+        "backoff_factor": policy.backoff_factor,
+        "raise_on_status": False
+    }
+
+    # Older versions of Retry do not understand some parameters:
+    if version < (1, 26, 0):
+        kwargs.update({
+            "method_whitelist": None,
+        })
+    else:
+        new_kwargs = {
+            "allowed_methods": None,  # Aka. allow all!
+            "other": 0,  # Fail on all other errors.
+        }
+        if version >= (2, 0, 0):
+            new_kwargs["backoff_max"] = policy.backoff_max
+        kwargs.update(new_kwargs)
+
+    return urllib3.Retry(**kwargs)
+
+
+def init_session(auth: AuthBase, policy: typing.Optional[RetryPolicy] = None,
+                 verify: typing.Union[str, bool] = get_default_verify_paths().capath) -> Session:
+    """
+    Factory to initialize a session object.
+    """
+    session = Session()
+    session.auth = auth
+    session.cookies = CookieManager.get_jar()
+    session.verify = verify
+
+    if policy:
+        retries = generate_retry_policy(policy=policy)
+        for proto in ('http://', 'https://'):
+            session.mount(proto, HTTPAdapter(max_retries=retries))
+
+    return session

requirements.txt

@@ -3,3 +3,4 @@ requests
 python-dateutil
 pytz
 pyyaml
+urllib3