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

[Anthony Lenton]

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).

[Jonathan Lange (jml)]

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)]

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

[lp:~elachuni/piston-mini-client/timeouts]

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)]

> 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.

=== modified file 'doc/envvars.rst'
--- doc/envvars.rst	2012-03-30 21:35:01 +0000
+++ doc/envvars.rst	2012-04-02 14:26:18 +0000
@@ -13,6 +13,9 @@
    ``piston_mini_client`` will log all requests and responses, including
    headers, to this location.
 
+ * ``PISTON_MINI_CLIENT_DEFAULT_TIMEOUT``: Is used as a socket timeout for
+   instances that don't explicitly set a timeout.  Should be in seconds.
+
  * ``http_proxy`` / ``https_proxy``: ``piston_mini_client`` will check these
    variables to determine if a proxy should be used for each scheme.
    The `SocksiPy <http://socksipy.sourceforge.net/>`_ module is needed for
 
=== modified file 'doc/tuning.rst'
--- doc/tuning.rst	2012-03-30 21:35:01 +0000
+++ doc/tuning.rst	2012-04-02 14:26:18 +0000
@@ -176,3 +176,20 @@
 ``PISTON_MINI_CLIENT_LOG_FILENAME`` in the environment to point a file,
 and all ``PistonAPI`` instances will use this location by default. That is,
 unless they're explicitly being instantiated to log elsewhere.
+
+=================
+Handling timeouts
+=================
+When you instantiate a ``PistonAPI`` you can provide an optional ``timeout``
+argument that will be used as a socket timeout for the requests that instance
+makes.  To explicitly set no timeout, pass in ``timeout=0``.  If you leave
+the default ``timeout=None``, the instance will first check for an environment
+variable ``PISTON_MINI_CLIENT_DEFAULT_TIMEOUT``, and if that is undefined or
+invalid, then the class's default timeout will be used; this can be defined
+by setting a ``default_timeout`` class attribute when
+writing the API class.  Finally, if the class's default timeout is
+also ``None``, Python's system-wide socket default timeout will be used.
+
+You can't currently define timeouts on a per-request basis.  If you need to
+change the timeout used for certain requests, you'll need to use a new
+``PistonAPI`` instance.
 
=== modified file 'piston_mini_client/__init__.py'
--- piston_mini_client/__init__.py	2012-03-30 21:35:01 +0000
+++ piston_mini_client/__init__.py	2012-04-02 14:26:18 +0000
@@ -16,6 +16,7 @@
 
 import json
 import os
+import socket
 import urllib
 from datetime import datetime
 from functools import wraps
@@ -23,13 +24,16 @@
 
 from piston_mini_client.failhandlers import (
     APIError,
+    ExceptionFailHandler,
     format_request,
     format_response,
-    ExceptionFailHandler,
+    SocketError,
+    TimeoutError,
 )
 from piston_mini_client.consts import (
     DISABLE_SSL_VALIDATION_ENVVAR,
     LOG_FILENAME_ENVVAR,
+    TIMEOUT_ENVVAR,
 )
 
 
@@ -195,6 +199,7 @@
     It's a small wrapper around ``httplib2`` to allow for a bit of state to
     be stored (like the service root) so that you don't need to repeat
     yourself as much.
+
     It's not intended to be used directly.  Children classes should implement
     methods that actually call out to the api methods.
 
@@ -203,20 +208,16 @@
     methods provided by this class.
     """
     SUPPORTED_SCHEMAS = ("http", "https")
-
     default_service_root = ''
-
     default_content_type = 'application/json'
-
+    default_timeout = None
     fail_handler = ExceptionFailHandler
-
     extra_headers = None
-
     serializers = None
 
     def __init__(self, service_root=None, cachedir=None, auth=None,
                  offline_mode=False, disable_ssl_validation=False,
-                 log_filename=None):
+                 log_filename=None, timeout=None):
         """Initialize a ``PistonAPI``.
 
         ``service_root`` is the url to the server's service root.
@@ -241,6 +242,15 @@
 
         If you pass in a ``log_filename``, all requests and responses
         including headers will be logged to this file.
