fix: Setting a default timeout on all HTTP connections (#397)

* Setting a default timeout on all HTTP connections

* Refactored and cleaned up tests

* Further cleaning up the tests

* Cleaning up tests based on feedback

Author: hiranya911

firebase_admin/__init__.py

@@ -49,8 +49,8 @@ def initialize_app(credential=None, options=None, name=_DEFAULT_APP_NAME):
           Google Application Default Credentials are used.
       options: A dictionary of configuration options (optional). Supported options include
           ``databaseURL``, ``storageBucket``, ``projectId``, ``databaseAuthVariableOverride``,
-          ``serviceAccountId`` and ``httpTimeout``. If ``httpTimeout`` is not set, HTTP
-          connections initiated by client modules such as ``db`` will not time out.
+          ``serviceAccountId`` and ``httpTimeout``. If ``httpTimeout`` is not set, the SDK
+          uses a default timeout of 120 seconds.
       name: Name of the app (optional).
     Returns:
       App: A newly initialized instance of App.

firebase_admin/_http_client.py

@@ -32,6 +32,9 @@
     raise_on_status=False, backoff_factor=0.5)
 
 
+DEFAULT_TIMEOUT_SECONDS = 120
+
+
 class HttpClient:
     """Base HTTP client used to make HTTP calls.
 
@@ -41,7 +44,7 @@ class HttpClient:
 
     def __init__(
             self, credential=None, session=None, base_url='', headers=None,
-            retries=DEFAULT_RETRY_CONFIG):
+            retries=DEFAULT_RETRY_CONFIG, timeout=DEFAULT_TIMEOUT_SECONDS):
         """Creates a new HttpClient instance from the provided arguments.
 
         If a credential is provided, initializes a new HTTP session authorized with it. If neither
@@ -55,6 +58,8 @@ def __init__(
           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).
+          timeout: HTTP timeout in seconds. Defaults to 120 seconds when not specified. Set to
+              None to disable timeouts (optional).
         """
         if credential:
             self._session = transport.requests.AuthorizedSession(credential)
@@ -69,6 +74,7 @@ def __init__(
             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
+        self._timeout = timeout
 
     @property
     def session(self):
@@ -78,6 +84,10 @@ def session(self):
     def base_url(self):
         return self._base_url
 
+    @property
+    def timeout(self):
+        return self._timeout
+
     def parse_body(self, resp):
         raise NotImplementedError
 
@@ -93,15 +103,17 @@ class call this method to send HTTP requests out. Refer to
           method: HTTP method name as a string (e.g. get, post).
           url: URL of the remote endpoint.
           kwargs: An additional set of keyword arguments to be passed into the requests API
-              (e.g. json, params).
+              (e.g. json, params, timeout).
 
         Returns:
           Response: An HTTP response object.
 
         Raises:
           RequestException: Any requests exceptions encountered while making the HTTP call.
         """
-        resp = self._session.request(method, self._base_url + url, **kwargs)
+        if 'timeout' not in kwargs:
+            kwargs['timeout'] = self.timeout
+        resp = self._session.request(method, self.base_url + url, **kwargs)
         resp.raise_for_status()
         return resp
 

firebase_admin/db.py

@@ -775,7 +775,7 @@ def __init__(self, app):
             self._auth_override = json.dumps(auth_override, separators=(',', ':'))
         else:
             self._auth_override = None
-        self._timeout = app.options.get('httpTimeout')
+        self._timeout = app.options.get('httpTimeout', _http_client.DEFAULT_TIMEOUT_SECONDS)
         self._clients = {}
 
         emulator_host = os.environ.get(_EMULATOR_HOST_ENV_VAR)
@@ -900,14 +900,13 @@ def __init__(self, credential, base_url, timeout, params=None):
           credential: A Google credential that can be used to authenticate requests.
           base_url: A URL prefix to be added to all outgoing requests. This is typically the
               Firebase Realtime Database URL.
