Implementing HTTP retries for the SDK (#247)

* Implementing HTTP retries for the SDK

* Adding test case for credential

* Updated changelog

Author: hiranya911

CHANGELOG.md

@@ -1,6 +1,8 @@
 # Unreleased
 
--
+- [added] Implemented HTTP retries. The SDK now retries HTTP calls on
+  low-level connection and socket read errors, as well as HTTP 500 and
+  503 errors.
 
 # v2.15.0
 

firebase_admin/_http_client.py

@@ -19,16 +19,28 @@
 
 from google.auth import transport
 import requests
+from requests.packages.urllib3.util import retry # pylint: disable=import-error
+
+
+# Default retry configuration: Retries once on low-level connection and socket read errors.
+# Retries up to 4 times on HTTP 500 and 503 errors, with exponential backoff. Returns the
+# last response upon exhausting all retries.
+DEFAULT_RETRY_CONFIG = retry.Retry(
+    connect=1, read=1, status=4, status_forcelist=[500, 503],
+    raise_on_status=False, backoff_factor=0.5)
 
 
 class HttpClient(object):
     """Base HTTP client used to make HTTP calls.
 
-    HttpClient maintains an HTTP session, and handles request authentication if necessary.
+    HttpClient maintains an HTTP session, and handles request authentication and retries if
+    necessary.
     """
 
-    def __init__(self, credential=None, session=None, base_url='', headers=None):
-        """Cretes a new HttpClient instance from the provided arguments.
+    def __init__(
+            self, credential=None, session=None, base_url='', headers=None,
+            retries=DEFAULT_RETRY_CONFIG):
+        """Creates a new HttpClient instance from the provided arguments.
 
         If a credential is provided, initializes a new HTTP session authorized with it. If neither
         a credential nor a session is provided, initializes a new unauthorized session.
@@ -38,6 +50,9 @@ def __init__(self, credential=None, session=None, base_url='', headers=None):
           session: A custom HTTP session (optional).
           base_url: A URL prefix to be added to all outgoing requests (optional).
           headers: A map of headers to be added to all outgoing requests (optional).
+          retries: A urllib retry configuration. Default settings would retry once for low-level
+              connection and socket read errors, and up to 4 times for HTTP 500 and 503 errors.
+              Pass a False value to disable retries (optional).
         """
         if credential:
             self._session = transport.requests.AuthorizedSession(credential)
@@ -48,6 +63,9 @@ def __init__(self, credential=None, session=None, base_url='', headers=None):
 
         if headers:
             self._session.headers.update(headers)
+        if retries:
+            self._session.mount('http://', requests.adapters.HTTPAdapter(max_retries=retries))
+            self._session.mount('https://', requests.adapters.HTTPAdapter(max_retries=retries))
         self._base_url = base_url
 
     @property

tests/test_http_client.py

@@ -13,12 +13,17 @@
 # limitations under the License.
 
 """Tests for firebase_admin._http_client."""
+import pytest
+from pytest_localserver import plugin
 import requests
 
 from firebase_admin import _http_client
 from tests import testutils
 
 
+# Fixture for mocking a HTTP server
+httpserver = plugin.httpserver
+
 _TEST_URL = 'http://firebase.test.url/'
 
 
@@ -59,8 +64,57 @@ def test_base_url():
     assert recorder[0].method == 'GET'
     assert recorder[0].url == _TEST_URL + 'foo'
 
+def test_credential():
+    client = _http_client.HttpClient(
+        credential=testutils.MockGoogleCredential())
+    assert client.session is not None
+    recorder = _instrument(client, 'body')
+    resp = client.request('get', _TEST_URL)
+    assert resp.status_code == 200
+    assert resp.text == 'body'
+    assert len(recorder) == 1
+    assert recorder[0].method == 'GET'
+    assert recorder[0].url == _TEST_URL
+    assert recorder[0].headers['Authorization'] == 'Bearer mock-token'
+
 def _instrument(client, payload, status=200):
     recorder = []
     adapter = testutils.MockAdapter(payload, status, recorder)
     client.session.mount(_TEST_URL, adapter)
     return recorder
+
+
+class TestHttpRetry(object):
+    """Unit tests for the default HTTP retry configuration."""
+
+    @classmethod
+    def setup_class(cls):
+        # Turn off exponential backoff for faster execution
+        _http_client.DEFAULT_RETRY_CONFIG.backoff_factor = 0
+
+    def test_retry_on_503(self, httpserver):
+        httpserver.serve_content({}, 503)
+        client = _http_client.JsonHttpClient(
+            credential=testutils.MockGoogleCredential(), base_url=httpserver.url)
+        with pytest.raises(requests.exceptions.HTTPError) as excinfo:
+            client.request('get', '/')
+        assert excinfo.value.response.status_code == 503
+        assert len(httpserver.requests) == 5
+
+    def test_retry_on_500(self, httpserver):
+        httpserver.serve_content({}, 500)
+        client = _http_client.JsonHttpClient(
+            credential=testutils.MockGoogleCredential(), base_url=httpserver.url)
+        with pytest.raises(requests.exceptions.HTTPError) as excinfo:
+            client.request('get', '/')
+        assert excinfo.value.response.status_code == 500
+        assert len(httpserver.requests) == 5
+
+    def test_no_retry_on_404(self, httpserver):
+        httpserver.serve_content({}, 404)
+        client = _http_client.JsonHttpClient(
+            credential=testutils.MockGoogleCredential(), base_url=httpserver.url)
+        with pytest.raises(requests.exceptions.HTTPError) as excinfo:
+            client.request('get', '/')
+        assert excinfo.value.response.status_code == 404
+        assert len(httpserver.requests) == 1