Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Connect timeout #1801

Closed
wants to merge 8 commits into from

7 participants

@kevinburke

Per discussion with @sigmavirus24 in IRC, this PR kills the Timeout class and adds support for connect timeouts via a simple tuple interface:

r = requests.get('https://api.twilio.com', timeout=(3.05, 27))

Sometimes you try to connect to a dead host (this happens to us all the time, because of AWS) and you would like to fail fairly quickly in this case; the request has no chance of succeeding. However, once the connection succeeds, you'd like to give the server a fairly long time to process the request and return a response.

The attached tests and documentation have more information. Hope this is okay!

Kevin Burke added some commits
Kevin Burke merge conflicts in utils.py and the tests 3058c99
Kevin Burke Add docs for Timeout, structures class d49b494
Kevin Burke Migrate connect timeout interface to use a tuple
Also:

- add comment to Makefile explaining how to run only one test
- remove classes implementing Timeout class
- add tests for invalid timeouts
fb11b9a
Kevin Burke Add timeout documentation 5538173
Kevin Burke period 2d87793
@sigmavirus24
Collaborator

I'm generally +1 on the idea and the API. I haven't combed over the diff though and a quick skim makes me think there's more there than needs to be. Also, did I see "TimeoutSauce"? Is there any reason to not simply call it "Timeout"?

@Lukasa
Collaborator

In principle I'm +0.5 on this. It's a nice extension. However, as memory serves, @kennethreitz considered this approach and ended up not taking it. It might be that now that someone has written the code he'll be happy with it, but equally, it might be that he doesn't like the approach.

@sigmavirus24 Yeah, TimeoutSauce is used for the urllib3 Timeout object, because we have our own Timeout object (an exception).

@sigmavirus24
Collaborator

@Lukasa As I understood it @kennethreitz was more concerned with the addition (and requirement) of the Timeout class from urllib3. And thanks for clearing up the naming, I still think there has to be a better name for it. (I'm shaving a yak, I know)

@kevinburke

@sigmavirus24 That was my understanding from IRC as well.

@sigmavirus24
Collaborator

@kevinburke I discussed it with you on IRC so the likelihood is that you came to that conclusion through me.

@kennethreitz

I'd rather us have our current interface and just have it support the intended use case, personally. You don't have to tweak with a bunch of timeouts when you're using a web browser :)

@kennethreitz

The main reason for this is because we have the tuple interface in another spot (e.g. an expansion of the file upload api) and it's my least favorite part of the API.

@kennethreitz

I'm not against the class either, I just need to brew about it for a bit, basically.

@kennethreitz

I was under the impression that your concern was for the streaming API, not for more standard uses like GET?

@kevinburke
@kennethreitz

Perhaps we can make this an option on the connection adapter itself. That was the intention for this interface.

@Lukasa
Collaborator

That's no unreasonable, the HTTPAdapter could happily take these as parameters/properties.

@kevinburke

Yeah the HTTPAdapter seems like a good place for this. To try and avoid more go-rounds, what should the interface look like on the Adapter? so far we've proposed

  • a Timeout class - seems the least popular
  • a tuple (connect, read) which is also not implemented in very many other places in the library
  • separate parameters - a timeout parameter would apply to both, a connect_timeout param to the connect and a read_timeout param to the read.
@kevinburke

Ping :)

@Lukasa
Collaborator

Hmm. I'd rather not do a Timeout class, I'd prefer the optional tuple I think. But hold off until @kennethreitz gets another chance to look at this.

@Lukasa Lukasa referenced this pull request
Merged

The timeout is in seconds. #1923

@p2
p2 commented

I found this because my Flask app currently tries to connect to a dead host for which I've set a timeout of 5 seconds but it takes forever to actually time out. What is the status on this one?

@Lukasa
Collaborator

Right now we do apply the timeout to connections, we just don't have it configurable. Are you using stream=True?

@p2
p2 commented