+
+        ``timeout`` will be used as a socket timeout for all calls this
+        instance makes.  To explicitly set no timeout, set timeout=0.  The
+        default timeout=None will first check for an environment variable
+        ``PISTON_MINI_CLIENT_DEFAULT_TIMEOUT`` and try to use that. If this
+        environment variable is not found or it is an invalid float, the
+        class's ``default_timeout`` will be used.  Finally, if the class's
+        default is also None, Python's default timeout for sockets will be
+        used.  All these should be in seconds.
         """
         if service_root is None:
             service_root = self.default_service_root
@@ -259,6 +269,12 @@
         self._auth = auth
         self._offline_mode = offline_mode
         self._disable_ssl_validation = disable_ssl_validation
+        if timeout is None:
+            try:
+                timeout = float(os.environ.get(TIMEOUT_ENVVAR))
+            except (TypeError, ValueError):
+                timeout = self.default_timeout
+        self._timeout = timeout
         # create one httplib2.Http object per scheme so that we can
         # have per-scheme proxy settings (see also Issue 26
         #   http://code.google.com/p/httplib2/issues/detail?id=26)
@@ -279,6 +295,7 @@
             try:
                 http = httplib2.Http(
                     cache=self._httplib2_cache,
+                    timeout=self._timeout,
                     disable_ssl_certificate_validation=True,
                     proxy_info=proxy_info)
             except TypeError:
@@ -286,7 +303,7 @@
                 pass
         if http is None:
             http = httplib2.Http(cache=self._httplib2_cache,
-                                 proxy_info=proxy_info)
+                timeout=self._timeout, proxy_info=proxy_info)
         return http
 
     def _get_proxy_info(self, scheme):
@@ -498,6 +515,12 @@
                 raise APIError('Unable to connect to %s' % self._service_root)
             else:
                 raise
+        except socket.timeout, e:
+            raise TimeoutError('Timed out attempting to connect to %s' %
+                self._service_root)
+        except (socket.gaierror, socket.error), e:
+            raise SocketError('connecting to %s: %s' % (self._service_root,
+                e.message))
         if self.log_filename:
             self._dump_response(response, response_body)
         handler = self.fail_handler(url, method, body, headers)
 
=== modified file 'piston_mini_client/consts.py'
--- piston_mini_client/consts.py	2012-03-30 21:35:01 +0000
+++ piston_mini_client/consts.py	2012-04-02 14:26:18 +0000
@@ -5,3 +5,4 @@
 DISABLE_SSL_VALIDATION_ENVVAR = 'PISTON_MINI_CLIENT_DISABLE_SSL_VALIDATION'
 DEBUG_ENVVAR = 'PISTON_MINI_CLIENT_DEBUG'
 LOG_FILENAME_ENVVAR = 'PISTON_MINI_CLIENT_LOG_FILENAME'
+TIMEOUT_ENVVAR = 'PISTON_MINI_CLIENT_DEFAULT_TIMEOUT'
 
=== modified file 'piston_mini_client/failhandlers.py'
--- piston_mini_client/failhandlers.py	2012-03-30 21:35:01 +0000
+++ piston_mini_client/failhandlers.py	2012-04-02 14:26:18 +0000
@@ -66,6 +66,18 @@
         return self.msg
 
 
+class SocketError(APIError):
+    def __init__(self, msg):
+        self.msg = msg
+
+    def __str__(self):
+        return self.msg
+
+
+class TimeoutError(SocketError):
+    pass
+
+
 class BaseFailHandler(object):
     """A base class for fail handlers.
 
 
=== added file 'piston_mini_client/tests/test_timeout.py'
--- piston_mini_client/tests/test_timeout.py	1970-01-01 00:00:00 +0000
+++ piston_mini_client/tests/test_timeout.py	2012-04-02 14:26:18 +0000
@@ -0,0 +1,73 @@
+# -*- coding: utf-8 -*-
+# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
+# GNU Lesser General Public License version 3 (see the file LICENSE).
+
+import socket
+from unittest import TestCase
+from mock import patch
+from piston_mini_client import PistonAPI
+from piston_mini_client.failhandlers import APIError
+
+
+class LazyAPI(PistonAPI):
+    default_service_root = 'http://test.info/api/1.0/'
+    default_timeout = 42
+
+    def sleep(self, amount):
+        return self._get('/snooze/%s/' % amount)
+
+
+class TimeoutTestCase(TestCase):
+    @patch('os.environ.get')
+    @patch('httplib2.Http')
+    def test_timeout_in_constructor_wins(self, mock_http, mock_get):
+        mock_get.return_value = '3.14'
+        api = LazyAPI(timeout=1)
+        self.assertEqual(1, api._timeout)
+        mock_http.assert_called_with(cache=None, proxy_info=None, timeout=1,
+            disable_ssl_certificate_validation=True)
+
+    @patch('os.environ.get')
+    @patch('httplib2.Http')
+    def test_timeout_in_env_beats_class_default(self, mock_http, mock_get):
+        mock_get.return_value = '3.14'
+        api = LazyAPI()
+        self.assertEqual(3.14, api._timeout)
+        mock_http.assert_called_with(cache=None, proxy_info=None, timeout=3.14,
+            disable_ssl_certificate_validation=True)
+
+    @patch('os.environ.get')
+    @patch('httplib2.Http')
+    def test_no_envvar_falls_back_to_class_default(self, mock_http, mock_get):
+        mock_get.return_value = None
+        api = LazyAPI()
+        self.assertEqual(42, api._timeout)
+        mock_http.assert_called_with(cache=None, proxy_info=None, timeout=42)
+
+    @patch('os.environ.get')
+    @patch('httplib2.Http')
+    def test_no_nothing_falls_back_to_system_default(self, mock_http,
+        mock_get):
+        class DefaultAPI(PistonAPI):
+            default_service_root = 'http://test.info/api/1.0/'
+
+        mock_get.return_value = None
+        api = DefaultAPI()
+        self.assertEqual(None, api._timeout)
+        mock_http.assert_called_with(cache=None, proxy_info=None, timeout=None)
+
+    @patch('os.environ.get')
+    @patch('httplib2.Http')
+    def test_invalid_envvar_uses_class_default(self, mock_http, mock_get):
+        mock_get.return_value = 'invalid'
+        api = LazyAPI()
+        self.assertEqual(42, api._timeout)
+        mock_http.assert_called_with(cache=None, proxy_info=None, timeout=42,
+            disable_ssl_certificate_validation=True)
+
+    @patch('httplib2.HTTPConnectionWithTimeout.connect')
+    def test_timeout_is_handled_by_failhandler(self, mock_connect):
+        mock_connect.side_effect = socket.timeout
+        api = LazyAPI()
+
+        self.assertRaises(APIError, api.sleep, 2)