-
Notifications
You must be signed in to change notification settings - Fork 195
Add connection/read timeout for requests adapter #342
Changes from 7 commits
153cfac
1e7739a
7be26b6
8c310c7
28c5699
1417c51
09dfa86
f0bc05c
ead832b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,6 +110,18 @@ def test_initialization_with_ipv6_addresses_proxy_inline_port(self): | |
assert c.proxy_host == 'ffff:aaaa::1' | ||
assert c.proxy_port == 8443 | ||
|
||
def test_initialization_timeout(self): | ||
c = HTTP11Connection('httpbin.org', timeout=30) | ||
|
||
assert c._connect_timeout == 30 | ||
assert c._read_timeout == 30 | ||
|
||
def test_initialization_tuple_timeout(self): | ||
c = HTTP11Connection('httpbin.org', timeout=(5, 60)) | ||
|
||
assert c._connect_timeout == 5 | ||
assert c._read_timeout == 60 | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need these tests for HTTP/2 as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok,I add this in test/test_hyper.py |
||
def test_basic_request(self): | ||
c = HTTP11Connection('httpbin.org') | ||
c._sock = sock = DummySocket() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
import hyper | ||
import hyper.http11.connection | ||
import pytest | ||
from socket import timeout as SocketTimeout | ||
from contextlib import contextmanager | ||
from mock import patch | ||
from concurrent.futures import ThreadPoolExecutor, TimeoutError | ||
|
@@ -1230,6 +1231,104 @@ def do_connect(conn): | |
|
||
self.tear_down() | ||
|
||
def test_connection_timeout(self): | ||
self.set_up(timeout=0.5) | ||
|
||
def socket_handler(listener): | ||
time.sleep(1) | ||
sock = listener.accept()[0] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No point calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it, I just updated |
||
sock.close() | ||
|
||
self._start_server(socket_handler) | ||
conn = self.get_connection() | ||
try: | ||
conn.connect() | ||
except (SocketTimeout, ssl.SSLError): | ||
# Py2 raises this as a BaseSSLError, | ||
# Py3 raises it as socket timeout. | ||
# assert 'timed out' in e.message | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably we don't need the |
||
pass | ||
else: | ||
assert False | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry, i'm not familiar with pytest before, i replace it with pytest.raises or pytest.fail There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
self.tear_down() | ||
|
||
def test_read_timeout(self): | ||
self.set_up(timeout=0.5) | ||
|
||
req_event = threading.Event() | ||
|
||
def socket_handler(listener): | ||
sock = listener.accept()[0] | ||
|
||
# We get two messages for the connection open and then a HEADERS | ||
# frame. | ||
receive_preamble(sock) | ||
sock.recv(65535) | ||
|
||
# Wait for request | ||
req_event.wait(5) | ||
|
||
# Sleep wait for read timeout | ||
time.sleep(1) | ||
|
||
sock.close() | ||
|
||
self._start_server(socket_handler) | ||
conn = self.get_connection() | ||
conn.request('GET', '/') | ||
req_event.set() | ||
|
||
try: | ||
conn.get_response() | ||
except (SocketTimeout, ssl.SSLError): | ||
# Py2 raises this as a BaseSSLError, | ||
# Py3 raises it as socket timeout. | ||
# assert 'timed out' in e.message | ||
pass | ||
else: | ||
assert False | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
self.tear_down() | ||
|
||
def test_default_connection_timeout(self): | ||
self.set_up(timeout=None) | ||
|
||
# Confirm that we send the connection upgrade string and the initial | ||
# SettingsFrame. | ||
data = [] | ||
send_event = threading.Event() | ||
|
||
def socket_handler(listener): | ||
time.sleep(1) | ||
sock = listener.accept()[0] | ||
|
||
# We should get one big chunk. | ||
first = sock.recv(65535) | ||
data.append(first) | ||
|
||
# We need to send back a SettingsFrame. | ||
f = SettingsFrame(0) | ||
sock.send(f.serialize()) | ||
|
||
send_event.set() | ||
sock.close() | ||
|
||
self._start_server(socket_handler) | ||
conn = self.get_connection() | ||
try: | ||
conn.connect() | ||
except (SocketTimeout, ssl.SSLError): | ||
# Py2 raises this as a BaseSSLError, | ||
# Py3 raises it as socket timeout. | ||
assert False | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
send_event.wait(5) | ||
|
||
assert data[0].startswith(b'PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How can this possibly pass? If the connection fails, we're never going to see this data. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. default timeout There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, sorry, I see that. Thanks! |
||
|
||
self.tear_down() | ||
|
||
|
||
@patch('hyper.http20.connection.H2_NPN_PROTOCOLS', PROTOCOLS) | ||
class TestRequestsAdapter(SocketLevelTest): | ||
|
@@ -1290,7 +1389,8 @@ def socket_handler(listener): | |
|
||
s = requests.Session() | ||
s.mount('https://%s' % self.host, HTTP20Adapter()) | ||
r = s.get('https://%s:%s/some/path' % (self.host, self.port)) | ||
r = s.get('https://%s:%s/some/path' % (self.host, self.port), | ||
timeout=(10, 60)) | ||
|
||
# Assert about the received values. | ||
assert r.status_code == 200 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably add this to the common
HTTPConnection
object as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok,I just added