Repository: boto/botocore
Pull request #634 (2015-08-25 19:00:19)

Feature customizable timeout
So far Amazon SWF suggests to use 70 seconds read_timeout (which is 10 seconds longer than the service timeout 60 seconds).

There was no way to customize this. This PR provides a couple new attributes in botocore.client.Config to do that. You can now do it in both botocore and boto3 clients.

    from botocore.session import Session
    from botocore.client import Config
    import boto3


    # Both timeouts default to 60, you can customize them, seperately
    config = Config(connect_timeout=50, read_timeout=70)

    session = Session()

    # There will be a line of debug log for this
    session.set_debug_logger()

    # Botocore with default config
    swf = session.create_client('swf')

    # Botocore with customized config
    swf = session.create_client('swf', config=config)

    # Boto3 with customized config
    swf = boto3.client('swf', config=config)


jamesls: Just had some small comments.
jamesls: The `DEFAULT_TIMEOUT` is still used in two places, `_get_client_args` and the `__init__` of the Config class.  I would expected either:

* We use the values from the Config object directly in `_get_client_args`, since they have default values if they're not provided.  This will require some restructuring of the internals of `_get_client_args`.
* The `DEFAULT_TIMEOUT` is used in `_get_client_args` and has no default in `Config`.

I think I slightly prefer option 1 because I *think* the implementation ends up being simpler.
rayluo: So the only way to achieve option 1 in _get_client_args() would be to create a Config instance just for using its default value:

        if client_config is None:
            client_config = Config()
        timeout = (client_config.connect_timeout, client_config.read_timeout)

The code does look simpler, which can justify the minor performance penalty of building an extra instance.

New change is coming soon!
jamesls: @rayluo A few lines below (L228) we create a copy of the client config.  Let's consolidate all that so we're not creating multiple config objects.  That's what I was implying when I mentioned "restructuring some of the internals of `_get_client_args`.  Move the endpoint creation till after we've created the copy of the config, and just use whatever the timeouts happen to be in that config object to create the endpoint.
rayluo: OK then. We will have to conditionally initialize the both timeout parameters. The ending part of _get_client_args() will look like this:

        # Create a new client config to be passed to the client based
        # on final values. ...
        config_kwargs = dict(
            region_name=region_name, signature_version=signature_version,
            user_agent=user_agent)
        if client_config is not None:
            config_kwargs.update(
                connect_timeout=client_config.connect_timeout,
                read_timeout=client_config.read_timeout)
        new_config = Config(**config_kwargs)

        endpoint_creator = EndpointCreator(...)
        endpoint = endpoint_creator.create_endpoint(
            ...,
            timeout=(new_config.connect_timeout, new_config.read_timeout),
            )

        return {...}

I am refactoring some test cases.
jamesls: Just had a quick question on one of the unit tests from your latest changes.
jamesls: Just one small change to the unit tests, otherwise looks good.  :shipit: after updating that one test case.
mtdowling: I'm still curious why timeouts need to be associated with the Endpoint. @jamesls do you have an opinion on that?

Patch:
diff --git a/botocore/client.py b/botocore/client.py
index 2268b4255..5d59a227f 100644
--- a/botocore/client.py
+++ b/botocore/client.py
@@ -18,7 +18,7 @@ import botocore.serialize
 import botocore.validate
 from botocore import waiter, xform_name
 from botocore.awsrequest import prepare_request_dict
-from botocore.endpoint import EndpointCreator
+from botocore.endpoint import EndpointCreator, DEFAULT_TIMEOUT
 from botocore.exceptions import ClientError, DataNotFoundError
 from botocore.exceptions import OperationNotPageableError
 from botocore.hooks import first_non_none_response
@@ -181,13 +181,6 @@ class ClientCreator(object):
 
         event_emitter = copy.copy(self._event_emitter)
 
-        endpoint_creator = EndpointCreator(self._endpoint_resolver,
-                                           region_name, event_emitter)
-        endpoint = endpoint_creator.create_endpoint(
-            service_model, region_name, is_secure=is_secure,
-            endpoint_url=endpoint_url, verify=verify,
-            response_parser_factory=self._response_parser_factory)
-
         response_parser = botocore.parsers.create_parser(protocol)
 
         # Determine what region the user provided either via the
@@ -225,9 +218,22 @@ class ClientCreator(object):
         # Create a new client config to be passed to the client based
         # on the final values. We do not want the user to be able
         # to try to modify an existing client with a client config.
-        client_config = Config(
+        config_kwargs = dict(
             region_name=region_name, signature_version=signature_version,
             user_agent=user_agent)
+        if client_config is not None:
+            config_kwargs.update(
+                connect_timeout=client_config.connect_timeout,
+                read_timeout=client_config.read_timeout)
+        new_config = Config(**config_kwargs)
+
+        endpoint_creator = EndpointCreator(self._endpoint_resolver,
+                                           region_name, event_emitter)
+        endpoint = endpoint_creator.create_endpoint(
+            service_model, region_name, is_secure=is_secure,
+            endpoint_url=endpoint_url, verify=verify,
+            response_parser_factory=self._response_parser_factory,
+            timeout=(new_config.connect_timeout, new_config.read_timeout))
 
         return {
             'serializer': serializer,
@@ -237,7 +243,7 @@ class ClientCreator(object):
             'request_signer': signer,
             'service_model': service_model,
             'loader': self._loader,
-            'client_config': client_config
+            'client_config': new_config
         }
 
     def _create_methods(self, service_model):
@@ -521,11 +527,17 @@ class Config(object):
         * Signature version
         * User agent
         * User agent extra
+        * Connect timeout
+        * Read timeout
 
     """
     def __init__(self, region_name=None, signature_version=None,
-                 user_agent=None, user_agent_extra=None):
+                 user_agent=None, user_agent_extra=None,
+                 connect_timeout=DEFAULT_TIMEOUT,
+                 read_timeout=DEFAULT_TIMEOUT):
         self.region_name = region_name
         self.signature_version = signature_version
         self.user_agent = user_agent
         self.user_agent_extra = user_agent_extra
+        self.connect_timeout = connect_timeout
+        self.read_timeout = read_timeout
diff --git a/botocore/data/logs/2014-03-28/paginators-1.json b/botocore/data/logs/2014-03-28/paginators-1.json
index 57449c575..f68932415 100644
--- a/botocore/data/logs/2014-03-28/paginators-1.json
+++ b/botocore/data/logs/2014-03-28/paginators-1.json
@@ -38,6 +38,12 @@
         "events",
         "searchedLogStreams"
       ]
+    },
+    "GetLogEvents": {
+      "input_token": "nextToken",
+      "output_token": "nextForwardToken",
+      "limit_key": "limit",
+      "result_key": "events"
     }
   }
 }
diff --git a/botocore/endpoint.py b/botocore/endpoint.py
index 0b4d83e81..c28f1b354 100644
--- a/botocore/endpoint.py
+++ b/botocore/endpoint.py
@@ -96,6 +96,7 @@ class Endpoint(object):
         self.proxies = proxies
         self.http_session = PreserveAuthSession()
         self.timeout = timeout
+        logger.debug('Setting %s timeout as %s', endpoint_prefix, self.timeout)
         self._lock = threading.Lock()
         if response_parser_factory is None:
             response_parser_factory = parsers.ResponseParserFactory()
@@ -221,7 +222,7 @@ class EndpointCreator(object):
 
     def create_endpoint(self, service_model, region_name=None, is_secure=True,
                         endpoint_url=None, verify=None,
-                        response_parser_factory=None):
+                        response_parser_factory=None, timeout=DEFAULT_TIMEOUT):
         if region_name is None:
             region_name = self._configured_region
         # Use the endpoint resolver heuristics to build the endpoint url.
@@ -247,16 +248,17 @@ class EndpointCreator(object):
             final_endpoint_url = endpoint['uri']
         if not is_valid_endpoint_url(final_endpoint_url):
             raise ValueError("Invalid endpoint: %s" % final_endpoint_url)
-        return self._get_endpoint(
-            service_model, final_endpoint_url, verify, response_parser_factory)
 
-    def _get_endpoint(self, service_model, endpoint_url,
-                      verify, response_parser_factory):
-        endpoint_prefix = service_model.endpoint_prefix
-        event_emitter = self._event_emitter
-        return self._get_endpoint_complex(endpoint_prefix, endpoint_url,
-                                          verify, event_emitter,
-                                          response_parser_factory)
+        proxies = self._get_proxies(final_endpoint_url)
+        verify_value = self._get_verify_value(verify)
+        return Endpoint(
+            final_endpoint_url,
+            endpoint_prefix=service_model.endpoint_prefix,
+            event_emitter=self._event_emitter,
+            proxies=proxies,
+            verify=verify_value,
+            timeout=timeout,
+            response_parser_factory=response_parser_factory)
 
     def _get_proxies(self, url):
         # We could also support getting proxies from a config file,
@@ -275,17 +277,3 @@ class EndpointCreator(object):
         # Otherwise use the value from REQUESTS_CA_BUNDLE, or default to
         # True if the env var does not exist.
         return os.environ.get('REQUESTS_CA_BUNDLE', True)
-
-    def _get_endpoint_complex(self, endpoint_prefix,
-                              endpoint_url, verify,
-                              event_emitter,
-                              response_parser_factory=None):
-        proxies = self._get_proxies(endpoint_url)
-        verify = self._get_verify_value(verify)
-        return Endpoint(
-            endpoint_url,
-            endpoint_prefix=endpoint_prefix,
-            event_emitter=event_emitter,
-            proxies=proxies,
-            verify=verify,
-            response_parser_factory=response_parser_factory)


Test patch:
diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py
index 56b7e0916..38b42bcfa 100644
--- a/tests/unit/test_client.py
+++ b/tests/unit/test_client.py
@@ -16,6 +16,7 @@ import mock
 
 import botocore
 from botocore import client
+from botocore.endpoint import DEFAULT_TIMEOUT
 from botocore import hooks
 from botocore.credentials import Credentials
 from botocore.exceptions import ParamValidationError
@@ -467,34 +468,56 @@ class TestAutoGeneratedClient(unittest.TestCase):
             # Missing required 'Foo' param.
             service_client.test_operation(Bar='two')
 
+    def test_client_with_custom_both_timeout(self):
+        self.create_client_creator().create_client('myservice', 'us-west-2',
+            client_config=client.Config(connect_timeout=123, read_timeout=234))
+        call_kwargs = self.endpoint_creator.create_endpoint.call_args[1]
+        self.assertEqual(call_kwargs['timeout'], (123, 234))
+
+    def test_client_with_custom_connect_timeout(self):
+        self.create_client_creator().create_client('myservice', 'us-west-2',
+            client_config=client.Config(connect_timeout=123))
+        call_kwargs = self.endpoint_creator.create_endpoint.call_args[1]
+        self.assertEqual(call_kwargs['timeout'], (123, DEFAULT_TIMEOUT))
+
+    def test_client_with_custom_read_timeout(self):
+        self.create_client_creator().create_client('myservice', 'us-west-2',
+            client_config=client.Config(read_timeout=234))
+        call_kwargs = self.endpoint_creator.create_endpoint.call_args[1]
+        self.assertEqual(call_kwargs['timeout'], (DEFAULT_TIMEOUT, 234))
+
+    def test_client_with_custom_neither_timeout(self):
+        self.create_client_creator().create_client('myservice', 'us-west-2')
+        call_kwargs = self.endpoint_creator.create_endpoint.call_args[1]
+        self.assertEqual(call_kwargs['timeout'],
+                         (DEFAULT_TIMEOUT, DEFAULT_TIMEOUT))
+
     def test_client_with_custom_params(self):
         creator = self.create_client_creator()
-
         creator.create_client('myservice', 'us-west-2',
                               is_secure=False, verify=False)
-        self.endpoint_creator.create_endpoint.assert_called_with(
-            mock.ANY, 'us-west-2', is_secure=False,
-            endpoint_url=None, verify=False,
-            response_parser_factory=None)
+        call_kwargs = self.endpoint_creator.create_endpoint.call_args[1]
+        self.assertFalse(call_kwargs['is_secure'])
+        self.assertFalse(call_kwargs['verify'])
+
+    def test_client_without_endpoint_url(self):
+        self.create_client_creator().create_client('myservice', 'us-west-2')
+        call_kwargs = self.endpoint_creator.create_endpoint.call_args[1]
+        self.assertEqual(call_kwargs['endpoint_url'], None)
 
     def test_client_with_endpoint_url(self):
         creator = self.create_client_creator()
-
         creator.create_client('myservice', 'us-west-2',
                               endpoint_url='http://custom.foo')
-        self.endpoint_creator.create_endpoint.assert_called_with(
-            mock.ANY, 'us-west-2', is_secure=True,
-            endpoint_url='http://custom.foo', verify=None,
-            response_parser_factory=None)
+        call_kwargs = self.endpoint_creator.create_endpoint.call_args[1]
+        self.assertEqual(call_kwargs['endpoint_url'], 'http://custom.foo')
 
     def test_client_with_response_parser_factory(self):
         factory = mock.Mock()
         creator = self.create_client_creator(response_parser_factory=factory)
         creator.create_client('myservice', 'us-west-2')
-        self.endpoint_creator.create_endpoint.assert_called_with(
-            mock.ANY, 'us-west-2', is_secure=True,
-            endpoint_url=None, verify=None,
-            response_parser_factory=factory)
+        call_kwargs = self.endpoint_creator.create_endpoint.call_args[1]
+        self.assertEqual(call_kwargs['response_parser_factory'], factory)
 
     def test_operation_cannot_paginate(self):
         pagination_config = {
diff --git a/tests/unit/test_endpoint.py b/tests/unit/test_endpoint.py
index cb490d9be..a42f542b7 100644
--- a/tests/unit/test_endpoint.py
+++ b/tests/unit/test_endpoint.py
@@ -278,6 +278,16 @@ class TestEndpointCreator(unittest.TestCase):
                                                 endpoint_url='https://foo')
         self.assertEqual(endpoint.host, 'https://foo')
 
+    def test_create_endpoint_with_default_timeout(self):
+        endpoint = self.creator.create_endpoint(
+            self.service_model, 'us-west-2')
+        self.assertEqual(endpoint.timeout, DEFAULT_TIMEOUT)
+
+    def test_create_endpoint_with_customized_timeout(self):
+        endpoint = self.creator.create_endpoint(
+            self.service_model, 'us-west-2', timeout=123)
+        self.assertEqual(endpoint.timeout, 123)
+
     def test_get_endpoint_default_verify_ssl(self):
         endpoint = self.creator.create_endpoint(
             self.service_model, 'us-west-2')