Hadn't been using stream=True, I guess I should use it if only to get the timeout? Just did a quick test with the following script (the host in the test, http://pillbox.nlm.nih.gov, is still down), and with stream=True it does timeout after 5 seconds, without it runs anywhere from 20 to 120 seconds—not what I would expect.

import requests

url = 'http://pillbox.nlm.nih.gov/assets/large/abc.jpg'
requests.get(url, timeout=5)

Using requests 2.2.1 with Python 3.3.3

@Lukasa
Collaborator

That's weird, it works fine on Python 2.7. Seems like a Python 3 bug, because I can reproduce your problem in 3.4. @kevinburke, are you aware of any timeout bugs in urllib3?

@kevinburke
@Lukasa
Collaborator

Weird, I'm not seeing that happen in Python 2.7 then. We'll need to investigate this inconsistency.

@kirelagin

Hm, we definitely need a sane way of configuring both timeouts, and I think that the tuple-approach is nice. requests is for humans and humans don't really like all that extra classes and additional keyword arguments mess.

By the way, speaking about hosts that are down, I just did a test with

import requests

requests.get('http://google.com:81/', timeout=5)

and I get 35 seconds both on Python 2.7 and 3.3 (requests 2.2.1). That's not what I would expect from timeout=5… And with timeout=1 I get 7 seconds. I mean, we really need a sane interface…

@Lukasa
Collaborator

@kirelagin I see the same behaviour as you, but I believe this to be a socket-level problem. Dumping Wireshark shows that my OS X box makes five independent attempts to connect. Each of those connection attempts only retransmits for five seconds.

I suspect this behaviour comes down to the fact that httplib uses socket.create_connection, not socket.connect(). Python's socket module documentation has this to say (emphasis mine):

This is a higher-level function than socket.connect(): if host is a non-numeric hostname, it will try to resolve it for both AF_INET and AF_INET6, and then try to connect to all possible addresses in turn until a connection succeeds.

Closer examination of Wireshark trace shows that we are definitely hitting different addresses: five of them.

If we wanted to 'fix' this behaviour (more on those scare quotes in a minute) it would be incredibly complicated: we'd end up needing to either circumvent httplib's connection logic or use signals or some form of interrupt-processing to return control to us after a given timeout.

More generally, I don't know what 'fix' would mean here. This behaviour is naively surprising (you said we'd time out after 5 seconds but it took 30!), but makes sense once you understand what's happening. I believe that this is an 'expert friendly' interface (thanks to Nick Coghlan for the term), so I'm prepared to believe that we should change it. With that said, if we do change it, how do we give the 'I want to wait 5 seconds for each possible target' behaviour to expert users?

@kevinburke
@Lukasa
Collaborator

Uh, is there? Where? I can't see it...

@kirelagin

Ah, yeah, that makes sense.

five independent attempts

different addresses: five of them.

