Merge pull request #166 from analogue/pass_timeouts

Support timeouts in _request_options

CHANGELOG.rst

@@ -1,3 +1,9 @@
+3.0.0 (2015-XX-XX)
+---------------------
+- Support passing in connect_timeout and timeout via _request_options to the Fido and Requests clients
+- Timeout in HTTPFuture now defaults to None (wait indefinitely) instead of 5s. You should make sure
+  any calls to http_future.result(..) without a timeout are updated accordingly.
+
 2.1.0 (2015-07-20)
 ---------------------
 - Add warning for deprecated operations

bravado/client.py

@@ -174,25 +174,31 @@ def __getattr__(self, name):
 
     def construct_request(self, **op_kwargs):
         """
-        :param op_kwargs: parameter name/value pairs to pass to the invocation
-            of the operation.
+        :param op_kwargs: parameter name/value pairs to passed to the
+            invocation of the operation.
         :return: request in dict form
         """
         request_options = op_kwargs.pop('_request_options', {})
         url = self.operation.swagger_spec.api_url.rstrip('/') + self.path_name
         request = {
             'method': self.operation.http_method.upper(),
             'url': url,
-            'params': {},
+            'params': {},  # filled in downstream
             'headers': request_options.get('headers', {}),
         }
+
+        # Copy over optional request options
+        for request_option in ('connect_timeout', 'timeout'):
+            if request_option in request_options:
+                request[request_option] = request_options[request_option]
+
         self.construct_params(request, op_kwargs)
         return request
 
     def construct_params(self, request, op_kwargs):
         """
         Given the parameters passed to the operation invocation, validates and
-        marshals the parmameters into the provided request dict.
+        marshals the parameters into the provided request dict.
 
         :type request: dict
         :param op_kwargs: the kwargs passed to the operation invocation
@@ -225,10 +231,10 @@ def __call__(self, **op_kwargs):
         """
         log.debug(u"%s(%s)" % (self.operation.operation_id, op_kwargs))
         warn_for_deprecated_op(self.operation)
-        request = self.construct_request(**op_kwargs)
+        request_params = self.construct_request(**op_kwargs)
 
         def response_callback(response_adapter):
             return unmarshal_response(response_adapter, self)
 
         return self.operation.swagger_spec.http_client.request(
-            request, response_callback)
+            request_params, response_callback)

bravado/fido_client.py

@@ -59,13 +59,19 @@ def request(self, request_params, response_callback=None):
         url = '%s?%s' % (request_params['url'], urllib_utf8.urlencode(
             request_params.get('params', []), True))
 
