Repository: google/google-api-python-client
Pull request #320 (2016-12-26 08:04:12)

Set default timeout for all the HTTP requests
This will avoid having hanging connections when using Google API calls.

In case the user have set default socket timeout, that value is used instead of default.

googlebot: Thanks for your pull request.  It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: **Please visit <https://cla.developers.google.com/> to sign.**

Once you've signed, please reply here (e.g. `I signed it!`) and we'll verify.  Thanks.

---

- If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address.  Check [your existing CLA data](https://cla.developers.google.com/clas) and verify that your [email is set on your git commits](https://help.github.com/articles/setting-your-email-in-git/).
- If you signed the CLA as a corporation, please let us know the company's name.

<!-- need_sender_cla -->
i-maravic: I signed it!
i-maravic: @fabriziodemaria
i-maravic: @jonparrott Could you take a look at this PR please?
jonparrott: 5 minutes seems way too high. App Engine will terminate requests after 60 seconds.
i-maravic: @jonparrott I wanted to be conservative.

I've changed it to 60s now.
jonparrott: @i-maravic I guess my concern here is that I don't like the idea of using any library-specific timeout that the user doesn't explicitly set. It's surprising behavior. What prompted this change? Can I get some more context to better understand what problem this is trying to solve?
i-maravic: @jonparrott We're using google-api-python-client when running periodic (hourly or daily) batch jobs for uploading to BigQuery and reading/writing to Google Cloud Storage. All the periodic jobs are being executed in Docker containers.

Recently we've experienced issues with those jobs hanging forever without making any progress. After doing some investigation we detected that the code making requests to Google APIs (aka. google-api-python-client) is hanging forever while waiting something to read from TCP socket. This was surprising behaviour for us.

After doing even more digging we discovered that Docker had some weird networking issues, which were causing this in the first place. We're not 100% sure how to reproduce Docker issue though, but restarting the Docker service mitigated a lot of problems we're experiencing.

I fell that library doing HTTP requests should set a sane default timeout for doing HTTP requests, because we could easily avoid issues like this in the future. I would argue that this is what most of the users would like to have any how.

I would also like to point out that you already have code in your lib, which is dealing with Timeout exceptions that might be thrown from the HTTP requests and do the retries accordingly.

I know that you're not using _requests_ library, but there is very nice [explanation](http://docs.python-requests.org/en/master/user/quickstart/#timeouts) why all production code using it should set timeouts for all the HTTP requests it's doing.
jonparrott: @i-maravic I see, okay.

I need the CLA bot to verify your signature status before I can merge.
i-maravic: I work for Spotify. Spotify should have signed the CLA.
jonparrott: @i-maravic gotcha.

@lukesneeringer @landrito I'd appreciate if one of you could chime in and let me know your thoughts before I merge this.
lukesneeringer: A 60 second default timeout seems reasonable to me, and it is easy enough to override. LGTM.
jonparrott: @lukesneeringer thanks for chiming in!

@i-maravic okay, so it looks like this is going to happen, can I ask two things of you:

1. Is it possible to add tests or assertions to existing tests to cover this?
2. This timeout only applies to the http object used to fetch discovery documents. Do you want to apply the timeout to the http object used to make actual API requests as well?
i-maravic: @jonparrott Sounds like reasonable asks.

Could you point me out where is the http object for actual API requests constructed?

jonparrott: @i-maravic just below in `build_from_document`: https://github.com/google/google-api-python-client/pull/320/files#diff-bceb3b293cf7c789403d137d70ef8dd3R370
i-maravic: @jonparrott Now the default timeout is applied to all discovery and API requests (auth or not)

Hopefully I didn't miss and httplib2.Http() instantiation in the lib
jonparrott: @i-maravic this looks good, one last question:

Do you want to pass in `timeout` to `authorized_http` [here](https://github.com/google/google-api-python-client/pull/320/files#diff-bceb3b293cf7c789403d137d70ef8dd3R369)? Otherwise, the authorized http instance won't have the timeout.
i-maravic: @jonparrott `authorized_http` gets timeout set [here](https://github.com/google/google-api-python-client/pull/320/files#diff-39e7a8494b350ee804d3775fae23de87R88)
jonparrott: @i-maravic gotcha. Alright, this LGTM. Thanks!

Patch:
diff --git a/describe.py b/describe.py
index 6d577defa..e3881aec4 100755
--- a/describe.py
+++ b/describe.py
@@ -34,7 +34,7 @@ from googleapiclient.discovery import DISCOVERY_URI
 from googleapiclient.discovery import build
 from googleapiclient.discovery import build_from_document
 from googleapiclient.discovery import UnknownApiNameOrVersion
-import httplib2
+from googleapiclient.http import build_http
 import uritemplate
 
 CSS = """<style>
@@ -346,6 +346,7 @@ def document_api(name, version):
     print 'Warning: {} {} found but could not be built.'.format(name, version)
     return
 
+  http = build_http()
   response, content = http.request(
       uritemplate.expand(
           FLAGS.discovery_uri_template, {
@@ -366,7 +367,7 @@ def document_api_from_discovery_document(uri):
   Args:
     uri: string, URI of discovery document.
   """
-  http = httplib2.Http()
+  http = build_http()
   response, content = http.request(FLAGS.discovery_uri)
   discovery = json.loads(content)
 
@@ -384,7 +385,7 @@ if __name__ == '__main__':
   if FLAGS.discovery_uri:
     document_api_from_discovery_document(FLAGS.discovery_uri)
   else:
-    http = httplib2.Http()
+    http = build_http()
     resp, content = http.request(
         FLAGS.directory_uri,
         headers={'X-User-IP': '0.0.0.0'})
diff --git a/googleapiclient/_auth.py b/googleapiclient/_auth.py
index 044ed12da..87d370951 100644
--- a/googleapiclient/_auth.py
+++ b/googleapiclient/_auth.py
@@ -14,8 +14,6 @@
 
 """Helpers for authentication using oauth2client or google-auth."""
 
-import httplib2
-
 try:
     import google.auth
     import google.auth.credentials
@@ -31,6 +29,8 @@ try:
 except ImportError:  # pragma: NO COVER
     HAS_OAUTH2CLIENT = False
 
+from googleapiclient.http import build_http
+
 
 def default_credentials():
     """Returns Application Default Credentials."""
@@ -86,6 +86,7 @@ def authorized_http(credentials):
     """
     if HAS_GOOGLE_AUTH and isinstance(
             credentials, google.auth.credentials.Credentials):
-        return google_auth_httplib2.AuthorizedHttp(credentials)
+        return google_auth_httplib2.AuthorizedHttp(credentials,
+                                                   http=build_http())
     else:
-        return credentials.authorize(httplib2.Http())
+        return credentials.authorize(build_http())
diff --git a/googleapiclient/discovery.py b/googleapiclient/discovery.py
index 74f0a09e6..6291f34e8 100644
--- a/googleapiclient/discovery.py
+++ b/googleapiclient/discovery.py
@@ -61,6 +61,7 @@ from googleapiclient.errors import MediaUploadSizeError
 from googleapiclient.errors import UnacceptableMimeTypeError
 from googleapiclient.errors import UnknownApiNameOrVersion
 from googleapiclient.errors import UnknownFileType
+from googleapiclient.http import build_http
 from googleapiclient.http import BatchHttpRequest
 from googleapiclient.http import HttpMock
 from googleapiclient.http import HttpMockSequence
@@ -97,6 +98,7 @@ V2_DISCOVERY_URI = ('https://{api}.googleapis.com/$discovery/rest?'
                     'version={apiVersion}')
 DEFAULT_METHOD_DOC = 'A description of how to use this function'
 HTTP_PAYLOAD_METHODS = frozenset(['PUT', 'POST', 'PATCH'])
+
 _MEDIA_SIZE_BIT_SHIFTS = {'KB': 10, 'MB': 20, 'GB': 30, 'TB': 40}
 BODY_PARAMETER_DEFAULT_VALUE = {
     'description': 'The request body.',
@@ -213,7 +215,10 @@ def build(serviceName,
       'apiVersion': version
       }
 
-  discovery_http = http if http is not None else httplib2.Http()
+  if http is None:
+    discovery_http = build_http()
+  else:
+    discovery_http = http
 
   for discovery_url in (discoveryServiceUrl, V2_DISCOVERY_URI,):
     requested_url = uritemplate.expand(discovery_url, params)
@@ -366,7 +371,7 @@ def build_from_document(
     # If the service doesn't require scopes then there is no need for
     # authentication.
     else:
-      http = httplib2.Http()
+      http = build_http()
 
   if model is None:
     features = service.get('features', [])
diff --git a/googleapiclient/http.py b/googleapiclient/http.py
index 0ef10b955..aece93339 100644
--- a/googleapiclient/http.py
+++ b/googleapiclient/http.py
@@ -80,6 +80,8 @@ MAX_URI_LENGTH = 2048
 
 _TOO_MANY_REQUESTS = 429
 
+DEFAULT_HTTP_TIMEOUT_SEC = 60
+
 
 def _should_retry_response(resp_status, content):
   """Determines whether a response should be retried.
@@ -1732,3 +1734,21 @@ def tunnel_patch(http):
 
   http.request = new_request
   return http
+
+
+def build_http():
+  """Builds httplib2.Http object
+
+  Returns:
+  A httplib2.Http object, which is used to make http requests, and which has timeout set by default.
+  To override default timeout call
+
+    socket.setdefaulttimeout(timeout_in_sec)
+
+  before interacting with this method.
+  """
+  if socket.getdefaulttimeout() is not None:
+    http_timeout = socket.getdefaulttimeout()
+  else:
+    http_timeout = DEFAULT_HTTP_TIMEOUT_SEC
+  return httplib2.Http(timeout=http_timeout)
diff --git a/googleapiclient/sample_tools.py b/googleapiclient/sample_tools.py
index 2b4e7b45d..5ed632dab 100644
--- a/googleapiclient/sample_tools.py
+++ b/googleapiclient/sample_tools.py
@@ -23,10 +23,10 @@ __all__ = ['init']
 
 
 import argparse
-import httplib2
 import os
 
 from googleapiclient import discovery
+from googleapiclient.http import build_http
 from oauth2client import client
 from oauth2client import file
 from oauth2client import tools
@@ -88,7 +88,7 @@ def init(argv, name, version, doc, filename, scope=None, parents=[], discovery_f
   credentials = storage.get()
   if credentials is None or credentials.invalid:
     credentials = tools.run_flow(flow, storage, flags)
-  http = credentials.authorize(http = httplib2.Http())
+  http = credentials.authorize(http=build_http())
 
   if discovery_filename is None:
     # Construct a service object via the discovery service.


Test patch:
diff --git a/tests/test__auth.py b/tests/test__auth.py
index 6711ffe6a..68e7aae6b 100644
--- a/tests/test__auth.py
+++ b/tests/test__auth.py
@@ -66,10 +66,13 @@ class TestAuthWithGoogleAuth(unittest2.TestCase):
     def test_authorized_http(self):
         credentials = mock.Mock(spec=google.auth.credentials.Credentials)
 
-        http = _auth.authorized_http(credentials)
+        authorized_http = _auth.authorized_http(credentials)
 
-        self.assertIsInstance(http, google_auth_httplib2.AuthorizedHttp)
-        self.assertEqual(http.credentials, credentials)
+        self.assertIsInstance(authorized_http, google_auth_httplib2.AuthorizedHttp)
+        self.assertEqual(authorized_http.credentials, credentials)
+        self.assertIsInstance(authorized_http.http, httplib2.Http)
+        self.assertIsInstance(authorized_http.http.timeout, int)
+        self.assertGreater(authorized_http.http.timeout, 0)
 
 
 class TestAuthWithOAuth2Client(unittest2.TestCase):
@@ -112,11 +115,14 @@ class TestAuthWithOAuth2Client(unittest2.TestCase):
     def test_authorized_http(self):
         credentials = mock.Mock(spec=oauth2client.client.Credentials)
 
-        http = _auth.authorized_http(credentials)
+        authorized_http = _auth.authorized_http(credentials)
 
-        self.assertEqual(http, credentials.authorize.return_value)
-        self.assertIsInstance(
-            credentials.authorize.call_args[0][0], httplib2.Http)
+        http = credentials.authorize.call_args[0][0]
+
+        self.assertEqual(authorized_http, credentials.authorize.return_value)
+        self.assertIsInstance(http, httplib2.Http)
+        self.assertIsInstance(http.timeout, int)
+        self.assertGreater(http.timeout, 0)
 
 
 class TestAuthWithoutAuth(unittest2.TestCase):
diff --git a/tests/test_discovery.py b/tests/test_discovery.py
index 3e26db941..47adeb794 100644
--- a/tests/test_discovery.py
+++ b/tests/test_discovery.py
@@ -65,6 +65,7 @@ from googleapiclient.errors import ResumableUploadError
 from googleapiclient.errors import UnacceptableMimeTypeError
 from googleapiclient.errors import UnknownApiNameOrVersion
 from googleapiclient.errors import UnknownFileType
+from googleapiclient.http import build_http
 from googleapiclient.http import BatchHttpRequest
 from googleapiclient.http import HttpMock
 from googleapiclient.http import HttpMockSequence
@@ -402,13 +403,32 @@ class DiscoveryFromDocument(unittest.TestCase):
       discovery, base=base, credentials=self.MOCK_CREDENTIALS)
     self.assertEquals("https://www.googleapis.com/plus/v1/", plus._baseUrl)
 
-  def test_building_with_optional_http(self):
+  def test_building_with_optional_http_with_authorization(self):
     discovery = open(datafile('plus.json')).read()
     plus = build_from_document(
       discovery, base="https://www.googleapis.com/",
       credentials=self.MOCK_CREDENTIALS)
-    self.assertIsInstance(
-      plus._http, (httplib2.Http, google_auth_httplib2.AuthorizedHttp))
+
+    # plus service requires Authorization, hence we expect to see AuthorizedHttp object here
+    self.assertIsInstance(plus._http, google_auth_httplib2.AuthorizedHttp)
+    self.assertIsInstance(plus._http.http, httplib2.Http)
+    self.assertIsInstance(plus._http.http.timeout, int)
+    self.assertGreater(plus._http.http.timeout, 0)
+
+  def test_building_with_optional_http_with_no_authorization(self):
+    discovery = open(datafile('plus.json')).read()
+    # Cleanup auth field, so we would use plain http client
+    discovery = json.loads(discovery)
+    discovery['auth'] = {}
+    discovery = json.dumps(discovery)
+
+    plus = build_from_document(
+      discovery, base="https://www.googleapis.com/",
+      credentials=self.MOCK_CREDENTIALS)
+    # plus service requires Authorization
+    self.assertIsInstance(plus._http, httplib2.Http)
+    self.assertIsInstance(plus._http.timeout, int)
+    self.assertGreater(plus._http.timeout, 0)
 
   def test_building_with_explicit_http(self):
     http = HttpMock()
@@ -1316,7 +1336,7 @@ class Discovery(unittest.TestCase):
         zoo_uri = util._add_query_parameter(zoo_uri, 'userIp',
                                             os.environ['REMOTE_ADDR'])
 
-    http = httplib2.Http()
+    http = build_http()
     original_request = http.request
     def wrapped_request(uri, method='GET', *args, **kwargs):
         if uri == zoo_uri:
diff --git a/tests/test_http.py b/tests/test_http.py
index 36b43bf16..512e8e17b 100644
--- a/tests/test_http.py
+++ b/tests/test_http.py
@@ -43,6 +43,7 @@ from googleapiclient.discovery import build
 from googleapiclient.errors import BatchError
 from googleapiclient.errors import HttpError
 from googleapiclient.errors import InvalidChunkSizeError
+from googleapiclient.http import build_http
 from googleapiclient.http import BatchHttpRequest
 from googleapiclient.http import HttpMock
 from googleapiclient.http import HttpMockSequence
@@ -235,7 +236,7 @@ class TestMediaUpload(unittest.TestCase):
     def _postproc(*kwargs):
       pass
 
-    http = httplib2.Http()
+    http = build_http()
     media_upload = MediaFileUpload(
         datafile('small.png'), chunksize=500, resumable=True)
     req = HttpRequest(
@@ -1341,6 +1342,34 @@ class TestHttpMock(unittest.TestCase):
     self.assertRaises(HttpError, request.execute)
 
 
+class TestHttpBuild(unittest.TestCase):
+  original_socket_default_timeout = None
+
+  @classmethod
+  def setUpClass(cls):
+    cls.original_socket_default_timeout = socket.getdefaulttimeout()
+
+  @classmethod
+  def tearDownClass(cls):
+    socket.setdefaulttimeout(cls.original_socket_default_timeout)
+
+  def test_build_http_sets_default_timeout_if_none_specified(self):
+    socket.setdefaulttimeout(None)
+    http = build_http()
+    self.assertIsInstance(http.timeout, int)
+    self.assertGreater(http.timeout, 0)
+
+  def test_build_http_default_timeout_can_be_overridden(self):
+    socket.setdefaulttimeout(1.5)
+    http = build_http()
+    self.assertAlmostEqual(http.timeout, 1.5, delta=0.001)
+
+  def test_build_http_default_timeout_can_be_set_to_zero(self):
+    socket.setdefaulttimeout(0)
+    http = build_http()
+    self.assertEquals(http.timeout, 0)
+
+
 if __name__ == '__main__':
   logging.getLogger().setLevel(logging.ERROR)
   unittest.main()