Ensure that the user-specified timeout is set before making a request

[poupas]

Hi,

urllib3 apparently does not honor the timeout value when establishing a new connection.

In the code, one can see that the timeout value is being set on the socket, albeit only after the connection attempt.

conn.request(method, url, **httplib_request_kw)
conn.sock.settimeout(timeout)

I believe this partly defeats the purpose of the timeout parameter.

Here's a small script that illustrates this:

import urllib3
conn = urllib3.connection_from_url('http://host.that.times.out', timeout=1.0)
r1 = conn.request('GET', '/')

Results

Without the change:

$ time python urllib3_timeout.py 
Traceback (most recent call last):
 File "urllib3_timeout.py", line 4, in <module>
    r1 = conn.request('GET', '/')
  File "/usr/local/lib/python2.6/dist-packages/urllib3-1.0.2-py2.6.egg/urllib3/request.py", line 65, in request
    **urlopen_kw)
  File "/usr/local/lib/python2.6/dist-packages/urllib3-1.0.2-py2.6.egg/urllib3/request.py", line 78, in request_encode_url
    return self.urlopen(method, url, **urlopen_kw)
  File "/usr/local/lib/python2.6/dist-packages/urllib3-1.0.2-py2.6.egg/urllib3/connectionpool.py", line 361, in urlopen
    redirect, assert_same_host)  # Try again
  File "/usr/local/lib/python2.6/dist-packages/urllib3-1.0.2-py2.6.egg/urllib3/connectionpool.py", line 361, in urlopen
    redirect, assert_same_host)  # Try again
  File "/usr/local/lib/python2.6/dist-packages/urllib3-1.0.2-py2.6.egg/urllib3/connectionpool.py", line 361, in urlopen
    redirect, assert_same_host)  # Try again
  File "/usr/local/lib/python2.6/dist-packages/urllib3-1.0.2-py2.6.egg/urllib3/connectionpool.py", line 361, in urlopen
    redirect, assert_same_host)  # Try again
  File "/usr/local/lib/python2.6/dist-packages/urllib3-1.0.2-py2.6.egg/urllib3/connectionpool.py", line 296, in urlopen
    raise MaxRetryError("Max retries exceeded for url: %s" % url)
urllib3.exceptions.MaxRetryError: Max retries exceeded for url: /

real    1m24.056s
user    0m0.044s
sys 0m0.016s

With the change:

$ time python urllib3_timeout.py 
Traceback (most recent call last):
  File "urllib3_timeout.py", line 4, in <module>
    r1 = conn.request('GET', '/')
  File "/usr/local/lib/python2.6/dist-packages/urllib3-1.0.2-py2.6.egg/urllib3/request.py", line 65, in request
    **urlopen_kw)
  File "/usr/local/lib/python2.6/dist-packages/urllib3-1.0.2-py2.6.egg/urllib3/request.py", line 78, in request_encode_url
    return self.urlopen(method, url, **urlopen_kw)
  File "/usr/local/lib/python2.6/dist-packages/urllib3-1.0.2-py2.6.egg/urllib3/connectionpool.py", line 344, in urlopen
    self.timeout)
urllib3.exceptions.TimeoutError: Request timed out after 1.000000 seconds

real    0m1.040s
user    0m0.020s
sys 0m0.020s

Hope you find the patch useful.

Thank you for your work.

[shazow]

Ah good find. It seems we respect the timeout after establishing the connection, but not if the the establishing step takes too long.

For your specific patch, perhaps it would be better to check if conn.sock is None instead of using hasattr.

But wouldn't this bug still exist if the connection hasn't been created yet, such as on the first attempt?

[poupas]

The reason for using hasattr is that, starting with Python 2.6, httplib supports a timeout parameter in the HTTP(S)Connection constructors, thus solving the first connection attempt issue you describe.

The code in the patch tries to determine if the conn object has the timeout attribute and sets it accordingly. If conn does not support this attribute, which is the case in Python < 2.6, then the behavior remains unchanged.

[shazow]

Ah I see, thank you for the explanation!

Sounds good. Let me think about this for a while to see if we can do this better somehow, if not then I'll merge your patch in next week. :)

Please bug me if it hasn't happened! Thanks again.

[poupas]

Thank you for considering the patch :)

[piotr-dobrogost]

The reason for using hasattr is that, starting with Python 2.6, httplib supports a timeout parameter in the
HTTP(S)Connection constructors, thus solving the first connection attempt issue you describe.

I dont' get it. There's no timeout parameter used when creating new httplib.HTTPConnection in HTTPConnectionPool._new_conn (https://github.com/shazow/urllib3/blob/1.0.2/urllib3/connectionpool.py#L150)

[poupas]

When the class is instantiated, if the timeout parameter is not specified, it is initialized with socket._GLOBAL_DEFAULT_TIMEOUT.

This can be seen in Python's 2.6 httplib.py file:

class HTTPConnection:
(...)
    def __init__(self, host, port=None, strict=None,
                 timeout=socket._GLOBAL_DEFAULT_TIMEOUT):
        self.timeout = timeout
        self.sock = None
(...)

Maybe, specifying the timeout parameter in the HTTPConnection constructor is a more correct approach. However, we must assure that we're running in Python >= 2.6 in order to do this, or else the call will fail. Another option that could work is, in the HTTPConnectionPool._new_conn method:

conn = HTTPConnection(host=self.host, port=self.port)
conn.timeout = self.timeout
return conn

In Python >= 2.6 the timeout value would be set; in Python < 2.6, the timeout attribute would have no effect.

I don't favor any particular solution, as long as it works and you're happy with it :)

[shazow]

I liked your latest version more, so I edited your commit and merged it with your original message/authorship. :)

[poupas]

Cool. Thanks :)

[piotr-dobrogost]

@shazow
Shouldn't that have been

if timeout is _Default:
    timeout = self.timeout
conn.timeout = timeout # This only does anything in Py26+

@poupas
Setting timeout in HTTPConnectionPool._new_conn() is not enough as custom timeout may be passed to HTTPConnectionPool._make_request().

[shazow]

@piotr-dobrogost You're right, fixing.