From 224839dd9c873d336f6e2f82a95fb174f04a3fee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fl=C3=A1vio=20Juvenal?= Date: Thu, 20 Aug 2015 21:25:22 -0300 Subject: [PATCH 1/3] (RSC13) The client library must use default connection and request timeouts (missing tests) --- ably/http/http.py | 57 ++++++++++++++++++++++++++++++----------- ably/util/exceptions.py | 4 +++ 2 files changed, 46 insertions(+), 15 deletions(-) diff --git a/ably/http/http.py b/ably/http/http.py index 69a8efc6..31eba873 100644 --- a/ably/http/http.py +++ b/ably/http/http.py @@ -2,6 +2,8 @@ import functools import logging +import socket +import time from six.moves import range from six.moves.urllib.parse import urljoin @@ -124,6 +126,13 @@ def links(self): class Http(object): + CONNECTION_RETRY = { + 'single_request_connect_timeout': 4, + 'single_request_read_timeout': 15, + 'max_retry_attempts': 3, + 'cumulative_timeout': 10, + } + def __init__(self, ably, options): options = options or {} self.__ably = ably @@ -148,21 +157,39 @@ def make_request(self, method, url, headers=None, body=None, skip_auth=False, ti if not skip_auth: headers.update(self.auth._get_auth_headers()) - request = requests.Request(method, url, data=body, headers=headers) - prepped = self.__session.prepare_request(request) - - # log.debug("Method: %s" % method) - # log.debug("Url: %s" % url) - # log.debug("Headers: %s" % headers) - # log.debug("Body: %s" % body) - # log.debug("Prepped: %s" % prepped) - - # TODO add timeouts from options here - response = self.__session.send(prepped) - - AblyException.raise_for_response(response) - - return Response(response) + single_request_connect_timeout = self.CONNECTION_RETRY['single_request_connect_timeout'] + single_request_read_timeout = self.CONNECTION_RETRY['single_request_read_timeout'] + max_retry_attempts = self.CONNECTION_RETRY['max_retry_attempts'] + cumulative_timeout = self.CONNECTION_RETRY['cumulative_timeout'] + requested_at = time.time() + for retry_count in range(max_retry_attempts): + try: + request = requests.Request(method, url, data=body, headers=headers) + prepped = self.__session.prepare_request(request) + response = self.__session.send( + prepped, + timeout=(single_request_connect_timeout, + single_request_read_timeout)) + + AblyException.raise_for_response(response) + return Response(response) + except (requests.exceptions.RequestException, + socket.timeout, + socket.error, + AblyException) as e: + # See http://docs.python-requests.org/en/latest/user/quickstart/#errors-and-exceptions + # and https://github.com/kennethreitz/requests/issues/1236 + # for why catching these exceptions. + + # if not server error, throw exception up + if isinstance(e, AblyException) and not e.is_server_error: + raise e + + # if last try or cumulative timeout is done, throw exception up + time_passed = time.time() - requested_at + if retry_count == max_retry_attempts - 1 or \ + time_passed > cumulative_timeout: + raise e def request(self, request): return self.make_request(request.method, request.url, headers=request.headers, body=request.body) diff --git a/ably/util/exceptions.py b/ably/util/exceptions.py index 2f2640b8..82ea9bee 100644 --- a/ably/util/exceptions.py +++ b/ably/util/exceptions.py @@ -19,6 +19,10 @@ def __init__(self, reason, status_code, code): def __unicode__(self): return six.u('%s %s %s') % (self.code, self.status_code, self.reason) + @property + def is_server_error(self): + return self.status_code == 500 + @staticmethod def raise_for_response(response): if response.status_code >= 200 and response.status_code < 300: From 0baf9b715d51a5f3de55fa4686f554c6fcca8ebd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fl=C3=A1vio=20Juvenal?= Date: Thu, 20 Aug 2015 21:53:19 -0300 Subject: [PATCH 2/3] Correctly testing RSC13 --- test/ably/resthttp_test.py | 53 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 test/ably/resthttp_test.py diff --git a/test/ably/resthttp_test.py b/test/ably/resthttp_test.py new file mode 100644 index 00000000..45826592 --- /dev/null +++ b/test/ably/resthttp_test.py @@ -0,0 +1,53 @@ +from __future__ import absolute_import + +import unittest +import time + +import mock +import requests + +from ably import AblyRest + + +class TestRestHttp(unittest.TestCase): + def test_max_retry_attempts_and_timeouts(self): + ably = AblyRest(token="foo") + self.assertIn('single_request_connect_timeout', ably.http.CONNECTION_RETRY) + self.assertIn('single_request_read_timeout', ably.http.CONNECTION_RETRY) + self.assertIn('max_retry_attempts', ably.http.CONNECTION_RETRY) + + with mock.patch('requests.sessions.Session.send', + side_effect=requests.exceptions.RequestException) as send_mock: + try: + ably.http.make_request('GET', '/', skip_auth=True) + except requests.exceptions.RequestException: + pass + + self.assertEqual( + send_mock.call_count, + ably.http.CONNECTION_RETRY['max_retry_attempts']) + self.assertEqual( + send_mock.call_args, + mock.call(mock.ANY, timeout=(ably.http.CONNECTION_RETRY['single_request_connect_timeout'], + ably.http.CONNECTION_RETRY['single_request_read_timeout']))) + + def test_cumulative_timeout(self): + ably = AblyRest(token="foo") + self.assertIn('cumulative_timeout', ably.http.CONNECTION_RETRY) + + cumulative_timeout_original_value = ably.http.CONNECTION_RETRY['cumulative_timeout'] + ably.http.CONNECTION_RETRY['cumulative_timeout'] = 0.5 + + def sleep_and_raise(*args, **kwargs): + time.sleep(0.51) + raise requests.exceptions.RequestException + with mock.patch('requests.sessions.Session.send', + side_effect=sleep_and_raise) as send_mock: + try: + ably.http.make_request('GET', '/', skip_auth=True) + except requests.exceptions.RequestException: + pass + + self.assertEqual(send_mock.call_count, 1) + + ably.http.CONNECTION_RETRY['cumulative_timeout'] = cumulative_timeout_original_value From f76fb0f033a8073d4f9a1e540ab30f659c574e93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fl=C3=A1vio=20Juvenal?= Date: Fri, 21 Aug 2015 11:12:14 -0300 Subject: [PATCH 3/3] Catching correct Exception in http retry --- ably/http/http.py | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/ably/http/http.py b/ably/http/http.py index 31eba873..5aae4f21 100644 --- a/ably/http/http.py +++ b/ably/http/http.py @@ -2,7 +2,6 @@ import functools import logging -import socket import time from six.moves import range @@ -173,13 +172,9 @@ def make_request(self, method, url, headers=None, body=None, skip_auth=False, ti AblyException.raise_for_response(response) return Response(response) - except (requests.exceptions.RequestException, - socket.timeout, - socket.error, - AblyException) as e: - # See http://docs.python-requests.org/en/latest/user/quickstart/#errors-and-exceptions - # and https://github.com/kennethreitz/requests/issues/1236 - # for why catching these exceptions. + except Exception as e: + # Need to catch `Exception`, see: + # https://github.com/kennethreitz/requests/issues/1236#issuecomment-133312626 # if not server error, throw exception up if isinstance(e, AblyException) and not e.is_server_error: