Merge lp:~elachuni/piston-mini-client/timeouts into lp:piston-mini-client

Proposed by Anthony Lenton on 2012-04-02
Status: Merged
Approved by: Anthony Lenton on 2012-04-02
Approved revision: 55
Merged at revision: 53
Proposed branch: lp:~elachuni/piston-mini-client/timeouts
Merge into: lp:piston-mini-client
Diff against target: 268 lines (+137/-8) 6 files modified
To merge this branch: bzr merge lp:~elachuni/piston-mini-client/timeouts
Reviewer Review Type Date Requested Status
software-store-developers 2012-04-02 Pending
Review via email: mp+100417@code.launchpad.net

Description of the Change

This branch adds code for configuring and handling socket timeouts, to fix bug #937865

While I was there, I also wrapped other socket exceptions so that all errors are raised as APIError (or subclasses of that).

To post a comment you must log in.
Jonathan Lange (jml) wrote :

Thanks for looking into this Anthony.

A few thoughts,

 * Does the timeout span the whole request & response, or just the time it takes to get a connected socket? If the former, then a timeout per request would be very helpful.

 * Have you had a chance to test this with a pathological endpoint as described in bug 937865? I don't know off the top of my head how to make such a thing, but would be happy to figure it out together with you. I don't think such a thing needs to be added to the automatic test suite, but it would be good to have at least once-off verification that the patch achieves its purpose.

 * It would be helpful if the timeout exception included the time taken and a pointer to some docs on how to change the timeout.

Thanks again,
jml

Michael Vogt (mvo) wrote :

On Mon, Apr 02, 2012 at 02:12:26PM -0000, Jonathan Lange wrote:
[..]
> * Have you had a chance to test this with a pathological endpoint as described in bug 937865? I don't know off the top of my head how to make such a thing, but would be happy to figure it out together with you. I don't think such a thing needs to be added to the automatic test suite, but it would be good to have at least once-off verification that the patch achieves its purpose.
[..]

We recently added some code to software-center to test slow network,
it maybe useful for this test as well (we borrowed it from bzr iirc):

http://bazaar.launchpad.net/~software-store-developers/software-center/trunk/view/head:/contrib/simulate-slow-network.sh

Use: "tc qdisc del dev eth0 root" (or wlan0 or whatnot) to reset the
slowness again.

Cheers,
 Michael

54. By Anthony Lenton on 2012-04-02

Changed default default_timeout from 0 to None.

55. By Anthony Lenton on 2012-04-02

Added a test to check default default_timeout.

Anthony Lenton (elachuni) wrote :

> Thanks for looking into this Anthony.
>
> A few thoughts,
>
> * Does the timeout span the whole request & response, or just the time it
> takes to get a connected socket? If the former, then a timeout per request
> would be very helpful.

It affects both connect, read and write, but I afaik *each* of these operations gets the same timeout, so it's not a timeout for the whole API-level operation. Testing with a reasonably slow api and long request (staging ubuntu-recommender's "recommend-all-apps" call), setting timeout=0.5 makes the call return with an error after a few seconds:

Python 2.7.3rc2 (default, Mar 21 2012, 21:13:11)
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import timeit
>>> import sreclient
>>>
>>> api = sreclient.SoftwareCenterRecommenderAPI(
... service_root='http://rec.staging.ubuntu.com/api/1.0',
... timeout=0.5, log_filename='/tmp/api.log')
>>>
>>> def stmt():
... try:
... api._get('recommend_all_apps',
... extra_headers={'cache-control': 'no-cache'})
... except:
... print "Operation timed out!"
...
>>>
>>> timeit.repeat(stmt, number=1, repeat=3)
Operation timed out!
Operation timed out!
Operation timed out!
[3.7237908840179443, 2.8568179607391357, 2.0180060863494873]

re: Adding a timeout per request, I think this would make sense but httplib2 doesn't offer a per-request timeout, as it does its best to reuse connections between requests and only sets the timeout when it instantiates the connection (although I believe you can update the timeout on a socket no prob, so there should be a way to do this). I would be interested in looking into this option a bit more.

> * Have you had a chance to test this with a pathological endpoint as
> described in bug 937865? I don't know off the top of my head how to make such
> a thing, but would be happy to figure it out together with you. I don't think
> such a thing needs to be added to the automatic test suite, but it would be
> good to have at least once-off verification that the patch achieves its
> purpose.

The test above seems to say that it *doesn't* provide the kind of timeout you were asking for :/

> * It would be helpful if the timeout exception included the time taken and a
> pointer to some docs on how to change the timeout.

Where would you expect to find the pointer to the docs, when you print the exception? in the docstring? somewhere else?

> Thanks again,
> jml

Cheers,

achuni.

Preview Diff

1=== modified file 'doc/envvars.rst'
2--- doc/envvars.rst 2012-03-30 21:35:01 +0000
3+++ doc/envvars.rst 2012-04-02 14:26:18 +0000
4@@ -13,6 +13,9 @@
5 ``piston_mini_client`` will log all requests and responses, including
6 headers, to this location.
7
8+ * ``PISTON_MINI_CLIENT_DEFAULT_TIMEOUT``: Is used as a socket timeout for
9+ instances that don't explicitly set a timeout. Should be in seconds.
10+
11 * ``http_proxy`` / ``https_proxy``: ``piston_mini_client`` will check these
12 variables to determine if a proxy should be used for each scheme.
13 The `SocksiPy <http://socksipy.sourceforge.net/>`_ module is needed for
14
15=== modified file 'doc/tuning.rst'
16--- doc/tuning.rst 2012-03-30 21:35:01 +0000
17+++ doc/tuning.rst 2012-04-02 14:26:18 +0000
18@@ -176,3 +176,20 @@
19 ``PISTON_MINI_CLIENT_LOG_FILENAME`` in the environment to point a file,
20 and all ``PistonAPI`` instances will use this location by default. That is,
21 unless they're explicitly being instantiated to log elsewhere.
22+
23+=================
24+Handling timeouts
25+=================
26+When you instantiate a ``PistonAPI`` you can provide an optional ``timeout``
27+argument that will be used as a socket timeout for the requests that instance
28+makes. To explicitly set no timeout, pass in ``timeout=0``. If you leave
29+the default ``timeout=None``, the instance will first check for an environment
30+variable ``PISTON_MINI_CLIENT_DEFAULT_TIMEOUT``, and if that is undefined or
31+invalid, then the class's default timeout will be used; this can be defined
32+by setting a ``default_timeout`` class attribute when
33+writing the API class. Finally, if the class's default timeout is
34+also ``None``, Python's system-wide socket default timeout will be used.
35+
36+You can't currently define timeouts on a per-request basis. If you need to
37+change the timeout used for certain requests, you'll need to use a new
38+``PistonAPI`` instance.
39
40=== modified file 'piston_mini_client/__init__.py'
41--- piston_mini_client/__init__.py 2012-03-30 21:35:01 +0000
42+++ piston_mini_client/__init__.py 2012-04-02 14:26:18 +0000
43@@ -16,6 +16,7 @@
44
45 import json
46 import os
47+import socket
48 import urllib
49 from datetime import datetime
50 from functools import wraps
51@@ -23,13 +24,16 @@
52
53 from piston_mini_client.failhandlers import (
54 APIError,
55+ ExceptionFailHandler,
56 format_request,
57 format_response,
58- ExceptionFailHandler,
59+ SocketError,
60+ TimeoutError,
61 )
62 from piston_mini_client.consts import (
63 DISABLE_SSL_VALIDATION_ENVVAR,
64 LOG_FILENAME_ENVVAR,
65+ TIMEOUT_ENVVAR,
66 )
67
68
69@@ -195,6 +199,7 @@
70 It's a small wrapper around ``httplib2`` to allow for a bit of state to
71 be stored (like the service root) so that you don't need to repeat
72 yourself as much.
73+
74 It's not intended to be used directly. Children classes should implement
75 methods that actually call out to the api methods.
76
77@@ -203,20 +208,16 @@
78 methods provided by this class.
79 """
80 SUPPORTED_SCHEMAS = ("http", "https")
81-
82 default_service_root = ''
83-
84 default_content_type = 'application/json'
85-
86+ default_timeout = None
87 fail_handler = ExceptionFailHandler
88-
89 extra_headers = None
90-
91 serializers = None
92
93 def __init__(self, service_root=None, cachedir=None, auth=None,
94 offline_mode=False, disable_ssl_validation=False,
95- log_filename=None):
96+ log_filename=None, timeout=None):
97 """Initialize a ``PistonAPI``.
98
99 ``service_root`` is the url to the server's service root.
100@@ -241,6 +242,15 @@
101
102 If you pass in a ``log_filename``, all requests and responses
103 including headers will be logged to this file.
104+
105+ ``timeout`` will be used as a socket timeout for all calls this
106+ instance makes. To explicitly set no timeout, set timeout=0. The
107+ default timeout=None will first check for an environment variable
108+ ``PISTON_MINI_CLIENT_DEFAULT_TIMEOUT`` and try to use that. If this
109+ environment variable is not found or it is an invalid float, the
110+ class's ``default_timeout`` will be used. Finally, if the class's
111+ default is also None, Python's default timeout for sockets will be
112+ used. All these should be in seconds.
113 """
114 if service_root is None:
115 service_root = self.default_service_root
116@@ -259,6 +269,12 @@
117 self._auth = auth
118 self._offline_mode = offline_mode
119 self._disable_ssl_validation = disable_ssl_validation
120+ if timeout is None:
121+ try:
122+ timeout = float(os.environ.get(TIMEOUT_ENVVAR))
123+ except (TypeError, ValueError):
124+ timeout = self.default_timeout
125+ self._timeout = timeout
126 # create one httplib2.Http object per scheme so that we can
127 # have per-scheme proxy settings (see also Issue 26
128 # http://code.google.com/p/httplib2/issues/detail?id=26)
129@@ -279,6 +295,7 @@
130 try:
131 http = httplib2.Http(
132 cache=self._httplib2_cache,
133+ timeout=self._timeout,
134 disable_ssl_certificate_validation=True,
135 proxy_info=proxy_info)
136 except TypeError:
137@@ -286,7 +303,7 @@
138 pass
139 if http is None:
140 http = httplib2.Http(cache=self._httplib2_cache,
141- proxy_info=proxy_info)
142+ timeout=self._timeout, proxy_info=proxy_info)
143 return http
144
145 def _get_proxy_info(self, scheme):
146@@ -498,6 +515,12 @@
147 raise APIError('Unable to connect to %s' % self._service_root)
148 else:
149 raise
150+ except socket.timeout, e:
151+ raise TimeoutError('Timed out attempting to connect to %s' %
152+ self._service_root)
153+ except (socket.gaierror, socket.error), e:
154+ raise SocketError('connecting to %s: %s' % (self._service_root,
155+ e.message))
156 if self.log_filename:
157 self._dump_response(response, response_body)
158 handler = self.fail_handler(url, method, body, headers)
159
160=== modified file 'piston_mini_client/consts.py'
161--- piston_mini_client/consts.py 2012-03-30 21:35:01 +0000
162+++ piston_mini_client/consts.py 2012-04-02 14:26:18 +0000
163@@ -5,3 +5,4 @@
164 DISABLE_SSL_VALIDATION_ENVVAR = 'PISTON_MINI_CLIENT_DISABLE_SSL_VALIDATION'
165 DEBUG_ENVVAR = 'PISTON_MINI_CLIENT_DEBUG'
166 LOG_FILENAME_ENVVAR = 'PISTON_MINI_CLIENT_LOG_FILENAME'
167+TIMEOUT_ENVVAR = 'PISTON_MINI_CLIENT_DEFAULT_TIMEOUT'
168
169=== modified file 'piston_mini_client/failhandlers.py'
170--- piston_mini_client/failhandlers.py 2012-03-30 21:35:01 +0000
171+++ piston_mini_client/failhandlers.py 2012-04-02 14:26:18 +0000
172@@ -66,6 +66,18 @@
173 return self.msg
174
175
176+class SocketError(APIError):
177+ def __init__(self, msg):
178+ self.msg = msg
179+
180+ def __str__(self):
181+ return self.msg
182+
183+
184+class TimeoutError(SocketError):
185+ pass
186+
187+
188 class BaseFailHandler(object):
189 """A base class for fail handlers.
190
191
192=== added file 'piston_mini_client/tests/test_timeout.py'
193--- piston_mini_client/tests/test_timeout.py 1970-01-01 00:00:00 +0000
194+++ piston_mini_client/tests/test_timeout.py 2012-04-02 14:26:18 +0000
195@@ -0,0 +1,73 @@
196+# -*- coding: utf-8 -*-
197+# Copyright 2010-2012 Canonical Ltd. This software is licensed under the
198+# GNU Lesser General Public License version 3 (see the file LICENSE).
199+
200+import socket
201+from unittest import TestCase
202+from mock import patch
203+from piston_mini_client import PistonAPI
204+from piston_mini_client.failhandlers import APIError
205+
206+
207+class LazyAPI(PistonAPI):
208+ default_service_root = 'http://test.info/api/1.0/'
209+ default_timeout = 42
210+
211+ def sleep(self, amount):
212+ return self._get('/snooze/%s/' % amount)
213+
214+
215+class TimeoutTestCase(TestCase):
216+ @patch('os.environ.get')
217+ @patch('httplib2.Http')
218+ def test_timeout_in_constructor_wins(self, mock_http, mock_get):
219+ mock_get.return_value = '3.14'
220+ api = LazyAPI(timeout=1)
221+ self.assertEqual(1, api._timeout)
222+ mock_http.assert_called_with(cache=None, proxy_info=None, timeout=1,
223+ disable_ssl_certificate_validation=True)
224+
225+ @patch('os.environ.get')
226+ @patch('httplib2.Http')
227+ def test_timeout_in_env_beats_class_default(self, mock_http, mock_get):
228+ mock_get.return_value = '3.14'
229+ api = LazyAPI()
230+ self.assertEqual(3.14, api._timeout)
231+ mock_http.assert_called_with(cache=None, proxy_info=None, timeout=3.14,
232+ disable_ssl_certificate_validation=True)
233+
234+ @patch('os.environ.get')
235+ @patch('httplib2.Http')
236+ def test_no_envvar_falls_back_to_class_default(self, mock_http, mock_get):
237+ mock_get.return_value = None
238+ api = LazyAPI()
239+ self.assertEqual(42, api._timeout)
240+ mock_http.assert_called_with(cache=None, proxy_info=None, timeout=42)
241+
242+ @patch('os.environ.get')
243+ @patch('httplib2.Http')
244+ def test_no_nothing_falls_back_to_system_default(self, mock_http,
245+ mock_get):
246+ class DefaultAPI(PistonAPI):
247+ default_service_root = 'http://test.info/api/1.0/'
248+
249+ mock_get.return_value = None
250+ api = DefaultAPI()
251+ self.assertEqual(None, api._timeout)
252+ mock_http.assert_called_with(cache=None, proxy_info=None, timeout=None)
253+
254+ @patch('os.environ.get')
255+ @patch('httplib2.Http')
256+ def test_invalid_envvar_uses_class_default(self, mock_http, mock_get):
257+ mock_get.return_value = 'invalid'
258+ api = LazyAPI()
259+ self.assertEqual(42, api._timeout)
260+ mock_http.assert_called_with(cache=None, proxy_info=None, timeout=42,
261+ disable_ssl_certificate_validation=True)
262+
263+ @patch('httplib2.HTTPConnectionWithTimeout.connect')
264+ def test_timeout_is_handled_by_failhandler(self, mock_connect):
265+ mock_connect.side_effect = socket.timeout
266+ api = LazyAPI()
267+
268+ self.assertRaises(APIError, api.sleep, 2)

Subscribers

People subscribed via source and target branches