-          timeout: HTTP request timeout in seconds. If not set connections will never
+          timeout: HTTP request timeout in seconds. If set to None connections will never
               timeout, which is the default behavior of the underlying requests library.
           params: Dict of query parameters to add to all outgoing requests.
         """
-        _http_client.JsonHttpClient.__init__(
-            self, credential=credential, base_url=base_url, headers={'User-Agent': _USER_AGENT})
-        self.credential = credential
-        self.timeout = timeout
+        super().__init__(
+            credential=credential, base_url=base_url,
+            timeout=timeout, headers={'User-Agent': _USER_AGENT})
         self.params = params if params else {}
 
     def request(self, method, url, **kwargs):
@@ -937,8 +936,6 @@ def request(self, method, url, **kwargs):
                 query = extra_params
         kwargs['params'] = query
 
-        if self.timeout:
-            kwargs['timeout'] = self.timeout
         try:
             return super(_Client, self).request(method, url, **kwargs)
         except requests.exceptions.RequestException as error:

firebase_admin/messaging.py

@@ -330,8 +330,9 @@ def __init__(self, app):
             'X-GOOG-API-FORMAT-VERSION': '2',
             'X-FIREBASE-CLIENT': 'fire-admin-python/{0}'.format(firebase_admin.__version__),
         }
-        self._client = _http_client.JsonHttpClient(credential=app.credential.get_credential())
-        self._timeout = app.options.get('httpTimeout')
+        timeout = app.options.get('httpTimeout', _http_client.DEFAULT_TIMEOUT_SECONDS)
+        self._client = _http_client.JsonHttpClient(
+            credential=app.credential.get_credential(), timeout=timeout)
         self._transport = _auth.authorized_http(app.credential.get_credential())
 
     @classmethod
@@ -348,8 +349,7 @@ def send(self, message, dry_run=False):
                 'post',
                 url=self._fcm_url,
                 headers=self._fcm_headers,
-                json=data,
-                timeout=self._timeout
+                json=data
             )
         except requests.exceptions.RequestException as error:
             raise self._handle_fcm_error(error)
@@ -416,8 +416,7 @@ def make_topic_management_request(self, tokens, topic, operation):
                 'post',
                 url=url,
                 json=data,
-                headers=_MessagingService.IID_HEADERS,
-                timeout=self._timeout
+                headers=_MessagingService.IID_HEADERS
             )
         except requests.exceptions.RequestException as error:
             raise self._handle_iid_error(error)

firebase_admin/project_management.py

@@ -478,11 +478,12 @@ def __init__(self, app):
                 'the GOOGLE_CLOUD_PROJECT environment variable.')
         self._project_id = project_id
         version_header = 'Python/Admin/{0}'.format(firebase_admin.__version__)
+        timeout = app.options.get('httpTimeout', _http_client.DEFAULT_TIMEOUT_SECONDS)
         self._client = _http_client.JsonHttpClient(
             credential=app.credential.get_credential(),
             base_url=_ProjectManagementService.BASE_URL,
-            headers={'X-Client-Version': version_header})
-        self._timeout = app.options.get('httpTimeout')
+            headers={'X-Client-Version': version_header},
+            timeout=timeout)
 
     def get_android_app_metadata(self, app_id):
         return self._get_app_metadata(
@@ -658,7 +659,6 @@ def _make_request(self, method, url, json=None):
 
     def _body_and_response(self, method, url, json=None):
         try:
-            return self._client.body_and_response(
-                method=method, url=url, json=json, timeout=self._timeout)
+            return self._client.body_and_response(method=method, url=url, json=json)
         except requests.exceptions.RequestException as error:
             raise _utils.handle_platform_error_from_requests(error)

tests/test_db.py

@@ -23,6 +23,7 @@
 import firebase_admin
 from firebase_admin import db
 from firebase_admin import exceptions
+from firebase_admin import _http_client
 from firebase_admin import _sseclient
 from tests import testutils
 
@@ -731,15 +732,8 @@ def test_parse_db_url_errors(self, url, emulator_host):
     def test_valid_db_url(self, url):
         firebase_admin.initialize_app(testutils.MockCredential(), {'databaseURL' : url})
         ref = db.reference()
-        recorder = []
-        adapter = MockAdapter('{}', 200, recorder)
-        ref._client.session.mount(url, adapter)
         assert ref._client.base_url == 'https://test.firebaseio.com'
         assert 'auth_variable_override' not in ref._client.params
-        assert ref._client.timeout is None
-        assert ref.get() == {}
-        assert len(recorder) == 1
-        assert recorder[0]._extra_kwargs.get('timeout') is None
 
     @pytest.mark.parametrize('url', [
         None, '', 'foo', 'http://test.firebaseio.com', 'https://google.com',
@@ -761,15 +755,13 @@ def test_multi_db_support(self):
         ref = db.reference()
         assert ref._client.base_url == default_url
         assert 'auth_variable_override' not in ref._client.params
-        assert ref._client.timeout is None
         assert ref._client is db.reference()._client
         assert ref._client is db.reference(url=default_url)._client
 
         other_url = 'https://other.firebaseio.com'
         other_ref = db.reference(url=other_url)
         assert other_ref._client.base_url == other_url
         assert 'auth_variable_override' not in ref._client.params
-        assert other_ref._client.timeout is None
         assert other_ref._client is db.reference(url=other_url)._client
         assert other_ref._client is db.reference(url=other_url + '/')._client
 
@@ -782,7 +774,6 @@ def test_valid_auth_override(self, override):
         default_ref = db.reference()
         other_ref = db.reference(url='https://other.firebaseio.com')
         for ref in [default_ref, other_ref]:
-            assert ref._client.timeout is None
             if override == {}:
                 assert 'auth_variable_override' not in ref._client.params
             else:
@@ -804,22 +795,22 @@ def test_invalid_auth_override(self, override):
         with pytest.raises(ValueError):
             db.reference(app=other_app, url='https://other.firebaseio.com')
 
-    def test_http_timeout(self):
+    @pytest.mark.parametrize('options, timeout', [
+        ({'httpTimeout': 4}, 4),
+        ({'httpTimeout': None}, None),
+        ({}, _http_client.DEFAULT_TIMEOUT_SECONDS),
+    ])
+    def test_http_timeout(self, options, timeout):
         test_url = 'https://test.firebaseio.com'
-        firebase_admin.initialize_app(testutils.MockCredential(), {
+        all_options = {
             'databaseURL' : test_url,
-            'httpTimeout': 60
-        })
+        }
+        all_options.update(options)
+        firebase_admin.initialize_app(testutils.MockCredential(), all_options)
         default_ref = db.reference()
         other_ref = db.reference(url='https://other.firebaseio.com')
         for ref in [default_ref, other_ref]:
-            recorder = []
-            adapter = MockAdapter('{}', 200, recorder)
-            ref._client.session.mount(ref._client.base_url, adapter)
-            assert ref._client.timeout == 60
-            assert ref.get() == {}
-            assert len(recorder) == 1
-            assert recorder[0]._extra_kwargs['timeout'] == pytest.approx(60, 0.001)
+            self._check_timeout(ref, timeout)
 
     def test_app_delete(self):
         app = firebase_admin.initialize_app(
@@ -841,6 +832,18 @@ def test_user_agent_format(self):
             firebase_admin.__version__, sys.version_info.major, sys.version_info.minor)
         assert db._USER_AGENT == expected
 
+    def _check_timeout(self, ref, timeout):
+        assert ref._client.timeout == timeout
+        recorder = []
+        adapter = MockAdapter('{}', 200, recorder)
+        ref._client.session.mount(ref._client.base_url, adapter)
+        assert ref.get() == {}
+        assert len(recorder) == 1
+        if timeout is None:
+            assert recorder[0]._extra_kwargs['timeout'] is None
+        else:
+            assert recorder[0]._extra_kwargs['timeout'] == pytest.approx(timeout, 0.001)
+
 
 @pytest.fixture(params=['foo', '$key', '$value'])
 def initquery(request):

tests/test_http_client.py

@@ -74,6 +74,24 @@ def test_credential():
     assert recorder[0].url == _TEST_URL
     assert recorder[0].headers['Authorization'] == 'Bearer mock-token'
 
+@pytest.mark.parametrize('options, timeout', [
+    ({}, _http_client.DEFAULT_TIMEOUT_SECONDS),
+    ({'timeout': 7}, 7),
+    ({'timeout': 0}, 0),
+    ({'timeout': None}, None),
+])
+def test_timeout(options, timeout):
+    client = _http_client.HttpClient(**options)
+    assert client.timeout == timeout
+    recorder = _instrument(client, 'body')
+    client.request('get', _TEST_URL)
+    assert len(recorder) == 1
+    if timeout is None:
+        assert recorder[0]._extra_kwargs['timeout'] is None
+    else:
+        assert recorder[0]._extra_kwargs['timeout'] == pytest.approx(timeout, 0.001)
+
+
 def _instrument(client, payload, status=200):
     recorder = []
     adapter = testutils.MockAdapter(payload, status, recorder)

tests/test_instance_id.py

@@ -19,6 +19,7 @@
 import firebase_admin
 from firebase_admin import exceptions
 from firebase_admin import instance_id
+from firebase_admin import _http_client
 from tests import testutils
 
 
@@ -73,6 +74,12 @@ def evaluate():
                 instance_id.delete_instance_id('test')
         testutils.run_without_project_id(evaluate)
 
+    def test_default_timeout(self):
+        cred = testutils.MockCredential()
+        app = firebase_admin.initialize_app(cred, {'projectId': 'explicit-project-id'})
+        iid_service = instance_id._get_iid_service(app)
+        assert iid_service._client.timeout == _http_client.DEFAULT_TIMEOUT_SECONDS
+
     def test_delete_instance_id(self):
         cred = testutils.MockCredential()
         app = firebase_admin.initialize_app(cred, {'projectId': 'explicit-project-id'})