Wait, google.com has six A records (plus one AAAA, that's why I see 35=5*7 seconds, I guess).

Anyway, I just checked Firefox and it is trying exactly one IPv6 and exactly one IPv4 address. I believe, that multiple DNS records are mostly used for load balancing, not fault-tolerance, so attempting only the first address by default makes most sense. Having an option to control this is useful, of course.

@p2
p2 commented

Is there need to be able to specify both timeouts independently? When I specify the timeout, I'm thinking of it as "I don't want this line of code to run longer than x seconds", I don't care which part of the connection takes how long.

It seems to me this would be a true "human" interpretation and could be implemented without having to rely on urllib3 by an internal timer that kills the request if it hasn't returned within the timeout.

@kevinburke

@lukasa shazow/urllib3#326 ; though now that I read it more carefully, if the OS itself is trying each DNS record in turn then there's not much that can be done. That pull request lets you specify the number of times you would like to retry a connection failure, whether a timeout or an error.

@kevinburke

@p2 Sadly computing the wall clock time for an HTTP request remains incredibly difficult. Mostly, our tools for determining the amount of time used for system calls like DNS resolution, establishing a socket, and sending data (and then passing this information to the necessary places, and subtracting these from a total amount of time) remains difficult.

My suggestion would be to run your HTTP request in a separate thread, then use a timer to cancel the thread if it takes longer than a stated amount of time to return a value. gevent can be used for this: http://www.gevent.org/gevent.html#timeouts

Sorry I can't be more helpful :(

@p2
p2 commented

@kevinburke That is kind of what I do now, I was just wondering if this would make sense as a default approach for requests as well. I personally don't have a need to specify individual timeouts, but that assumption may be too naïve.

@kirelagin

@p2 I agree that “human” interpretation is to have a total timeout. And, by the way, that's how requests works right now. But there, still might be possibilities when you want a more fine-grained control.

@kirelagin

Also, as a human when I'm telling a line of code “go, try to connect to Google with this timeout” I'm not thinking about multiple DNS A-records. I'm thinking of Google as a single entity. So there are, naturally, two sane options:

  1. Do not attempt multiple IPs. If some library code does this, I consider this code broken. If some OS does this, I consider the OS poor.
  2. Do whatever library/OS wants, but have timeout guard total execution time of all the queries. That's totally weird, but somewhat reasonable and definitely more natural than what's happening now…
@Lukasa
Collaborator

Ok, a lot of conversation happened here, let me deal with it in turn.

Anyway, I just checked Firefox and it is trying exactly one IPv6 and exactly one IPv4 address. I believe, that multiple DNS records are mostly used for load balancing, not fault-tolerance, so attempting only the first address by default makes most sense.

You checked Firefox against actual google.com then, not on an incorrect port. Browsers will also fall back to the other addresses if the first doesn't respond. This makes sense. Having multiple A records means "this host is available at these addresses". If I can't contact that host at one of those addresses, it's nonsensical to say "welp, clearly the host is down" when I know several other addresses I might be able to contact them at.

This feature of 'multiple addresses' is widely used for both balancing load and fault tolerance. In fact, if you really want to balance load then DNS SRV is the mechanism to use, not A/AAAA, as it provides better control over how the load is spread.

Is there need to be able to specify both timeouts independently? When I specify the timeout, I'm thinking of it as "I don't want this line of code to run longer than x seconds", I don't care which part of the connection takes how long.

The short answer is 'yes', because of the stream keyword argument. If you've set stream=True and use iter_content() or iter_lines(), it's useful to be able to set a timeout for how long those calls can block.

It seems to me this would be a true "human" interpretation and could be implemented without having to rely on urllib3 by an internal timer that kills the request if it hasn't returned within the timeout.

As @kevinburke points out, this isn't as easy as it seems. More importantly, it also leaves us exposed to implementation details. 'Until the request returns' is not a well-defined notion. What does it mean for the request to return? Do I have to download the whole body? Just the headers? Just the status line? Whatever we choose is going to be utterly arbitrary.

Also, as a human when I'm telling a line of code “go, try to connect to Google with this timeout” I'm not thinking about multiple DNS A-records. I'm thinking of Google as a single entity.

Agreed.

  1. Do not attempt multiple IPs. If some library code does this, I consider this code broken. If some OS does this, I consider the OS poor.

Woah, now you go off the rails. If you are thinking of Google as a single entity, then you would expect us to connect to it if it's up. If one time in seven we fail to connect, even though you always connect fine in your browser, you're going to assume requests is bugged as hell.

If a host is up, we must be able to connect you to it.


The ideal fix, from my position, would be to take over the logic used in socket.create_connection(). This allows us to have fine-grained control over timeouts. Unfortunately, it also complicates the timeout logic further, as you'd now have per-host connection attempt timeouts, total connection attempt timeout, and read timeout. That's beginning to become hugely complicated and to expose people to the complexities of TCP and IP in a way that I'm not delighted about.

@kennethreitz

Plans have been made. Expect something soon ;)

@ralphbean ralphbean referenced this pull request from a commit in fedora-infra/supybot-fedora
@ralphbean ralphbean Add a new command to list GH pull requests.
The command looks like this:

