Retry logic change proposal (#73)

Author: fclairamb

gql/client.py

@@ -25,7 +25,7 @@ def __init__(
         type_def=None,
         transport=None,
         fetch_schema_from_transport=False,
-        retries=0,
+        retries=0,  # We should remove this parameter and let the transport level handle it
     ):
         assert not (
             type_def and introspection
@@ -93,3 +93,13 @@ def _get_result(self, document, *args, **kwargs):
                 retries_count += 1
 
         raise RetryError(retries_count, last_exception)
+
+    def close(self):
+        """Close the client and it's underlying transport"""
+        self.transport.close()
+
+    def __enter__(self):
+        return self
+
+    def __exit__(self, *args):
+        self.close()

gql/transport/__init__.py

@@ -20,3 +20,12 @@ def execute(self, document):
         raise NotImplementedError(
             "Any Transport subclass must implement execute method"
         )
+
+    def close(self):
+        """Close the transport
+
+        This method doesn't have to be implemented unless the transport would benefit from it.
+        This is currently used by the RequestsHTTPTransport transport to close the session's
+        connection pool.
+        """
+        pass

gql/transport/requests.py

@@ -6,6 +6,7 @@
 from graphql.execution import ExecutionResult
 from graphql.language.ast import Document
 from graphql.language.printer import print_ast
+from requests.adapters import HTTPAdapter, Retry
 from requests.auth import AuthBase
 from requests.cookies import RequestsCookieJar
 
@@ -27,6 +28,7 @@ def __init__(
         use_json=False,  # type: bool
         timeout=None,  # type: int
         verify=True,  # type: bool
+        retries=0,  # type: int
         **kwargs  # type: Any
     ):
         """Initialize the transport with the given request parameters.
@@ -40,6 +42,7 @@ def __init__(
         :param verify: Either a boolean, in which case it controls whether we verify
             the server's TLS certificate, or a string, in which case it must be a path
             to a CA bundle to use. (Default: True).
+        :param retries: Pre-setup of the requests' Session for performing retries
         :param kwargs: Optional arguments that ``request`` takes. These can be seen at the :requests_: source code
             or the official :docs_:
 
@@ -55,6 +58,21 @@ def __init__(
         self.verify = verify
         self.kwargs = kwargs
 
+        # Creating a session that can later be re-use to configure custom mechanisms
+        self.session = requests.Session()
+
+        # If we specified some retries, we provide a predefined retry-logic
+        if retries > 0:
+            adapter = HTTPAdapter(
+                max_retries=Retry(
+                    total=retries,
+                    backoff_factor=0.1,
+                    status_forcelist=[500, 502, 503, 504],
+                )
+            )
+            for prefix in "http://", "https://":
+                self.session.mount(prefix, adapter)
+
     def execute(self, document, variable_values=None, timeout=None):
         # type: (Document, Dict, int) -> ExecutionResult
         """Execute the provided document AST against the configured remote server.
@@ -82,7 +100,8 @@ def execute(self, document, variable_values=None, timeout=None):
         # Pass kwargs to requests post method
         post_args.update(self.kwargs)
 
-        response = requests.post(self.url, **post_args)  # type: ignore
+        # Using the created session to perform requests
+        response = self.session.post(self.url, **post_args)  # type: ignore
         try:
             result = response.json()
             if not isinstance(result, dict):
@@ -96,3 +115,7 @@ def execute(self, document, variable_values=None, timeout=None):
                 "Server did not return a GraphQL result", response=response
             )
         return ExecutionResult(errors=result.get("errors"), data=result.get("data"))
+
+    def close(self):
+        """Closing the transport by closing the inner session"""
+        self.session.close()

tests/test_client.py

@@ -2,6 +2,8 @@
 
 import mock
 import pytest
+from urllib3.exceptions import NewConnectionError
+
 from graphql import build_ast_schema, parse
 
 from gql import Client, gql
@@ -56,10 +58,46 @@ def test_retries(execute_mock):
 
     with pytest.raises(Exception):
         client.execute(query)
-
+    client.close()
     assert execute_mock.call_count == expected_retries
 
 
+@mock.patch("urllib3.connection.HTTPConnection._new_conn")
+def test_retries_on_transport(execute_mock):
+    """Testing retries on the transport level
+
+    This forces us to override low-level APIs because the retry mechanism on the urllib3 (which
+    uses requests) is pretty low-level itself.
+    """
+    expected_retries = 3
+    execute_mock.side_effect = NewConnectionError(
+        "Should be HTTPConnection", "Fake connection error"
+    )
+    transport = RequestsHTTPTransport(
+        url="http://localhost:9999", retries=expected_retries,
+    )
+    client = Client(transport=transport)
+
+    query = gql(
+        """
+    {
+      myFavoriteFilm: film(id:"RmlsbToz") {
+        id
+        title
+        episodeId
+      }
+    }
+    """
+    )
+    with client:  # We're using the client as context manager
+        with pytest.raises(Exception):
+            client.execute(query)
+
+    # This might look strange compared to the previous test, but making 3 retries
+    # means you're actually doing 4 calls.
+    assert execute_mock.call_count == expected_retries + 1
+
+
 def test_no_schema_exception():
     with pytest.raises(Exception) as exc_info:
         client = Client()
@@ -95,6 +133,7 @@ def test_execute_result_error():
 
     with pytest.raises(Exception) as exc_info:
         client.execute(failing_query)
+    client.close()
     assert 'Cannot query field "id" on type "Continent".' in str(exc_info.value)
 
 
@@ -108,6 +147,7 @@ def test_http_transport_raise_for_status_error(http_transport_query):
 
     with pytest.raises(Exception) as exc_info:
         client.execute(http_transport_query)
+    client.close()
     assert "400 Client Error: Bad Request for url" in str(exc_info.value)
 
 
@@ -122,6 +162,7 @@ def test_http_transport_verify_error(http_transport_query):
     )
     with pytest.warns(Warning) as record:
         client.execute(http_transport_query)
+    client.close()  # We could have written `with client:` on top of the `with pytest...` instead
     assert len(record) == 1
     assert "Unverified HTTPS request is being made to host" in str(record[0].message)
 
@@ -150,4 +191,5 @@ def test_gql():
     """
     )
     result = client.execute(query)
+    client.close()
     assert result["user"] is None