Add connection/read timeout for requests adapter #342

hyxbiao · 2017-07-24T10:50:48Z

[+] Add connection/read timeout for requests adapter, we can use like this:

import requests
from hyper.contrib import HTTP20Adapter
s = requests.Session()
s.mount('https://http2bin.org', HTTP20Adapter())
r = s.get('https://http2bin.org/get', timeout=30)
print(r.status_code)

Lukasa

Thanks for this! I've left a note inline. It'd also be really useful to actually add tests for the timeout functionality that we're implicitly also adding to our lower-level classes.

Lukasa · 2017-07-24T11:44:10Z

hyper/contrib.py

@@ -77,7 +77,8 @@ def get_connection(self, host, port, scheme, cert=None, verify=True,
                secure=secure,
                ssl_context=ssl_context,
                proxy_host=proxy_netloc,
-                proxy_headers=proxy_headers)
+                proxy_headers=proxy_headers,
+                **kwargs)


We shouldn't be sending arbitrary kwargs from Requests to hyper: there's no reason to assume that those kwargs will match in any sensible way. We should aim to be explicit here.

OK, I had change to explicit timeout and add more timeout tests

KostyaEsmukov

Thanks for this! I was going to work on timeouts this week but you have started earlier :)

I have one more note on this: the _create_tunnel function of http11 module should respect the timeout too.

PS: Related issue: #187

KostyaEsmukov · 2017-07-24T13:30:45Z

hyper/http11/connection.py

@@ -150,6 +150,15 @@ def __init__(self, host, port=None, secure=None, ssl_context=None,
        #: the standard hyper parsing interface.
        self.parser = Parser()

+        # timeout
+        timeout = kwargs.get('timeout')


timeout should be optional. I think it should be None - that means no timeout.

Yes, timeout is optional and default None, is equivalent to socket.setblocking(1)

hyxbiao · 2017-07-25T02:17:59Z

timeout is optional, the default value is None, means that it will block connection/read, it is the same as request/adapters.py:
def send(self, request, stream=False, timeout=None, verify=True, cert=None, proxies=None):

if isinstance(timeout, tuple):
    try:
        connect, read = timeout
        timeout = TimeoutSauce(connect=connect, read=read)
    except ValueError as e:
        # this may raise a string formatting error.
        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)
else:
    timeout = TimeoutSauce(connect=timeout, read=timeout)

it also can set timeout to socket._GLOBAL_DEFAULT_TIMEOUT

Lukasa · 2017-07-25T06:45:21Z

hyper/contrib.py