```
threebean │ .pulls
   zodbot │ threebean: (pulls <username/repo>) -- List the pending pull requests on a github repo.
threebean │ .pulls fedora-infra
   zodbot │ threebean: Must be GitHub repo of the format <username/repo>
threebean │ .pulls fedora-infra/tahrir
   zodbot │ threebean: @hroncok's "Reenable gravatar fallback" fedora-infra/tahrir#209
threebean │ .pulls fedora-watwat/tahrir
   zodbot │ threebean: No such GitHub repository 'fedora-watwat/tahrir'
threebean │ .pulls kennethreitz/requests
   zodbot │ threebean: @dpursehouse's "Linkify Github usernames in authors list" kennethreitz/requests#2140
   zodbot │ threebean: @jamielennox's "Apply environment features to prepared requests" kennethreitz/requests#2129
   zodbot │ threebean: @alekstorm's "Create Request.links_multi and utils function for properly parsing Link headers" kennethreitz/requests#1980
   zodbot │ threebean: @kevinburke's "Connect timeout" kennethreitz/requests#1801
```

It requires that we add an ``github.oauth_token`` to the zodbot config which we
can get from https://github.com/settings/tokens/new
3fa47f0
@kennethreitz

@kevinburke can you rebase?

@kevinburke

Closing this one, I re-added it in #2176

@kevinburke kevinburke closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 16, 2013
  1. merge conflicts in utils.py and the tests

    Kevin Burke authored
  2. Add docs for Timeout, structures class

    Kevin Burke authored
  3. Migrate connect timeout interface to use a tuple

    Kevin Burke authored
    Also:
    
    - add comment to Makefile explaining how to run only one test
    - remove classes implementing Timeout class
    - add tests for invalid timeouts
  4. Add timeout documentation

    Kevin Burke authored
  5. period

    Kevin Burke authored
Commits on Feb 16, 2014
  1. @colons

    Be super specific about the units of timeouts.

    colons authored
    Because kennethreitz/requests#1922 bothers me *that much*.
  2. @colons
  3. Merge pull request #1 from colons/connect-timeout

    Kevin Burke authored
    Be super specific about the units of timeouts.
