Code review comment for lp:~elachuni/piston-mini-client/timeouts

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