@@ -92,13 +93,15 @@ def send(self, request, stream=False, cert=None, verify=True, proxies=None,
            proxy = prepend_scheme_if_needed(proxy, 'http')

        parsed = urlparse(request.url)
+        timeout = kwargs.get('timeout')


Let's just add timeout to our list of kwargs.

Lukasa · 2017-07-25T06:46:40Z

test/test_integration.py

+        conn = self.get_connection()
+        try:
+            conn.connect()
+            assert False


This should be pytest.fail, and generally should go in an else block.

Lukasa · 2017-07-25T06:47:30Z

test/test_integration.py

+
+            # Wait for request
+            req_event.wait(5)
+            # Now, send the headers for the response. This response has no body


This comment is misleading.

Lukasa · 2017-07-25T06:48:07Z

test/test_integration.py

+            time.sleep(1)
+
+            # Now, send the headers for the response. This response has no body
+            f = build_headers_frame(


We never actually need to send this, and doing so could cause exceptions, so let's not send any data here.

Lukasa · 2017-07-25T06:48:31Z

test/test_integration.py

+
+        try:
+            conn.get_response()
+            assert False


Same note about pytest.fail and else blocks.

Lukasa · 2017-07-25T06:49:28Z

test/test_integration.py

+
+        send_event.wait(5)
+
+        assert data[0].startswith(b'PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n')


How can this possibly pass? If the connection fails, we're never going to see this data.

default timeout None will block connect, so it will connect sucess and go on

Ah, sorry, I see that. Thanks!

Lukasa · 2017-07-25T06:50:00Z

test/test_integration.py

+
+        def socket_handler(listener):
+            time.sleep(1)
+            sock = listener.accept()[0]


No point calling accept: it'll usually fail, and throw exceptions, which we don't want.

if connection timeout smaller than 1s, it will throw timeout exception, in this test connection timeout is set to 0.5, so it will throw exception as expect

Right, but this exception is not caught, meaning it'll be logged and generally treated badly. We shouldn't do anything in the background thread that we know will fail, and accept will fail here.

got it, I just updated

Lukasa · 2017-07-25T14:14:26Z

hyper/http11/connection.py

@@ -101,7 +101,7 @@ class HTTP11Connection(object):

    def __init__(self, host, port=None, secure=None, ssl_context=None,
                 proxy_host=None, proxy_port=None, proxy_headers=None,
-                 **kwargs):
+                 timeout=None, **kwargs):


We should probably add this to the common HTTPConnection object as well.

ok，I just added

Lukasa · 2017-07-25T14:17:28Z

test/test_http11.py

+
+        assert c._connect_timeout == 5
+        assert c._read_timeout == 60
+


We need these tests for HTTP/2 as well.

ok，I add this in test/test_hyper.py

Lukasa · 2017-07-25T14:18:14Z

test/test_integration.py

+
+        def socket_handler(listener):
+            time.sleep(1)
+            sock = listener.accept()[0]


Right, but this exception is not caught, meaning it'll be logged and generally treated badly. We shouldn't do anything in the background thread that we know will fail, and accept will fail here.

Lukasa · 2017-07-25T14:18:24Z

test/test_integration.py

+            # assert 'timed out' in e.message
+            pass
+        else:
+            assert False


pytest.fail, please.

sorry, i'm not familiar with pytest before, i replace it with pytest.raises or pytest.fail

pytest.raises is the best thing to do if you don't plan to do anything in the except block.

Lukasa · 2017-07-25T14:18:40Z

test/test_integration.py

+        except (SocketTimeout, ssl.SSLError):
+            # Py2 raises this as a BaseSSLError,
+            # Py3 raises it as socket timeout.
+            # assert 'timed out' in e.message


Probably we don't need the assert statement in the comment?

Lukasa · 2017-07-25T14:18:59Z

test/test_integration.py

+            # assert 'timed out' in e.message
+            pass
+        else:
+            assert False


pytest.fail, please.

Lukasa · 2017-07-25T14:19:15Z

test/test_integration.py

+        except (SocketTimeout, ssl.SSLError):
+            # Py2 raises this as a BaseSSLError,
+            # Py3 raises it as socket timeout.
+            assert False


pytest.fail, please.

Lukasa · 2017-07-25T14:19:23Z

test/test_integration.py

+
+        send_event.wait(5)
+
+        assert data[0].startswith(b'PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n')


Ah, sorry, I see that. Thanks!

Lukasa · 2017-07-26T07:54:25Z

hyper/common/connection.py

@@ -78,8 +79,10 @@ def __init__(self,
        self._h1_kwargs.update(kwargs)
        self._h2_kwargs.update(kwargs)

+        self._timeout = timeout


This should go into _h1_kwargs and _h2_kwargs: it'll also need associated tests.

ok, it's done

…; add more tests

hyxbiao · 2017-07-26T12:18:46Z

update information:
add timeout for _create_tunnel funciton, almost forget it
move timeout to _h1/2_kwargs in hyper/common/connection.py
add more timeout tests

Lukasa

Very nicely done, thank you!

hyxbiao and others added 3 commits July 24, 2017 18:42

Add connection/read timeout for requests adapter

153cfac

Add connection/read timeout for requests adapter

1e7739a

update requests integration test

7be26b6

Lukasa suggested changes Jul 24, 2017

View reviewed changes

KostyaEsmukov suggested changes Jul 24, 2017

View reviewed changes

explicit timeout and more timeout tests

8c310c7

hyxbiao added 2 commits July 25, 2017 10:30

update timeout tests for py3

28c5699

update timeout tests for py3

1417c51

Lukasa suggested changes Jul 25, 2017

View reviewed changes

explicit timeout in send method and update timeout tests

09dfa86

Lukasa suggested changes Jul 25, 2017

View reviewed changes

add timeout in common/HTTPConnection and update timeout tests

f0bc05c

Lukasa suggested changes Jul 26, 2017

View reviewed changes

move timeout in _h1/2_kwargs; add timeout for _create_tunnel funciton…

ead832b

…; add more tests

Lukasa approved these changes Jul 27, 2017

View reviewed changes

Lukasa merged commit 75f25f7 into python-hyper:development Jul 27, 2017


		send_event.wait(5)

		assert data[0].startswith(b'PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n')

Add connection/read timeout for requests adapter #342

Add connection/read timeout for requests adapter #342

Uh oh!

Conversation

hyxbiao commented Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KostyaEsmukov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hyxbiao commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hyxbiao commented Jul 26, 2017

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

hyxbiao commented Jul 24, 2017 •

edited

Loading

hyxbiao commented Jul 25, 2017 •

edited

Loading