This page is out of date. Refresh to see the latest.
View
1  Makefile
@@ -5,6 +5,7 @@ init:
test:
py.test
+ # Or, run an individual test with -k, like py.test -k invalid_timeout
coverage:
py.test --verbose --cov-report term --cov=requests test_requests.py
View
2  docs/api.rst
@@ -5,7 +5,7 @@ Developer Interface
.. module:: requests
-This part of the documentation covers all the interfaces of Requests. For
+This part of the documentation covers all the interfaces of Requests. For
parts where Requests depends on external libraries, we document the most
important right here and provide links to the canonical documentation.
View
16 docs/dev/internals.rst
@@ -0,0 +1,16 @@
+.. _internals:
+
+Requests Internals
+==================
+
+This part of the documentation covers some data structures that are used by
+Requests.
+
+Data Structures
+---------------
+
+.. autoclass:: requests.structures.CaseInsensitiveDict
+ :members:
+
+.. autoclass:: requests.structures.LookupDict
+ :members:
View
38 docs/user/advanced.rst
@@ -626,3 +626,41 @@ Two excellent examples are `grequests`_ and `requests-futures`_.
.. _`grequests`: https://github.com/kennethreitz/grequests
.. _`requests-futures`: https://github.com/ross/requests-futures
+
+Timeouts
+--------
+
+Most external requests should have a timeout attached, in case the server is
+not responding in a timely manner.
+
+The **connect** timeout is the number of seconds Requests will wait for your
+client to establish a connection to a remote machine (corresponding to the
+`connect()`_) call on the socket. It's a good practice to set connect timeouts
+to slightly larger than a multiple of 3, to allow for the default `TCP
+retransmission window <http://www.hjp.at/doc/rfc/rfc2988.txt>`_.
+
+Once your client has connected to the server and sent the HTTP request, the
+**read** timeout is the number of seconds the client will wait for the server
+to send a response. (Specifically, it's the number of seconds that the client
+will wait *between* bytes sent from the server. In practice, this is the time
+before the server sends the first byte).
+
+If you specify a single value for the timeout, like this::
+
+ r = requests.get('https://github.com', timeout=5)
+
+The timeout value will be applied to both the ``connect`` and the ``read``
+timeouts. Specify a tuple if you would like to set the values separately::
+
+ r = requests.get('https://github.com', timeout=(3.05, 27))
+
+If the remote server is very slow, you can tell Requests to wait forever for it
+to respond, by passing None as a timeout value.
+
+.. code-block:: python
+
+ r = requests.get('https://github.com', timeout=None)
+
+.. _`connect()`: http://linux.die.net/man/2/connect
+
+
View
28 requests/adapters.py
@@ -18,13 +18,15 @@
from .utils import (DEFAULT_CA_BUNDLE_PATH, get_encoding_from_headers,
except_on_missing_scheme, get_auth_from_url)
from .structures import CaseInsensitiveDict
-from .packages.urllib3.exceptions import MaxRetryError
-from .packages.urllib3.exceptions import TimeoutError
-from .packages.urllib3.exceptions import SSLError as _SSLError
+from .packages.urllib3.exceptions import ConnectTimeoutError
from .packages.urllib3.exceptions import HTTPError as _HTTPError
+from .packages.urllib3.exceptions import MaxRetryError
from .packages.urllib3.exceptions import ProxyError as _ProxyError
+from .packages.urllib3.exceptions import ReadTimeoutError
+from .packages.urllib3.exceptions import SSLError as _SSLError
from .cookies import extract_cookies_to_jar
-from .exceptions import ConnectionError, Timeout, SSLError, ProxyError
+from .exceptions import (ConnectionError, ConnectTimeout, ReadTimeout, SSLError,
+ ProxyError)
from .auth import _basic_auth_str
DEFAULT_POOLBLOCK = False
@@ -291,6 +293,7 @@ def send(self, request, stream=False, timeout=None, verify=True, cert=None, prox
:param request: The :class:`PreparedRequest <PreparedRequest>` being sent.
:param stream: (optional) Whether to stream the request content.
:param timeout: (optional) The timeout on the request.
+ :type timeout: float or tuple (connect timeout, read timeout), e.g. (3.05, 25)
:param verify: (optional) Whether to verify SSL certificates.
:param cert: (optional) Any user-provided SSL certificate to be trusted.
:param proxies: (optional) The proxies dictionary to apply to the request.
@@ -304,8 +307,15 @@ def send(self, request, stream=False, timeout=None, verify=True, cert=None, prox
chunked = not (request.body is None or 'Content-Length' in request.headers)
- if stream:
- timeout = TimeoutSauce(connect=timeout)
+ if isinstance(timeout, tuple):
+ try:
+ connect, read = timeout
+ except ValueError:
+ err = ("Invalid timeout {0}. Pass a (connect, read) "
+ "timeout tuple, or a single float to set "
+ "both timeouts to the same value".format(timeout))
+ raise ValueError(err)
+ timeout = TimeoutSauce(connect=connect, read=read)
else:
timeout = TimeoutSauce(connect=timeout, read=timeout)
@@ -377,8 +387,10 @@ def send(self, request, stream=False, timeout=None, verify=True, cert=None, prox
except (_SSLError, _HTTPError) as e:
if isinstance(e, _SSLError):
raise SSLError(e)
- elif isinstance(e, TimeoutError):
- raise Timeout(e)
+ elif isinstance(e, ConnectTimeoutError):
+ raise ConnectTimeout(e)
+ elif isinstance(e, ReadTimeoutError):
+ raise ReadTimeout(e)
else:
raise
View
14 requests/exceptions.py
@@ -36,8 +36,20 @@ class SSLError(ConnectionError):
class Timeout(RequestException):
- """The request timed out."""
+ """The request timed out.
+ Catching this error will catch both :exc:`ConnectTimeout` and
+ :exc:`ReadTimeout` errors.
+ """
+
+class ConnectTimeout(Timeout):
+ """ The request timed out while trying to connect to the server.
+
+ This type of request is always safe to retry
+ """
+
+class ReadTimeout(Timeout):
+ """ The server exceeded the expected timeout """
class URLRequired(RequestException):
"""A valid URL is required to make a request."""
View
5 requests/structures.py
@@ -12,7 +12,6 @@
import collections
from itertools import islice
-
class IteratorProxy(object):
"""docstring for IteratorProxy"""
def __init__(self, i):
@@ -48,6 +47,8 @@ class CaseInsensitiveDict(collections.MutableMapping):
will contain case-sensitive keys. However, querying and contains
testing is case insensitive:
+ .. code-block:: python
+
cid = CaseInsensitiveDict()
cid['Accept'] = 'application/json'
cid['aCCEPT'] == 'application/json' # True
@@ -58,7 +59,7 @@ class CaseInsensitiveDict(collections.MutableMapping):
of how the header name was originally stored.
If the constructor, ``.update``, or equality comparison
- operations are given keys that have equal ``.lower()``s, the
+ operations are given keys that have equal ``.lower()`` s, the
behavior is undefined.
"""
View
48 test_requests.py
@@ -16,7 +16,8 @@
from requests.compat import (
Morsel, cookielib, getproxies, str, urljoin, urlparse)
from requests.cookies import cookiejar_from_dict, morsel_to_cookie
-from requests.exceptions import InvalidURL, MissingSchema
+from requests.exceptions import (InvalidURL, MissingSchema, ConnectTimeout,
+ ReadTimeout)
from requests.structures import CaseInsensitiveDict
try:
@@ -28,6 +29,9 @@
# Issue #1483: Make sure the URL always has a trailing slash
HTTPBIN = HTTPBIN.rstrip('/') + '/'
+# We need a host that will not immediately close the connection with a TCP
+# Reset. SO suggests this hostname
+TARPIT_HOST = 'http://10.255.255.1'
def httpbin(*suffix):
"""Returns url for HTTPBIN resource."""
@@ -194,7 +198,7 @@ def test_generic_cookiejar_works(self):
assert r.json()['cookies']['foo'] == 'bar'
# Make sure the session cj is still the custom one
assert s.cookies is cj
-
+
def test_param_cookiejar_works(self):
cj = cookielib.CookieJar()
cookiejar_from_dict({'foo' : 'bar'}, cj)
@@ -577,6 +581,46 @@ def test_time_elapsed_blank(self):
* 10**6) / 10**6)
assert total_seconds > 0.0
+ def test_invalid_timeout(self):
+ with pytest.raises(ValueError) as e:
+ requests.get(httpbin('get'), timeout=(3, 4, 5))
+ assert '(connect, read)' in str(e)
+
+ with pytest.raises(ValueError) as e:
+ requests.get(httpbin('get'), timeout="foo")
+ assert 'must be an int or float' in str(e)
+
+ def test_none_timeout(self):
+ """ Check that you can set None as a valid timeout value.
+
+ To actually test this behavior, we'd want to check that setting the
+ timeout to None actually lets the request block past the system default
+ timeout. However, this would make the test suite unbearably slow.
+ """
+ r = requests.get(httpbin('get'), timeout=None)
+ assert r.status_code == 200
+
+ def test_read_timeout(self):
+ try:
+ requests.get(httpbin('delay/10'), timeout=(None, 0.1))
+ assert False, "The recv() request should time out."
+ except ReadTimeout:
+ pass
+
+ def test_connect_timeout(self):
+ try:
+ requests.get(TARPIT_HOST, timeout=(0.1, None))
+ assert False, "The connect() request should time out."
+ except ConnectTimeout:
+ pass
+
+ def test_total_timeout_connect(self):
+ try:
+ requests.get(TARPIT_HOST, timeout=(0.1, 0.1))
+ assert False, "The connect() request should time out."
+ except ConnectTimeout:
+ pass
+
def test_response_is_iterable(self):
r = requests.Response()
io = StringIO.StringIO('abc')
Something went wrong with that request. Please try again.