From 555bc4858af1a074894742cee2ab673cd3b89c4c Mon Sep 17 00:00:00 2001 From: Craig Silverstein Date: Mon, 30 Oct 2017 15:15:39 -0700 Subject: [PATCH 1/2] Add support for user-specified timeout and retry options in the client. Sailthru customer support suggests retrying on transient failures when talking to sailthru. This adds support to the client library to make it easy to do so. We retry on connection timeouts and all 5xx errors. We do not retry on read timeouts since it's not safe for POSTs, which are not idempotent (it would be ok for GETs, but that's a TODO for another day). Test Plan: I couldn't figure out how to write an automated test for this since requests hides all the retry logic from us. (I didn't want to mock the internals of the requests module since I felt that was brittle.) But I did create a local server with a route that always returns a 500, and supports only GET and not POST (so it gives a 500 for GET and a 403 for POST). I then did ``` >>> import sailthru >>> c = sailthru.SailthruClient('key', 'secret', api_url='http://localhost:8080', retries=3, timeout=5) >>> print c.api_get('crash', '').is_ok() None ``` and looked in the server-logs and saw the `/crash` url was hit 4 times. I then did ``` >>> print c.api_post('crash', '').is_ok() None ``` and looked in the server-logs and saw the endpoint was only hit 1 time this time. I then added a route to my server that waits before it responds, and did ``` >>> print c.api_post('sleep', '').is_ok() None ``` and timed that it took 5 seconds for this to run. --- sailthru/sailthru_client.py | 20 ++++++++++++++++---- sailthru/sailthru_http.py | 24 ++++++++++++++++++++++-- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/sailthru/sailthru_client.py b/sailthru/sailthru_client.py index b6085e5..7db1925 100644 --- a/sailthru/sailthru_client.py +++ b/sailthru/sailthru_client.py @@ -55,10 +55,21 @@ class SailthruClient(object): client = SailthruCLient(api_key, api_secret) """ - def __init__(self, api_key, secret, api_url=None): + def __init__(self, api_key, secret, + api_url=None, timeout=None, retries=None): + """Create a new client. + + @param api_key: the key used to identify this app to sailthru + @param secret: the secret used to authenticate this user with sailthru + @param api_url: the protocol+host to send API requests to, or None for the default + @param timeout: how long to wait for HTTP requests, or None for the default + @param retries: how many times to retry on transient HTTP errors, or None for the default. + """ self.api_key = api_key self.secret = secret - self.api_url = api_url if api_url else 'https://api.sailthru.com' + self.api_url = api_url if api_url is not None else 'https://api.sailthru.com' + self.timeout = timeout if timeout is not None else 10 + self.retries = retries if retries is not None else 0 self.last_rate_limit_info = {} def send(self, template, email, _vars=None, options=None, schedule_time=None, limit=None): @@ -762,7 +773,8 @@ def _api_request(self, action, data, request_type): def _http_request(self, action, data, method, file_data=None): url = self.api_url + '/' + action file_data = file_data or {} - response = sailthru_http_request(url, data, method, file_data) + response = sailthru_http_request(url, data, method, file_data, + timeout=self.timeout, retries=self.retries) if (action in self.last_rate_limit_info): self.last_rate_limit_info[action][method] = response.get_rate_limit_headers() else: @@ -788,4 +800,4 @@ def get_last_rate_limit_info(self, action, method): if (action in self.last_rate_limit_info and method in self.last_rate_limit_info[action]): return self.last_rate_limit_info[action][method] - return None \ No newline at end of file + return None diff --git a/sailthru/sailthru_http.py b/sailthru/sailthru_http.py index ac332e6..86a8bf4 100644 --- a/sailthru/sailthru_http.py +++ b/sailthru/sailthru_http.py @@ -27,7 +27,8 @@ def flatten(hash_table, brackets=True): return f return flatten(hash_table, False) -def sailthru_http_request(url, data, method, file_data=None): +def sailthru_http_request(url, data, method, file_data=None, + timeout=10, retries=0): """ Perform an HTTP GET / POST / DELETE request """ @@ -37,7 +38,26 @@ def sailthru_http_request(url, data, method, file_data=None): try: headers = {'User-Agent': 'Sailthru API Python Client %s; Python Version: %s' % ('2.3.3', platform.python_version())} - response = requests.request(method, url, params=params, data=data, files=file_data, headers=headers, timeout=10) + if retries > 0: + session = requests.Session() + # We retry on connection errors and all 5xx errors. We do + # not retry on read errors since for POST requests that + # happens after the POST has finished, and POSTs are not + # necessarily safe to re-do. + retry = requests.urllib3.Retry(retries, + read=0, + method_whitelist=False, + status_forcelist={500, 502, 503, 504}, + raise_on_status=False) + session.mount(url, requests.adapters.HTTPAdapter(max_retries=retry)) + else: + session = requests + response = session.request(method, url, + params=params, + data=data, + files=file_data, + headers=headers, + timeout=timeout) return SailthruResponse(response) except requests.HTTPError as e: raise SailthruClientError(str(e)) From c973e63f646147ea3ea6f60a620b7ec7dc877442 Mon Sep 17 00:00:00 2001 From: Craig Silverstein Date: Thu, 2 Nov 2017 10:48:07 -0700 Subject: [PATCH 2/2] Apparently the right way to import urllib3 is via request.packages. The changelog at https://pypi.python.org/pypi/requests even seems to indicate this is kosher. The old code worked on newer versions of requests; this new code should work on older versions as well. Auditors: ragini Test plan: Ran `python -c 'import requests; requests.packages.urllib3'` on a requests module version 2.13.0. --- sailthru/sailthru_http.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/sailthru/sailthru_http.py b/sailthru/sailthru_http.py index 86a8bf4..c3b0bdf 100644 --- a/sailthru/sailthru_http.py +++ b/sailthru/sailthru_http.py @@ -44,11 +44,12 @@ def sailthru_http_request(url, data, method, file_data=None, # not retry on read errors since for POST requests that # happens after the POST has finished, and POSTs are not # necessarily safe to re-do. - retry = requests.urllib3.Retry(retries, - read=0, - method_whitelist=False, - status_forcelist={500, 502, 503, 504}, - raise_on_status=False) + retry = requests.packages.urllib3.Retry( + retries, + read=0, + method_whitelist=False, + status_forcelist={500, 502, 503, 504}, + raise_on_status=False) session.mount(url, requests.adapters.HTTPAdapter(max_retries=retry)) else: session = requests