-        request_params = {
+        fetch_kwargs = {
             'method': str(request_params.get('method', 'GET')),
             'body': stringify_body(request_params),
             'headers': request_params.get('headers', {}),
         }
 
-        return HttpFuture(fido.fetch(url, **request_params),
+        for fetch_kwarg in ('connect_timeout', 'timeout'):
+            if fetch_kwarg in request_params:
+                fetch_kwargs[fetch_kwarg] = request_params[fetch_kwarg]
+
+        concurrent_future = fido.fetch(url, **fetch_kwargs)
+
+        return HttpFuture(concurrent_future,
                           FidoResponseAdapter,
                           response_callback)
 

bravado/http_client.py

@@ -9,7 +9,8 @@ class HttpClient(object):
 
     def request(self, request_params, response_callback=None):
         """
-        :param request_params: complete request data.
+        :param request_params: complete request data. e.g. url, method,
+            headers, body, params, connect_timeout, timeout, etc.
         :type request_params: dict
         :param response_callback: Function to be called on response
         :type response_callback: method

bravado/http_future.py

@@ -1,8 +1,6 @@
 # -*- coding: utf-8 -*-
 from bravado.exception import HTTPError
 
-DEFAULT_TIMEOUT_S = 5.0
-
 
 class HttpFuture(object):
     """A future which inputs HTTP params"""
@@ -20,11 +18,12 @@ def __init__(self, future, response_adapter, callback):
         self.response_adapter = response_adapter
         self.response_callback = callback
 
-    def result(self, timeout=DEFAULT_TIMEOUT_S):
+    def result(self, timeout=None):
         """Blocking call to wait for API response
 
-        :param timeout: Timeout in seconds for which client will get blocked
-        to receive the response
+        :param timeout: Number of seconds to wait for a response. Defaults to
+            None which means wait indefinitely.
+        :type timeout: float
         :return: swagger response return value when given a callback or the
             http_response otherwise.
         """

bravado/requests_client.py

@@ -93,6 +93,31 @@ def __init__(self):
         self.session = requests.Session()
         self.authenticator = None
 
+    @staticmethod
+    def separate_params(request_params):
+        """Splits the passed in dict of request_params into two buckets.
+
+        - sanitized_params are valid kwargs for constructing a
+          requests.Request(..)
+        - misc_options are things like timeouts which can't be communicated
+          to the Requests library via the requests.Request(...) constructor.
+
+        :param request_params: kitchen sink of request params. Treated as a
+            read-only dict.
+        :returns: tuple(sanitized_params, misc_options)
+        """
+        sanitized_params = request_params.copy()
+        misc_options = {}
+
+        if 'connect_timeout' in sanitized_params:
+            misc_options['connect_timeout'] = \
+                sanitized_params.pop('connect_timeout')
+
+        if 'timeout' in sanitized_params:
+            misc_options['timeout'] = sanitized_params.pop('timeout')
+
+        return sanitized_params, misc_options
+
     def request(self, request_params, response_callback=None):
         """
         :param request_params: complete request data.
@@ -101,8 +126,12 @@ def request(self, request_params, response_callback=None):
         :returns: HTTP Future object
         :rtype: :class: `bravado_core.http_future.HttpFuture`
         """
+        sanitized_params, misc_options = self.separate_params(request_params)
+
         requests_future = RequestsFutureAdapter(
-            self.session, self.authenticated_request(request_params))
+            self.session,
+            self.authenticated_request(sanitized_params),
+            misc_options)
 
         return HttpFuture(
             requests_future,
@@ -168,35 +197,86 @@ def json(self, **kwargs):
 
 
 class RequestsFutureAdapter(object):
-    """A future which inputs HTTP params"""
+    """Mimics a :class:`concurrent.futures.Future` for the purposes of making
+    HTTP calls with the Requests library in a future-y sort of way.
+    """
 
-    def __init__(self, session, request):
+    def __init__(self, session, request, misc_options):
         """Kicks API call for Requests client
 
         :param session: session object to use for making the request
         :param request: dict containing API request parameters
+        :param misc_options: misc options to apply when sending a HTTP request.
+            e.g. timeout, connect_timeout, etc
+        :type misc_options: dict
         """
         self.session = session
         self.request = request
+        self.misc_options = misc_options
 
     def check_for_exceptions(self, response):
         try:
             response.raise_for_status()
         except Exception as e:
             add_response_detail_to_errors(e)
 
-    def result(self, timeout):
+    def build_timeout(self, result_timeout):
+        """
+        Build the appropriate timeout object to pass to `session.send(...)`
+        based on connect_timeout, the timeout passed to the service call, and
+        the timeout passed to the result call.
+
+        :param result_timeout: timeout that was passed into `future.result(..)`
+        :return: timeout
+        :rtype: float or tuple(connect_timeout, timeout)
+        """
+        # The API provides two ways to pass a timeout :( We're stuck
+        # dealing with it until we're ready to make a non-backwards
+        # compatible change.
+        #
+        #  - If both timeouts are the same, no problem
+        #  - If either has a non-None value, use the non-None value
+        #  - If both have a non-None value, use the greater of the two
+        timeout = None
+        has_service_timeout = 'timeout' in self.misc_options
+        service_timeout = self.misc_options.get('timeout')
+
+        if not has_service_timeout:
+            timeout = result_timeout
+        elif service_timeout == result_timeout:
+            timeout = service_timeout
+        else:
+            if service_timeout is None:
+                timeout = result_timeout
+            elif result_timeout is None:
+                timeout = service_timeout
+            else:
+                timeout = max(service_timeout, result_timeout)
+            log.warn("Two different timeouts have been passed: "
+                     "_request_options['timeout'] = {0} and "
+                     "future.result(timeout={1}). Using timeout of {2}."
+                     .format(service_timeout, result_timeout, timeout))
+
+        # Requests is weird in that if you want to specify a connect_timeout
+        # and idle timeout, then the timeout is passed as a tuple
+        if 'connect_timeout' in self.misc_options:
+            timeout = self.misc_options['connect_timeout'], timeout
+        return timeout
+
+    def result(self, timeout=None):
         """Blocking call to wait for API response
 
-        :param timeout: timeout in seconds to wait for response
-        :type timeout: integer
+        :param timeout: timeout in seconds to wait for response. Defaults to
+            None to wait indefinitely.
+        :type timeout: float
         :return: raw response from the server
         :rtype: dict
         """
         request = self.request
+        prepared_request = self.session.prepare_request(request)
         response = self.session.send(
-            self.session.prepare_request(request),
-            timeout=timeout)
+            prepared_request,
+            timeout=self.build_timeout(timeout))
 
         self.check_for_exceptions(response)
 

tests/fido_client/FidoClient/request_test.py

@@ -0,0 +1,52 @@
+from mock import patch, call, Mock
+import pytest
+import six
+
+
+try:
+    from bravado.fido_client import FidoClient
+except ImportError:
+    FidoClient = Mock()
+
+
+@pytest.mark.skipif(six.PY3, reason="twisted doesnt support py3 yet")
+def test_no_timeouts_passed_to_fido():
+    with patch('bravado.fido_client.fido.fetch') as fetch:
+        fido_client = FidoClient()
+        request_params = dict(url='http://foo.com/')
+        fido_client.request(request_params, response_callback=None)
+        assert fetch.call_args == call(
+            'http://foo.com/?', body='', headers={}, method='GET')
+
+
+@pytest.mark.skipif(six.PY3, reason="twisted doesnt support py3 yet")
+def test_timeout_passed_to_fido():
+    with patch('bravado.fido_client.fido.fetch') as fetch:
+        fido_client = FidoClient()
+        request_params = dict(url='http://foo.com/', timeout=1)
+        fido_client.request(request_params, response_callback=None)
+        assert fetch.call_args == call(
+            'http://foo.com/?', body='', headers={}, method='GET', timeout=1)
+
+
+@pytest.mark.skipif(six.PY3, reason="twisted doesnt support py3 yet")
+def test_connect_timeout_passed_to_fido():
+    with patch('bravado.fido_client.fido.fetch') as fetch:
+        fido_client = FidoClient()
+        request_params = dict(url='http://foo.com/', connect_timeout=1)
+        fido_client.request(request_params, response_callback=None)
+        assert fetch.call_args == call(
+            'http://foo.com/?', body='', headers={}, method='GET',
+            connect_timeout=1)
+
+
+@pytest.mark.skipif(six.PY3, reason="twisted doesnt support py3 yet")
+def test_connect_timeout_and_timeout_passed_to_fido():
+    with patch('bravado.fido_client.fido.fetch') as fetch:
+        fido_client = FidoClient()
+        request_params = dict(url='http://foo.com/', connect_timeout=1,
+                              timeout=2)
+        fido_client.request(request_params, response_callback=None)
+        assert fetch.call_args == call(
+            'http://foo.com/?', body='', headers={}, method='GET',
+            connect_timeout=1, timeout=2)

tests/requests_client/RequestsClient/separate_params_test.py

@@ -0,0 +1,12 @@
+from bravado.requests_client import RequestsClient
+
+
+def test_separate_params():
+    request_params = {
+        'url': 'http://foo.com',
+        'connect_timeout': 1,
+        'timeout': 2
+    }
+    sanitized, misc = RequestsClient.separate_params(request_params)
+    assert sanitized == {'url': 'http://foo.com'}
+    assert misc == {'connect_timeout': 1, 'timeout': 2}

tests/requests_client/RequestsFutureAdapter/build_timeout_test.py

@@ -0,0 +1,75 @@
+from mock import Mock
+import pytest
+from requests.sessions import Session
+
+from bravado.requests_client import RequestsFutureAdapter
+
+
+@pytest.fixture
+def request():
+    return dict(url='http://foo.com')
+
+
+@pytest.fixture
+def session():
+    return Mock(spec=Session)
+
+
+def test_no_timeouts(session, request):
+    misc_options = {}
+    future = RequestsFutureAdapter(session, request, misc_options)
+    assert future.build_timeout(result_timeout=None) is None
+
+
+def test_service_timeout_and_result_timeout_None(session, request):
+    misc_options = dict(timeout=1)
+    future = RequestsFutureAdapter(session, request, misc_options)
+    assert future.build_timeout(result_timeout=None) == 1
+
+
+def test_no_service_timeout_and_result_timeout_not_None(session, request):
+    misc_options = {}
+    future = RequestsFutureAdapter(session, request, misc_options)
+    assert future.build_timeout(result_timeout=1) == 1
+
+
+def test_service_timeout_lt_result_timeout(session, request):
+    misc_options = dict(timeout=10)
+    future = RequestsFutureAdapter(session, request, misc_options)
+    assert future.build_timeout(result_timeout=11) == 11
+
+
+def test_service_timeout_gt_result_timeout(session, request):
+    misc_options = dict(timeout=11)
+    future = RequestsFutureAdapter(session, request, misc_options)
+    assert future.build_timeout(result_timeout=10) == 11
+
+
+def test_service_timeout_None_result_timeout_not_None(session, request):
+    misc_options = dict(timeout=None)
+    future = RequestsFutureAdapter(session, request, misc_options)
+    assert future.build_timeout(result_timeout=10) == 10
+
+
+def test_service_timeout_not_None_result_timeout_None(session, request):
+    misc_options = dict(timeout=10)
+    future = RequestsFutureAdapter(session, request, misc_options)
+    assert future.build_timeout(result_timeout=None) == 10
+
+
+def test_both_timeouts_the_same(session, request):
+    misc_options = dict(timeout=10)
+    future = RequestsFutureAdapter(session, request, misc_options)
+    assert future.build_timeout(result_timeout=10) == 10
+
+
+def test_connect_timeout_and_idle_timeout(session, request):
+    misc_options = dict(connect_timeout=1, timeout=11)
+    future = RequestsFutureAdapter(session, request, misc_options)
+    assert future.build_timeout(result_timeout=None) == (1, 11)
+
+
+def test_connect_timeout_only(session, request):
+    misc_options = dict(connect_timeout=1)
+    future = RequestsFutureAdapter(session, request, misc_options)
+    assert future.build_timeout(result_timeout=None) == (1, None)