Add timeout (and verify) options to requests call

[arski]

Try testing simple-requests with 'http://www.google.com:81' as the URL for each fetcher in the swarm - they'll all take forever (literally) and never finish..

requests has nice timeout option for this - would be nice if simple-requests had a reasonable default set which could be overwritten in the Requests class.

Same goes for verify which is a requests arg to disable https certificate verification.

[ctheiss]

I'm not seeing either of these arguments in the docs: http://docs.python-requests.org/en/latest/api/#requests.Request.

Remember that you can always pass Request into swarm rather than a string.

for resp in Requests().swarm([ Request('POST', url='http://www.google.com:81', any_other_args=...), ... ]):
    print(resp.text)

[arski]

Yea indeed, I noticed that after I wrote. I've been using the timeout/verify args in requests.get before like documented in http://docs.python-requests.org/en/latest/user/quickstart/#timeouts

But looking at the actual implementation of that get() method in requests, what it does is build a Request object (like you do), with most of the args, but not all, the remaining, like timeout or verify are passed to the session objects in session.send() - see http://docs.python-requests.org/en/latest/user/advanced/#session-objects - that would go into here https://github.com/ctheiss/simple-requests/blob/master/simple_requests.py#L457 (check my branch for a hardcoded example https://github.com/arski/simple-requests/compare/master...requests-args - works well for me)

[ctheiss]

Hmm I see.

I suppose you could duplicate this behaviour by:

requests = Requests()
requests.session.verify = False
requests.session.timeout = 30

for resp in requests.swarm([ ... ]):
    print(resp.text)

Looking through the requests code, I'm having difficulty figuring out where timeout is actually used. It seems like it's just passed back and forth recursively from https://github.com/kennethreitz/requests/blob/master/requests/sessions.py#L514 to https://github.com/kennethreitz/requests/blob/master/requests/sessions.py#L171.

In either case, I'm hesitant to override requests' defaults, especially when it's easily work-aroundable.

[arski]

No I wasn't suggesting to override their defaults, but maybe add such parameters to your Requests - but if your example above works then there is no need indeed. I'll confirm tomorrow if it works.

[arski]

Hey again, it's used in https://github.com/kennethreitz/requests/blob/fe4c4f146124d7fba2e680581a5d6b9d98e3fdf8/requests/sessions.py#L522 - unfortunately, it expects it to be one of the kwargs in send() and is never used as a property of the Session object, so the workaround you proposed sadly doesn't work :(

[arski]

hey, any further thoughts on this?

[ctheiss]

Sorry I let this slide; let me circle back and think about it again; I'll have something by the end of the day.

[ctheiss]

Ok let's split this into verify and timeout. It looks like verify can be set as a session property (https://github.com/kennethreitz/requests/blob/fe4c4f146124d7fba2e680581a5d6b9d98e3fdf8/requests/sessions.py#L437). Happily, it looks like all the arguments into send are settable as a session property, except (as you pointed out) for timeout (https://github.com/kennethreitz/requests/blob/fe4c4f146124d7fba2e680581a5d6b9d98e3fdf8/requests/sessions.py#L539). However, there may be an explanation for this: it might not actually be used anywhere.

The only usage I see is (https://github.com/kennethreitz/requests/blob/fe4c4f146124d7fba2e680581a5d6b9d98e3fdf8/requests/sessions.py#L570), which in turn has only one usage: a recursive call to the first method (https://github.com/kennethreitz/requests/blob/fe4c4f146124d7fba2e680581a5d6b9d98e3fdf8/requests/sessions.py#L165).

If you can show me that it actually gets used somewhere, I'll open a ticket in the Requests project to have timeout be accessible as a Session property, just like verify and stream. If the response is negative, I'll add an optional kwargs to simple-requests. Does that sound like a plan?

[arski]

Hmm, odd, I'll test verify like that next week. As for timeout, it does indeed look odd, but I can tell for sure that setting it has an effect, so it has to be used somewhere... hm, need to find where

[EmilStenstrom]

I'll just want to second this request: an easy way to set a timeout on the requests that go out. I'll have a look at the code to see where it's set. In the requests API it looks like Requests are just data about where the request will go, but timeout is a property of making the request with .get().

[ctheiss]

@arski Ahh it's a double-bluff. Although timeout is pulled out of kwargs, it's still passed along as kwargs (https://github.com/kennethreitz/requests/blob/master/requests/sessions.py#L555) to the transport adapter.

I've opened up a ticket (https://github.com/kennethreitz/requests/issues/2011) in requests to have timeout added to the Session, but it was shot down. According to the BDFL in https://github.com/kennethreitz/requests/issues/1130, the "correct" way to implement timeouts over a set of requests is to overwrite the transport adapter. I'm very hesitant about putting such code in simple-requests, as it would require 100 lines of copy&paste from https://github.com/kennethreitz/requests/blob/master/requests/adapters.py#L294.

[EmilStenstrom]

Maybe you could just expose the adapter interface to us, so we can do the copy-pasting on our side instead?

[arski]

First of all, confirming that your approach for verify works indeed, that's great.

As for timeout can't we pass it as a parameter to simple-requests somewhere and then use it when it calls send() as I did in my branch?

[ctheiss]

Ok here's the recipe to make timeout work:

from simple_requests import Requests
requests = Requests()

# No default timeout
print(requests.one('https://httpbin.org/delay/3').content)

# Default timeout implementation
from requests.adapters import HTTPAdapter
class _DefaultTimeoutHTTPAdapter(HTTPAdapter):
    def send(self, request, stream=False, timeout=None, verify=True, cert=None, proxies=None):
        if timeout is None and hasattr(self, 'defaultTimeout'):
            timeout = self.defaultTimeout
        return super(_DefaultTimeoutHTTPAdapter, self).send(request, stream = stream, timeout = timeout, verify = verify, cert = cert, proxies = proxies)

adapter = _DefaultTimeoutHTTPAdapter()
requests.session.mount('http://', adapter)
requests.session.mount('https://', adapter)

# Set the timeout to one second
adapter.defaultTimeout = 1.0

# Timeout error
print(requests.one('https://httpbin.org/delay/3').content)

This isn't really "simple", but it is the recommended approach to setting a default timeout. And it works in the current version of simple-requests. Let me mull a bit about the best approach to making this "simple".

[ctheiss]

Added defaultTimeout parameter to Requests.__init__. This timeout will apply to all requests sent through this instance. It can also be changed at any time by setting the Requests.defaultTimeout property.

Available as of 1.1.0.

[arski]

perfect, thanks a lot!