From 7ffb24e0f316c2a14e99b398d04d5d9126d4c1a1 Mon Sep 17 00:00:00 2001 From: Rober Morales-Chaparro Date: Fri, 11 Jan 2019 21:42:18 +0100 Subject: [PATCH 01/27] Skipping the broken pipe errors. In the case the server legitimally close a connection but we are still sending data (e.g. sending a POST request), the old code prevents the response to be retrieved. With this change, we avoid a broken pipe in the request write process, to end the read process. --- src/urllib3/connection.py | 6 ++++++ src/urllib3/connectionpool.py | 12 ++++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/urllib3/connection.py b/src/urllib3/connection.py index 57c58fedb7..edac6c8053 100644 --- a/src/urllib3/connection.py +++ b/src/urllib3/connection.py @@ -27,6 +27,12 @@ class BaseSSLError(BaseException): class ConnectionError(Exception): pass +try: # Python 3: + # Not a no-op, we're adding this to the namespace so it can be imported. + BrokenPipeError = BrokenPipeError +except NameError: # Python 2: + class BrokenPipeError(Exception): + pass from .exceptions import ( NewConnectionError, diff --git a/src/urllib3/connectionpool.py b/src/urllib3/connectionpool.py index 157568a395..a1ad999fbd 100644 --- a/src/urllib3/connectionpool.py +++ b/src/urllib3/connectionpool.py @@ -31,7 +31,7 @@ port_by_scheme, DummyConnection, HTTPConnection, HTTPSConnection, VerifiedHTTPSConnection, - HTTPException, BaseSSLError, + HTTPException, BaseSSLError, BrokenPipeError ) from .request import RequestMethods from .response import HTTPResponse @@ -352,7 +352,15 @@ def _make_request(self, conn, method, url, timeout=_Default, chunked=False, if chunked: conn.request_chunked(method, url, **httplib_request_kw) else: - conn.request(method, url, **httplib_request_kw) + try: + conn.request(method, url, **httplib_request_kw) + except BrokenPipeError: # Python 3 + pass + except IOError as e: # Python 2 + if e.errno == errno.EPIPE: + pass + else: + raise # Reset the timeout for the recv() on the socket read_timeout = timeout_obj.read_timeout From d4416f0beb98ed093b3880095360336d915efc72 Mon Sep 17 00:00:00 2001 From: Rober Morales-Chaparro Date: Mon, 14 Jan 2019 14:54:41 +0100 Subject: [PATCH 02/27] Included `request_chunked()` in the control. Added a comment with the rationale. --- src/urllib3/connectionpool.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/urllib3/connectionpool.py b/src/urllib3/connectionpool.py index a1ad999fbd..cc3252c2a0 100644 --- a/src/urllib3/connectionpool.py +++ b/src/urllib3/connectionpool.py @@ -349,18 +349,20 @@ def _make_request(self, conn, method, url, timeout=_Default, chunked=False, # conn.request() calls httplib.*.request, not the method in # urllib3.request. It also calls makefile (recv) on the socket. - if chunked: - conn.request_chunked(method, url, **httplib_request_kw) - else: - try: + try: + if chunked: + conn.request_chunked(method, url, **httplib_request_kw) + else: conn.request(method, url, **httplib_request_kw) - except BrokenPipeError: # Python 3 - pass - except IOError as e: # Python 2 - if e.errno == errno.EPIPE: - pass - else: - raise + + # We are shallowing BrokenPipeError (errno.EPIPE) since the server is + # legitimately able to close the connection after sending a valid response. + # With this behaviour, the received response is still readable. + except BrokenPipeError: # Python 3 + pass + except IOError as e: # Python 2 + if e.errno != errno.EPIPE: + raise # Reset the timeout for the recv() on the socket read_timeout = timeout_obj.read_timeout From 3a285b9d908c2ce6dcaa1c5d739b42e7828026bb Mon Sep 17 00:00:00 2001 From: Rober Morales-Chaparro Date: Mon, 14 Jan 2019 20:57:26 +0100 Subject: [PATCH 03/27] Added tests --- dummyserver/handlers.py | 5 ++++- test/with_dummyserver/test_connectionpool.py | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/dummyserver/handlers.py b/dummyserver/handlers.py index 146241dc8d..6e3c791ed9 100644 --- a/dummyserver/handlers.py +++ b/dummyserver/handlers.py @@ -10,7 +10,7 @@ import zlib from io import BytesIO -from tornado.web import RequestHandler +from tornado.web import RequestHandler, HTTPError from tornado import httputil from datetime import datetime from datetime import timedelta @@ -308,5 +308,8 @@ def redirect_after(self, request): headers = [('Location', target), ('Retry-After', retry_after)] return Response(status='303 See Other', headers=headers) + def admin(self, _request): + raise HTTPError(401) + def shutdown(self, request): sys.exit() diff --git a/test/with_dummyserver/test_connectionpool.py b/test/with_dummyserver/test_connectionpool.py index d58b0c4483..560ad4b7ee 100644 --- a/test/with_dummyserver/test_connectionpool.py +++ b/test/with_dummyserver/test_connectionpool.py @@ -1057,5 +1057,13 @@ def test_pool_size_redirect(self): assert self.pool.num_connections == 1 +class TestBrokenPipeIgnore(HTTPDummyServerTestCase): + def setUp(self): + self.pool = HTTPConnectionPool(self.host, self.port) + + def test_broken_pipe_ignore(self): + resp = self.pool.urlopen('POST', '/admin') + assert resp.status == 401 + if __name__ == '__main__': unittest.main() From 5e68837d5c50eae2f0be401f00c4e2bbeaa327b7 Mon Sep 17 00:00:00 2001 From: Rober Morales-Chaparro Date: Tue, 15 Jan 2019 12:20:28 +0100 Subject: [PATCH 04/27] Added tests for the `request_chunked()` case --- test/with_dummyserver/test_connectionpool.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/test/with_dummyserver/test_connectionpool.py b/test/with_dummyserver/test_connectionpool.py index 560ad4b7ee..99d984a571 100644 --- a/test/with_dummyserver/test_connectionpool.py +++ b/test/with_dummyserver/test_connectionpool.py @@ -774,6 +774,13 @@ def test_mixed_case_hostname(self): response = pool.request('GET', "http://LoCaLhOsT:%d/" % self.port) self.assertEqual(response.status, 200) + def test_broken_pipe_ignore(self): + resp = self.pool.urlopen('POST', '/admin', chunked=False) + assert resp.status == 401 + + resp = self.pool.urlopen('POST', '/admin', chunked=True) + assert resp.status == 401 + class TestRetry(HTTPDummyServerTestCase): def setUp(self): @@ -1057,13 +1064,5 @@ def test_pool_size_redirect(self): assert self.pool.num_connections == 1 -class TestBrokenPipeIgnore(HTTPDummyServerTestCase): - def setUp(self): - self.pool = HTTPConnectionPool(self.host, self.port) - - def test_broken_pipe_ignore(self): - resp = self.pool.urlopen('POST', '/admin') - assert resp.status == 401 - if __name__ == '__main__': unittest.main() From 9a484d32780298d800f7309b395dfbbbbdb5c215 Mon Sep 17 00:00:00 2001 From: Rober Morales-Chaparro Date: Thu, 17 Jan 2019 12:39:33 +0100 Subject: [PATCH 05/27] ignoring chunked test for Google App Engine --- test/with_dummyserver/test_connectionpool.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/with_dummyserver/test_connectionpool.py b/test/with_dummyserver/test_connectionpool.py index 99d984a571..1493728f25 100644 --- a/test/with_dummyserver/test_connectionpool.py +++ b/test/with_dummyserver/test_connectionpool.py @@ -17,6 +17,7 @@ encode_multipart_formdata, HTTPConnectionPool, ) +from urllib3.contrib.appengine import AppEngineManager from urllib3.exceptions import ( ConnectTimeoutError, EmptyPoolError, @@ -775,9 +776,13 @@ def test_mixed_case_hostname(self): self.assertEqual(response.status, 200) def test_broken_pipe_ignore(self): - resp = self.pool.urlopen('POST', '/admin', chunked=False) + resp = self.pool.urlopen('POST', '/admin') assert resp.status == 401 + def test_broken_pipe_ignore_chunked(self): + if isinstance(self.pool, AppEngineManager): + self.skipTest("Google App Engine does not support chunked requests in URLFetch") + resp = self.pool.urlopen('POST', '/admin', chunked=True) assert resp.status == 401 From 3e18f38b6c7238be0faea595d73e8e3137fa5863 Mon Sep 17 00:00:00 2001 From: Seth Michael Larson Date: Thu, 17 Jan 2019 14:31:46 +0000 Subject: [PATCH 06/27] Skip on is_appengine() --- test/with_dummyserver/test_connectionpool.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/with_dummyserver/test_connectionpool.py b/test/with_dummyserver/test_connectionpool.py index 1493728f25..6ba8c450db 100644 --- a/test/with_dummyserver/test_connectionpool.py +++ b/test/with_dummyserver/test_connectionpool.py @@ -17,7 +17,7 @@ encode_multipart_formdata, HTTPConnectionPool, ) -from urllib3.contrib.appengine import AppEngineManager +from urllib3.contrib.appengine import is_appengine from urllib3.exceptions import ( ConnectTimeoutError, EmptyPoolError, @@ -780,7 +780,7 @@ def test_broken_pipe_ignore(self): assert resp.status == 401 def test_broken_pipe_ignore_chunked(self): - if isinstance(self.pool, AppEngineManager): + if is_appengine(): self.skipTest("Google App Engine does not support chunked requests in URLFetch") resp = self.pool.urlopen('POST', '/admin', chunked=True) From 5750179514d34ce2f9f7a3cf3c43dbf6ce55dde5 Mon Sep 17 00:00:00 2001 From: hodbn Date: Thu, 23 Apr 2020 05:25:06 -0700 Subject: [PATCH 07/27] rewrite broken pipe tests --- dummyserver/handlers.py | 5 +--- src/urllib3/connection.py | 3 ++ test/with_dummyserver/test_connectionpool.py | 12 -------- test/with_dummyserver/test_socketlevel.py | 31 ++++++++++++++++++++ 4 files changed, 35 insertions(+), 16 deletions(-) diff --git a/dummyserver/handlers.py b/dummyserver/handlers.py index cc4a574f02..696dbab076 100644 --- a/dummyserver/handlers.py +++ b/dummyserver/handlers.py @@ -10,7 +10,7 @@ import zlib from io import BytesIO -from tornado.web import RequestHandler, HTTPError +from tornado.web import RequestHandler from tornado import httputil from datetime import datetime from datetime import timedelta @@ -324,8 +324,5 @@ def redirect_after(self, request): headers = [("Location", target), ("Retry-After", retry_after)] return Response(status="303 See Other", headers=headers) - def admin(self, _request): - raise HTTPError(401) - def shutdown(self, request): sys.exit() diff --git a/src/urllib3/connection.py b/src/urllib3/connection.py index 5db65a5590..52511d4443 100644 --- a/src/urllib3/connection.py +++ b/src/urllib3/connection.py @@ -29,13 +29,16 @@ class BaseSSLError(BaseException): class ConnectionError(Exception): pass + try: # Python 3: # Not a no-op, we're adding this to the namespace so it can be imported. BrokenPipeError = BrokenPipeError except NameError: # Python 2: + class BrokenPipeError(Exception): pass + from .exceptions import ( NewConnectionError, ConnectTimeoutError, diff --git a/test/with_dummyserver/test_connectionpool.py b/test/with_dummyserver/test_connectionpool.py index 64fe354fa4..763e3aeb26 100644 --- a/test/with_dummyserver/test_connectionpool.py +++ b/test/with_dummyserver/test_connectionpool.py @@ -11,7 +11,6 @@ from .. import TARPIT_HOST, VALID_SOURCE_ADDRESSES, INVALID_SOURCE_ADDRESSES from ..port_helpers import find_unused_port from urllib3 import encode_multipart_formdata, HTTPConnectionPool -from urllib3.contrib.appengine import is_appengine from urllib3.exceptions import ( ConnectTimeoutError, EmptyPoolError, @@ -794,17 +793,6 @@ def test_preserves_path_dot_segments(self): response = pool.request("GET", "/echo_uri/seg0/../seg2") assert response.data == b"/echo_uri/seg0/../seg2" - def test_broken_pipe_ignore(self): - resp = self.pool.urlopen('POST', '/admin') - assert resp.status == 401 - - def test_broken_pipe_ignore_chunked(self): - if is_appengine(): - self.skipTest("Google App Engine does not support chunked requests in URLFetch") - - resp = self.pool.urlopen('POST', '/admin', chunked=True) - assert resp.status == 401 - class TestRetry(HTTPDummyServerTestCase): def test_max_retry(self): diff --git a/test/with_dummyserver/test_socketlevel.py b/test/with_dummyserver/test_socketlevel.py index 2c3ba47d4b..4b577216d5 100644 --- a/test/with_dummyserver/test_socketlevel.py +++ b/test/with_dummyserver/test_socketlevel.py @@ -1,6 +1,7 @@ # TODO: Break this module up into pieces. Maybe group by functionality tested # rather than the socket level-ness of it. from urllib3 import HTTPConnectionPool, HTTPSConnectionPool +from urllib3.connection import HTTPConnection from urllib3.poolmanager import proxy_from_url from urllib3.exceptions import ( MaxRetryError, @@ -1797,3 +1798,33 @@ def socket_handler(listener): ) as pool: pool.urlopen("GET", "/not_found", preload_content=False) assert pool.num_connections == 1 + + +class TestBrokenPipe(SocketDummyServerTestCase): + def test_broken_pipe_ignore(self, monkeypatch): + sock_shut = Event() + orig_connect = HTTPConnection.connect + # a buffer that will cause two sendall calls + buf = "a" * 1024 * 1024 * 4 + + def connect_and_wait(*args, **kw): + ret = orig_connect(*args, **kw) + assert sock_shut.wait(5) + return ret + + def socket_handler(listener): + resp_404 = b"HTTP/1.1 404 NOT FOUND\r\n\r\n" + for i in range(2): + sock = listener.accept()[0] + sock.send(resp_404) + sock.shutdown(socket.SHUT_RDWR) + sock_shut.set() + sock.close() + + monkeypatch.setattr(HTTPConnection, "connect", connect_and_wait) + self._start_server(socket_handler) + with HTTPConnectionPool(self.host, self.port) as pool: + r = pool.request("POST", "/", body=buf) + assert r.status == 404 + r = pool.request("POST", "/admin", chunked=True, body=buf) + assert r.status == 404 From 567a8f4b265b216fc78f92d2af9dc3caf5f4b878 Mon Sep 17 00:00:00 2001 From: hodbn Date: Thu, 23 Apr 2020 05:40:24 -0700 Subject: [PATCH 08/27] account for EPROTOTYPE on osx --- test/with_dummyserver/test_socketlevel.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/with_dummyserver/test_socketlevel.py b/test/with_dummyserver/test_socketlevel.py index 4b577216d5..cff2217c9d 100644 --- a/test/with_dummyserver/test_socketlevel.py +++ b/test/with_dummyserver/test_socketlevel.py @@ -45,6 +45,7 @@ class MimeToolMessage(object): import shutil import ssl import tempfile +import time import mock import pytest @@ -1810,6 +1811,9 @@ def test_broken_pipe_ignore(self, monkeypatch): def connect_and_wait(*args, **kw): ret = orig_connect(*args, **kw) assert sock_shut.wait(5) + # sleep a bit to let osx tear the socket completely + # See https://erickt.github.io/blog/2014/11/19/adventures-in-debugging-a-potential-osx-kernel-bug/ + time.sleep(2) return ret def socket_handler(listener): From ec5ba40545921244236a2f82e0ee012c5441a7e6 Mon Sep 17 00:00:00 2001 From: Quentin Pradet Date: Tue, 28 Apr 2020 10:32:05 +0400 Subject: [PATCH 09/27] Fix test_broken_pipe_ignore ignore on macOS When the server closed the socket but the client still tries to read the response, macOS can fail with EPROTOTYPE instead of simply EPIPE or ESHUTDOWN, so BrokenPipeError isn't enough. I also had to fix the test, otherwise the request would fail because it didn't read the whole response (which is still a healthy thing to do). --- src/urllib3/connectionpool.py | 9 ++++++--- test/with_dummyserver/test_socketlevel.py | 12 ++++++------ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/urllib3/connectionpool.py b/src/urllib3/connectionpool.py index 1b714ec35d..8052d15d4f 100644 --- a/src/urllib3/connectionpool.py +++ b/src/urllib3/connectionpool.py @@ -393,13 +393,16 @@ def _make_request( else: conn.request(method, url, **httplib_request_kw) - # We are shallowing BrokenPipeError (errno.EPIPE) since the server is + # We are swallowing BrokenPipeError (errno.EPIPE) since the server is # legitimately able to close the connection after sending a valid response. # With this behaviour, the received response is still readable. except BrokenPipeError: # Python 3 pass - except IOError as e: # Python 2 - if e.errno != errno.EPIPE: + except IOError as e: + # https://erickt.github.io/blog/2014/11/19/adventures-in-debugging-a-potential-osx-kernel-bug/ + if e.errno == errno.EPROTOTYPE: # macOS + pass + elif e.errno != errno.EPIPE: # Python 2 raise # Reset the timeout for the recv() on the socket diff --git a/test/with_dummyserver/test_socketlevel.py b/test/with_dummyserver/test_socketlevel.py index cff2217c9d..34c96a4398 100644 --- a/test/with_dummyserver/test_socketlevel.py +++ b/test/with_dummyserver/test_socketlevel.py @@ -45,7 +45,6 @@ class MimeToolMessage(object): import shutil import ssl import tempfile -import time import mock import pytest @@ -1811,16 +1810,17 @@ def test_broken_pipe_ignore(self, monkeypatch): def connect_and_wait(*args, **kw): ret = orig_connect(*args, **kw) assert sock_shut.wait(5) - # sleep a bit to let osx tear the socket completely - # See https://erickt.github.io/blog/2014/11/19/adventures-in-debugging-a-potential-osx-kernel-bug/ - time.sleep(2) return ret def socket_handler(listener): - resp_404 = b"HTTP/1.1 404 NOT FOUND\r\n\r\n" for i in range(2): sock = listener.accept()[0] - sock.send(resp_404) + sock.send( + b"HTTP/1.1 404 Not Found\r\n" + b"Connection: close\r\n" + b"Content-Length: 0\r\n" + b"\r\n" + ) sock.shutdown(socket.SHUT_RDWR) sock_shut.set() sock.close() From 2a12bcf34fb8c806b5b373f1822d266b09f8af4a Mon Sep 17 00:00:00 2001 From: hodbn Date: Fri, 24 Apr 2020 05:50:14 -0700 Subject: [PATCH 10/27] Change TARPIT_HOST to detect isolated network (#1862) --- test/__init__.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/__init__.py b/test/__init__.py index 2f6db2e041..01f02738d0 100644 --- a/test/__init__.py +++ b/test/__init__.py @@ -19,8 +19,14 @@ from urllib3.util import ssl_ # We need a host that will not immediately close the connection with a TCP -# Reset. SO suggests this hostname -TARPIT_HOST = "10.255.255.1" +# Reset. +if platform.system() == "Windows": + # Reserved loopback subnet address + TARPIT_HOST = "127.0.0.0" +else: + # Reserved internet scoped address + # https://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml + TARPIT_HOST = "240.0.0.0" # (Arguments for socket, is it IPv6 address?) VALID_SOURCE_ADDRESSES = [(("::1", 0), True), (("127.0.0.1", 0), False)] From 0f3afa9b6304b5a07d34bcc28b7a51ef9cb9912f Mon Sep 17 00:00:00 2001 From: hodbn Date: Sun, 26 Apr 2020 22:32:27 -0700 Subject: [PATCH 11/27] Add 'other errors' counter to Retry (#1824) --- src/urllib3/util/retry.py | 34 +++++++++++++++++++++++++++++++--- test/test_retry.py | 16 ++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/src/urllib3/util/retry.py b/src/urllib3/util/retry.py index ee30c91b14..3d82832418 100644 --- a/src/urllib3/util/retry.py +++ b/src/urllib3/util/retry.py @@ -54,8 +54,7 @@ class Retry(object): Total number of retries to allow. Takes precedence over other counts. Set to ``None`` to remove this constraint and fall back on other - counts. It's a good idea to set this to some sensibly-high value to - account for unexpected edge cases and avoid infinite retry loops. + counts. Set to ``0`` to fail on the first retry. @@ -96,6 +95,18 @@ class Retry(object): Set to ``0`` to fail on the first retry of this type. + :param int other: + How many times to retry on other errors. + + Other errors are errors that are not connect, read, redirect or status errors. + These errors might be raised after the request was sent to the server, so the + request might have side-effects. + + Set to ``0`` to fail on the first retry of this type. + + If ``total`` is not set, it's a good idea to set this to 0 to account + for unexpected edge cases and avoid infinite retry loops. + :param iterable method_whitelist: Set of uppercased HTTP method verbs that we should retry on. @@ -166,6 +177,7 @@ def __init__( read=None, redirect=None, status=None, + other=None, method_whitelist=DEFAULT_METHOD_WHITELIST, status_forcelist=None, backoff_factor=0, @@ -180,6 +192,7 @@ def __init__( self.connect = connect self.read = read self.status = status + self.other = other if redirect is False or total is False: redirect = 0 @@ -204,6 +217,7 @@ def new(self, **kw): read=self.read, redirect=self.redirect, status=self.status, + other=self.other, method_whitelist=self.method_whitelist, status_forcelist=self.status_forcelist, backoff_factor=self.backoff_factor, @@ -348,7 +362,14 @@ def is_retry(self, method, status_code, has_retry_after=False): def is_exhausted(self): """ Are we out of retries? """ - retry_counts = (self.total, self.connect, self.read, self.redirect, self.status) + retry_counts = ( + self.total, + self.connect, + self.read, + self.redirect, + self.status, + self.other, + ) retry_counts = list(filter(None, retry_counts)) if not retry_counts: return False @@ -386,6 +407,7 @@ def increment( read = self.read redirect = self.redirect status_count = self.status + other = self.other cause = "unknown" status = None redirect_location = None @@ -404,6 +426,11 @@ def increment( elif read is not None: read -= 1 + elif error: + # Other retry? + if other is not None: + other -= 1 + elif response and response.get_redirect_location(): # Redirect retry? if redirect is not None: @@ -432,6 +459,7 @@ def increment( read=read, redirect=redirect, status=status_count, + other=other, history=history, ) diff --git a/test/test_retry.py b/test/test_retry.py index c36476f8fa..aa9a7522b2 100644 --- a/test/test_retry.py +++ b/test/test_retry.py @@ -13,6 +13,7 @@ MaxRetryError, ReadTimeoutError, ResponseError, + SSLError, ) @@ -83,6 +84,7 @@ def test_retry_default(self): assert retry.connect is None assert retry.read is None assert retry.redirect is None + assert retry.other is None error = ConnectTimeoutError() retry = Retry(connect=1) @@ -97,6 +99,20 @@ def test_retry_default(self): assert Retry(0).raise_on_redirect assert not Retry(False).raise_on_redirect + def test_retry_other(self): + """ If an unexpected error is raised, should retry other times """ + other_error = SSLError() + retry = Retry(connect=1) + retry = retry.increment(error=other_error) + retry = retry.increment(error=other_error) + assert not retry.is_exhausted() + + retry = Retry(other=1) + retry = retry.increment(error=other_error) + with pytest.raises(MaxRetryError) as e: + retry.increment(error=other_error) + assert e.value.reason == other_error + def test_retry_read_zero(self): """ No second chances on read timeouts, by default """ error = ReadTimeoutError(None, "/", "read timed out") From 3ac99e2c720ec42bf7da80cff0e89bc3ad01ed74 Mon Sep 17 00:00:00 2001 From: hodbn Date: Tue, 28 Apr 2020 05:57:16 -0700 Subject: [PATCH 12/27] Don't insert 'None' into ConnectionPool if it was empty --- src/urllib3/connectionpool.py | 8 +++++--- test/test_connectionpool.py | 11 ++++++++++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/urllib3/connectionpool.py b/src/urllib3/connectionpool.py index 8052d15d4f..35f22b1118 100644 --- a/src/urllib3/connectionpool.py +++ b/src/urllib3/connectionpool.py @@ -712,9 +712,11 @@ def urlopen( # Everything went great! clean_exit = True - except queue.Empty: - # Timed out by queue. - raise EmptyPoolError(self, "No pool connections are available.") + except EmptyPoolError: + # Didn't get a connection from the pool, no need to clean up + clean_exit = True + release_this_conn = False + raise except ( TimeoutError, diff --git a/test/test_connectionpool.py b/test/test_connectionpool.py index 3cd215304f..615fdfc0c1 100644 --- a/test/test_connectionpool.py +++ b/test/test_connectionpool.py @@ -2,6 +2,7 @@ import ssl import pytest +from mock import Mock from urllib3.connectionpool import ( connection_from_url, @@ -279,7 +280,6 @@ def _test(exception, expect, reason=None): # Make sure that all of the exceptions return the connection # to the pool - _test(Empty, EmptyPoolError) _test(BaseSSLError, MaxRetryError, SSLError) _test(CertificateError, MaxRetryError, SSLError) @@ -292,6 +292,15 @@ def _test(exception, expect, reason=None): pool.request("GET", "/", retries=1, pool_timeout=SHORT_TIMEOUT) assert pool.pool.qsize() == POOL_SIZE + def test_empty_does_not_put_conn(self): + """Do not put None back in the pool if the pool was empty""" + + with HTTPConnectionPool(host="localhost", maxsize=1, block=True) as pool: + pool._get_conn = Mock(side_effect=EmptyPoolError(pool, "Pool is empty")) + pool._put_conn = Mock(side_effect=AssertionError("Unexpected _put_conn")) + with pytest.raises(EmptyPoolError): + pool.request("GET", "/") + def test_assert_same_host(self): with connection_from_url("http://google.com:80") as c: with pytest.raises(HostChangedError): From 2f92f1643ce5d8fae52dc6a2d6bf56105b4cf64b Mon Sep 17 00:00:00 2001 From: Quentin Pradet Date: Tue, 12 May 2020 21:21:47 +0400 Subject: [PATCH 13/27] Fix linting for flake8 v3.8 --- src/urllib3/util/ssl_.py | 4 ++-- test/contrib/test_pyopenssl.py | 4 ++-- test/contrib/test_securetransport.py | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/urllib3/util/ssl_.py b/src/urllib3/util/ssl_.py index f7e2b70558..3f45c5258b 100644 --- a/src/urllib3/util/ssl_.py +++ b/src/urllib3/util/ssl_.py @@ -29,8 +29,8 @@ def _const_compare_digest_backport(a, b): Returns True if the digests match, and False otherwise. """ result = abs(len(a) - len(b)) - for l, r in zip(bytearray(a), bytearray(b)): - result |= l ^ r + for left, right in zip(bytearray(a), bytearray(b)): + result |= left ^ right return result == 0 diff --git a/test/contrib/test_pyopenssl.py b/test/contrib/test_pyopenssl.py index 6132479abc..25b8deb16f 100644 --- a/test/contrib/test_pyopenssl.py +++ b/test/contrib/test_pyopenssl.py @@ -30,7 +30,7 @@ def teardown_module(): pass -from ..with_dummyserver.test_https import ( # noqa: F401 +from ..with_dummyserver.test_https import ( # noqa: E402, F401 TestHTTPS, TestHTTPS_TLSv1, TestHTTPS_TLSv1_1, @@ -41,7 +41,7 @@ def teardown_module(): TestHTTPS_NoSAN, TestHTTPS_IPV6SAN, ) -from ..with_dummyserver.test_socketlevel import ( # noqa: F401 +from ..with_dummyserver.test_socketlevel import ( # noqa: E402, F401 TestSNI, TestSocketClosing, TestClientCerts, diff --git a/test/contrib/test_securetransport.py b/test/contrib/test_securetransport.py index 1a0bb5192d..5fb8fbcfde 100644 --- a/test/contrib/test_securetransport.py +++ b/test/contrib/test_securetransport.py @@ -31,13 +31,13 @@ def teardown_module(): # SecureTransport does not support TLSv1.3 # https://github.com/urllib3/urllib3/issues/1674 -from ..with_dummyserver.test_https import ( # noqa: F401 +from ..with_dummyserver.test_https import ( # noqa: E402, F401 TestHTTPS, TestHTTPS_TLSv1, TestHTTPS_TLSv1_1, TestHTTPS_TLSv1_2, ) -from ..with_dummyserver.test_socketlevel import ( # noqa: F401 +from ..with_dummyserver.test_socketlevel import ( # noqa: E402, F401 TestSNI, TestSocketClosing, TestClientCerts, From fe8f85f3dacb4c4faab948bf67f32a33991ce464 Mon Sep 17 00:00:00 2001 From: Quentin Pradet Date: Fri, 29 May 2020 09:33:43 +0400 Subject: [PATCH 14/27] Use nox version that works with Python 3.5.2 This is the default `python3` shipped by Travis. --- _travis/install.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/_travis/install.sh b/_travis/install.sh index 756ced8962..adda3d13cb 100755 --- a/_travis/install.sh +++ b/_travis/install.sh @@ -10,7 +10,8 @@ set -exo pipefail if ! python3 -m pip --version; then curl https://bootstrap.pypa.io/get-pip.py -o get-pip.py sudo python3 get-pip.py - sudo python3 -m pip install nox + # https://github.com/theacodes/nox/issues/328 + sudo python3 -m pip install nox==2019.11.9 else # We're not in "dual Python" mode, so we can just install Nox normally. python3 -m pip install nox From d3901a89db3ec8bcf5d5c42388dec2dd5df22ad3 Mon Sep 17 00:00:00 2001 From: hodbn Date: Tue, 9 Jun 2020 09:42:11 -0700 Subject: [PATCH 15/27] Add InvalidChunkLength error --- src/urllib3/exceptions.py | 22 ++++++++++++++++++++++ src/urllib3/response.py | 5 +++-- test/test_response.py | 39 ++++++++++++++++++++++++++++++++++----- 3 files changed, 59 insertions(+), 7 deletions(-) diff --git a/src/urllib3/exceptions.py b/src/urllib3/exceptions.py index 880f1ba2f7..0d15da0c0d 100644 --- a/src/urllib3/exceptions.py +++ b/src/urllib3/exceptions.py @@ -231,6 +231,28 @@ def __repr__(self): ) +class InvalidChunkLength(HTTPError, httplib_IncompleteRead): + """Invalid chunk length in a chunked response.""" + + def __init__(self, response, length): + super(InvalidChunkLength, self).__init__( + response.tell(), response.length_remaining + ) + self.response = response + self.length = length + + def __repr__(self): + if self.expected is not None: + e = ", %i more expected" % self.expected + else: + e = "" + return "InvalidChunkLength(got length %r, %i bytes read%s)" % ( + self.length, + self.partial, + e, + ) + + class InvalidHeader(HTTPError): "The header provided was somehow invalid." pass diff --git a/src/urllib3/response.py b/src/urllib3/response.py index 7522f9a602..baf5cb7166 100644 --- a/src/urllib3/response.py +++ b/src/urllib3/response.py @@ -19,11 +19,12 @@ ReadTimeoutError, ResponseNotChunked, IncompleteRead, + InvalidChunkLength, InvalidHeader, HTTPError, ) from .packages.six import string_types as basestring, PY3 -from .packages.six.moves import http_client as httplib +from .packages.six.moves import http_client as httplib # noqa: F401 from .connection import HTTPException, BaseSSLError from .util.response import is_fp_closed, is_response_to_head @@ -697,7 +698,7 @@ def _update_chunk_length(self): except ValueError: # Invalid chunked protocol response, abort. self.close() - raise httplib.IncompleteRead(line) + raise InvalidChunkLength(self, line) def _handle_chunk(self, amt): returned_chunk = None diff --git a/test/test_response.py b/test/test_response.py index ef84e44bd9..44aed545ac 100644 --- a/test/test_response.py +++ b/test/test_response.py @@ -16,6 +16,8 @@ ResponseNotChunked, ProtocolError, InvalidHeader, + httplib_IncompleteRead, + InvalidChunkLength, ) from urllib3.packages.six.moves import http_client as httplib from urllib3.util.retry import Retry, RequestHistory @@ -758,9 +760,9 @@ def test_read_not_chunked_response_as_chunks(self): with pytest.raises(ResponseNotChunked): next(r) - def test_invalid_chunks(self): + def test_incomplete_chunk(self): stream = [b"foooo", b"bbbbaaaaar"] - fp = MockChunkedInvalidEncoding(stream) + fp = MockChunkedIncompleteRead(stream) r = httplib.HTTPResponse(MockSock) r.fp = fp r.chunked = True @@ -768,9 +770,29 @@ def test_invalid_chunks(self): resp = HTTPResponse( r, preload_content=False, headers={"transfer-encoding": "chunked"} ) - with pytest.raises(ProtocolError): + with pytest.raises(ProtocolError) as ctx: next(resp.read_chunked()) + orig_ex = ctx.value.args[1] + assert isinstance(orig_ex, httplib_IncompleteRead) + + def test_invalid_chunk_length(self): + stream = [b"foooo", b"bbbbaaaaar"] + fp = MockChunkedInvalidChunkLength(stream) + r = httplib.HTTPResponse(MockSock) + r.fp = fp + r.chunked = True + r.chunk_left = None + resp = HTTPResponse( + r, preload_content=False, headers={"transfer-encoding": "chunked"} + ) + with pytest.raises(ProtocolError) as ctx: + next(resp.read_chunked()) + + orig_ex = ctx.value.args[1] + assert isinstance(orig_ex, InvalidChunkLength) + assert orig_ex.length == six.b(fp.BAD_LENGTH_LINE) + def test_chunked_response_without_crlf_on_end(self): stream = [b"foo", b"bar", b"baz"] fp = MockChunkedEncodingWithoutCRLFOnEnd(stream) @@ -971,9 +993,16 @@ def close(self): self.closed = True -class MockChunkedInvalidEncoding(MockChunkedEncodingResponse): +class MockChunkedIncompleteRead(MockChunkedEncodingResponse): + def _encode_chunk(self, chunk): + return "9999\r\n%s\r\n" % chunk.decode() + + +class MockChunkedInvalidChunkLength(MockChunkedEncodingResponse): + BAD_LENGTH_LINE = "ZZZ\r\n" + def _encode_chunk(self, chunk): - return "ZZZ\r\n%s\r\n" % chunk.decode() + return "%s%s\r\n" % (self.BAD_LENGTH_LINE, chunk.decode()) class MockChunkedEncodingWithoutCRLFOnEnd(MockChunkedEncodingResponse): From 082b78d171f039b9dd33472cce580f6c41c80061 Mon Sep 17 00:00:00 2001 From: hodbn Date: Tue, 9 Jun 2020 09:43:00 -0700 Subject: [PATCH 16/27] Test buggy incomplete read --- test/test_response.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/test_response.py b/test/test_response.py index 44aed545ac..f7802b163c 100644 --- a/test/test_response.py +++ b/test/test_response.py @@ -17,6 +17,7 @@ ProtocolError, InvalidHeader, httplib_IncompleteRead, + IncompleteRead, InvalidChunkLength, ) from urllib3.packages.six.moves import http_client as httplib @@ -760,6 +761,25 @@ def test_read_not_chunked_response_as_chunks(self): with pytest.raises(ResponseNotChunked): next(r) + def test_buggy_incomplete_read(self): + # Simulate buggy versions of Python + # See http://bugs.python.org/issue16298 + content_length = 1337 + fp = BytesIO(b"") + resp = HTTPResponse( + fp, + headers={"content-length": str(content_length)}, + preload_content=False, + enforce_content_length=True, + ) + with pytest.raises(ProtocolError) as ctx: + resp.read(3) + + orig_ex = ctx.value.args[1] + assert isinstance(orig_ex, IncompleteRead) + assert orig_ex.partial == 0 + assert orig_ex.expected == content_length + def test_incomplete_chunk(self): stream = [b"foooo", b"bbbbaaaaar"] fp = MockChunkedIncompleteRead(stream) From 2ee6eac5b07c0cdc34b155fe4a57fa819bc1cb91 Mon Sep 17 00:00:00 2001 From: hodbn Date: Tue, 9 Jun 2020 12:07:25 -0700 Subject: [PATCH 17/27] Fix coverage, InvalidChunkLength is chunked and has no expected length --- src/urllib3/exceptions.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/urllib3/exceptions.py b/src/urllib3/exceptions.py index 0d15da0c0d..957fb966cf 100644 --- a/src/urllib3/exceptions.py +++ b/src/urllib3/exceptions.py @@ -242,14 +242,9 @@ def __init__(self, response, length): self.length = length def __repr__(self): - if self.expected is not None: - e = ", %i more expected" % self.expected - else: - e = "" - return "InvalidChunkLength(got length %r, %i bytes read%s)" % ( + return "InvalidChunkLength(got length %r, %i bytes read)" % ( self.length, self.partial, - e, ) From b0a8d7979af97564473294d88bdccf3f9a613ef6 Mon Sep 17 00:00:00 2001 From: hodbn Date: Tue, 9 Jun 2020 12:20:13 -0700 Subject: [PATCH 18/27] Update docs for an old Python version test --- test/test_response.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_response.py b/test/test_response.py index f7802b163c..a476f23b5a 100644 --- a/test/test_response.py +++ b/test/test_response.py @@ -762,7 +762,7 @@ def test_read_not_chunked_response_as_chunks(self): next(r) def test_buggy_incomplete_read(self): - # Simulate buggy versions of Python + # Simulate buggy versions of Python (<2.7.4) # See http://bugs.python.org/issue16298 content_length = 1337 fp = BytesIO(b"") From e83cb299a3c0093412b83d561c880bad0c2ccb8b Mon Sep 17 00:00:00 2001 From: hodbn Date: Wed, 10 Jun 2020 09:20:34 -0700 Subject: [PATCH 19/27] Remove unnecessary httplib export --- src/urllib3/response.py | 1 - test/test_connectionpool.py | 3 ++- test/with_dummyserver/test_socketlevel.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/urllib3/response.py b/src/urllib3/response.py index baf5cb7166..fcf9d7f94e 100644 --- a/src/urllib3/response.py +++ b/src/urllib3/response.py @@ -24,7 +24,6 @@ HTTPError, ) from .packages.six import string_types as basestring, PY3 -from .packages.six.moves import http_client as httplib # noqa: F401 from .connection import HTTPException, BaseSSLError from .util.response import is_fp_closed, is_response_to_head diff --git a/test/test_connectionpool.py b/test/test_connectionpool.py index 615fdfc0c1..193ba91df8 100644 --- a/test/test_connectionpool.py +++ b/test/test_connectionpool.py @@ -10,8 +10,9 @@ HTTPConnectionPool, HTTPSConnectionPool, ) -from urllib3.response import httplib, HTTPResponse +from urllib3.response import HTTPResponse from urllib3.util.timeout import Timeout +from urllib3.packages.six.moves import http_client as httplib from urllib3.packages.six.moves.http_client import HTTPException from urllib3.packages.six.moves.queue import Empty from urllib3.packages.ssl_match_hostname import CertificateError diff --git a/test/with_dummyserver/test_socketlevel.py b/test/with_dummyserver/test_socketlevel.py index 34c96a4398..8b1f20069d 100644 --- a/test/with_dummyserver/test_socketlevel.py +++ b/test/with_dummyserver/test_socketlevel.py @@ -10,7 +10,7 @@ SSLError, ProtocolError, ) -from urllib3.response import httplib +from urllib3.packages.six.moves import http_client as httplib from urllib3.util import ssl_wrap_socket from urllib3.util.ssl_ import HAS_SNI from urllib3.util import ssl_ From a024db54fac817fa9b9bb40f70bf2be1404998ea Mon Sep 17 00:00:00 2001 From: Bastiaan Bakker Date: Thu, 11 Jun 2020 15:13:09 +0200 Subject: [PATCH 20/27] Feature/support env var sslkeylogfile (#1867) --- CONTRIBUTORS.txt | 3 +++ docs/advanced-usage.rst | 15 ++++++++++++++- src/urllib3/util/ssl_.py | 7 +++++++ test/with_dummyserver/test_https.py | 25 +++++++++++++++++++++++++ 4 files changed, 49 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index ef41e32b73..68caa5defe 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -300,5 +300,8 @@ In chronological order: * Chris Olufson * Fix for connection not being released on HTTP redirect and response not preloaded +* [Bastiaan Bakker] + * Support for logging session keys via environment variable ``SSLKEYLOGFILE`` (Python 3.8+) + * [Your name or handle] <[email or website]> * [Brief summary of your changes] diff --git a/docs/advanced-usage.rst b/docs/advanced-usage.rst index 5bcc93adc5..e36287a064 100644 --- a/docs/advanced-usage.rst +++ b/docs/advanced-usage.rst @@ -128,7 +128,7 @@ you're contacting. When contacting a HTTP website through a HTTP or HTTPS proxy, the request will be forwarded with the `absolute URI -`_. +`_. When contacting a HTTPS website through a HTTP proxy, a TCP tunnel will be established with a HTTP CONNECT. Afterward a TLS connection will be established @@ -303,3 +303,16 @@ Here's an example using brotli encoding via the ``Accept-Encoding`` header:: >>> from urllib3 import PoolManager >>> http = PoolManager() >>> http.request('GET', 'https://www.google.com/', headers={'Accept-Encoding': 'br'}) + +Decrypting captured TLS sessions with Wireshark +----------------------------------------------- +Python 3.8 and higher support logging of TLS pre-master secrets. +With these secrets tools like `Wireshark `_ can decrypt captured +network traffic. + +To enable this simply define environment variable `SSLKEYLOGFILE`: + + export SSLKEYLOGFILE=/path/to/keylogfile.txt + +Then configure the key logfile in `Wireshark `_, see +`Wireshark TLS Decryption `_ for instructions. diff --git a/src/urllib3/util/ssl_.py b/src/urllib3/util/ssl_.py index 3f45c5258b..3d89a56c08 100644 --- a/src/urllib3/util/ssl_.py +++ b/src/urllib3/util/ssl_.py @@ -2,6 +2,7 @@ import errno import warnings import hmac +import os import sys from binascii import hexlify, unhexlify @@ -293,6 +294,12 @@ def create_urllib3_context( # We do our own verification, including fingerprints and alternative # hostnames. So disable it here context.check_hostname = False + + # Enable logging of TLS session keys via defacto standard environment variable + # 'SSLKEYLOGFILE', if the feature is available (Python 3.8+). + if hasattr(context, "keylog_filename"): + context.keylog_filename = os.environ.get("SSLKEYLOGFILE") + return context diff --git a/test/with_dummyserver/test_https.py b/test/with_dummyserver/test_https.py index 897c8ab17e..e0b2504b84 100644 --- a/test/with_dummyserver/test_https.py +++ b/test/with_dummyserver/test_https.py @@ -698,6 +698,31 @@ def test_tls_protocol_name_of_socket(self): finally: conn.close() + @pytest.mark.skipif( + not hasattr(ssl.SSLContext, "keylog_filename"), + reason="requires OpenSSL 1.1.1+", + ) + @pytest.mark.skipif(sys.version_info < (3, 8), reason="requires python 3.8+") + @pytest.mark.skipif( + sys.platform == "win32", + reason="does not work reliably in Appveyor test enviroment for not yet known reasons", + ) + def test_sslkeylogfile(self, tmpdir, monkeypatch): + keylog_file = tmpdir.join("keylogfile.txt") + monkeypatch.setenv("SSLKEYLOGFILE", str(keylog_file)) + with HTTPSConnectionPool( + self.host, self.port, ca_certs=DEFAULT_CA + ) as https_pool: + r = https_pool.request("GET", "/") + assert r.status == 200, r.data + assert keylog_file.check(file=1), "keylogfile '%s' should exist" % str( + keylog_file + ) + assert keylog_file.read().startswith("# TLS secrets log file"), ( + "keylogfile '%s' should start with '# TLS secrets log file'" + % str(keylog_file) + ) + @requiresTLSv1() class TestHTTPS_TLSv1(TestHTTPS): From c9c57b407d52b6e3f955c65a084d2e66197650cd Mon Sep 17 00:00:00 2001 From: PleasantMachine9 <65126927+PleasantMachine9@users.noreply.github.com> Date: Sun, 14 Jun 2020 19:03:34 +0000 Subject: [PATCH 21/27] Add documentation for using Nox more efficiently (#1887) --- docs/contributing.rst | 36 +++++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/docs/contributing.rst b/docs/contributing.rst index c66b28ada9..468aead747 100644 --- a/docs/contributing.rst +++ b/docs/contributing.rst @@ -13,8 +13,8 @@ If you wish to add a new feature or fix a bug: to start making your changes. #. Write a test which shows that the bug was fixed or that the feature works as expected. -#. Format your changes with black using command `$ nox -s blacken` and lint your - changes using command `nox -s lint`. +#. Format your changes with black using command `$ nox -rs blacken` and lint your + changes using command `nox -rs lint`. #. Send a pull request and bug the maintainer until it gets merged and published. :) Make sure to add yourself to ``CONTRIBUTORS.txt``. @@ -35,8 +35,8 @@ We use some external dependencies, multiple interpreters and code coverage analysis while running test suite. Our ``noxfile.py`` handles much of this for you:: - $ nox --sessions test-2.7 test-3.7 - [ Nox will create virtualenv, install the specified dependencies, and run the commands in order.] + $ nox --reuse-existing-virtualenvs --sessions test-2.7 test-3.7 + [ Nox will create virtualenv if needed, install the specified dependencies, and run the commands in order.] nox > Running session test-2.7 ....... ....... @@ -51,15 +51,15 @@ you:: There is also a nox command for running all of our tests and multiple python versions. - $ nox --sessions test + $ nox --reuse-existing-virtualenvs --sessions test Note that code coverage less than 100% is regarded as a failing run. Some platform-specific tests are skipped unless run in that platform. To make sure the code works in all of urllib3's supported platforms, you can run our ``tox`` suite:: - $ nox --sessions test - [ Nox will create virtualenv, install the specified dependencies, and run the commands in order.] + $ nox --reuse-existing-virtualenvs --sessions test + [ Nox will create virtualenv if needed, install the specified dependencies, and run the commands in order.] ....... ....... nox > Session test-2.7 was successful. @@ -73,6 +73,28 @@ suite:: Our test suite `runs continuously on Travis CI `_ with every pull request. +To run specific tests or quickly re-run without nox recreating the env, do the following:: + + $ nox --reuse-existing-virtualenvs --sessions test-3.8 -- pyTestArgument1 pyTestArgument2 pyTestArgumentN + [ Nox will create virtualenv, install the specified dependencies, and run the commands in order.] + nox > Running session test-3.8 + nox > Re-using existing virtual environment at .nox/test-3-8. + ....... + ....... + nox > Session test-3.8 was successful. + +After the ``--`` indicator, any arguments will be passed to pytest. +To specify an exact test case the following syntax also works: +``test/dir/module_name.py::TestClassName::test_method_name`` +(eg.: ``test/with_dummyserver/test_https.py::TestHTTPS::test_simple``). +The following argument is another valid example to pass to pytest: ``-k test_methode_name``. +These are useful when developing new test cases and there is no point +re-running ther entire test suite every iteration. It is also possible to +further parameterize pytest for local testing. + +For all valid arguments, check `the pytest documentation +`_. + Releases -------- From 75aca6a8caa1c3a048e9c754abc3d75dfd84802e Mon Sep 17 00:00:00 2001 From: hodbn Date: Wed, 24 Jun 2020 12:38:17 -0700 Subject: [PATCH 22/27] Fix testing of SSLKEYLOGFILE on AppVeyor --- test/with_dummyserver/test_https.py | 10 ++-------- test/with_dummyserver/test_socketlevel.py | 5 +++-- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/test/with_dummyserver/test_https.py b/test/with_dummyserver/test_https.py index e0b2504b84..414aea49f0 100644 --- a/test/with_dummyserver/test_https.py +++ b/test/with_dummyserver/test_https.py @@ -698,16 +698,10 @@ def test_tls_protocol_name_of_socket(self): finally: conn.close() - @pytest.mark.skipif( - not hasattr(ssl.SSLContext, "keylog_filename"), - reason="requires OpenSSL 1.1.1+", - ) @pytest.mark.skipif(sys.version_info < (3, 8), reason="requires python 3.8+") - @pytest.mark.skipif( - sys.platform == "win32", - reason="does not work reliably in Appveyor test enviroment for not yet known reasons", - ) def test_sslkeylogfile(self, tmpdir, monkeypatch): + if not hasattr(util.SSLContext, "keylog_filename"): + pytest.skip("requires OpenSSL 1.1.1+") keylog_file = tmpdir.join("keylogfile.txt") monkeypatch.setenv("SSLKEYLOGFILE", str(keylog_file)) with HTTPSConnectionPool( diff --git a/test/with_dummyserver/test_socketlevel.py b/test/with_dummyserver/test_socketlevel.py index 8b1f20069d..b5a6a8b41b 100644 --- a/test/with_dummyserver/test_socketlevel.py +++ b/test/with_dummyserver/test_socketlevel.py @@ -11,8 +11,8 @@ ProtocolError, ) from urllib3.packages.six.moves import http_client as httplib +from urllib3 import util from urllib3.util import ssl_wrap_socket -from urllib3.util.ssl_ import HAS_SNI from urllib3.util import ssl_ from urllib3.util.timeout import Timeout from urllib3.util.retry import Retry @@ -88,8 +88,9 @@ def multicookie_response_handler(listener): class TestSNI(SocketDummyServerTestCase): - @pytest.mark.skipif(not HAS_SNI, reason="SNI-support not available") def test_hostname_in_first_request_packet(self): + if not util.HAS_SNI: + pytest.skip("SNI-support not available") done_receiving = Event() self.buf = b"" From aa06d7afbbe444d367c4580643643f090db01f6f Mon Sep 17 00:00:00 2001 From: hodbn Date: Mon, 29 Jun 2020 22:14:13 -0700 Subject: [PATCH 23/27] Raise a meaningful error for an unknown scheme (#1872) --- src/urllib3/exceptions.py | 12 ++++++++++- src/urllib3/poolmanager.py | 5 ++++- test/with_dummyserver/test_poolmanager.py | 25 ++++++++++++++++++++++- 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/src/urllib3/exceptions.py b/src/urllib3/exceptions.py index 957fb966cf..e59ebbf261 100644 --- a/src/urllib3/exceptions.py +++ b/src/urllib3/exceptions.py @@ -153,6 +153,16 @@ def __init__(self, location): self.location = location +class URLSchemeUnknown(LocationValueError): + "Raised when a URL input has an unsupported scheme." + + def __init__(self, scheme): + message = "Not supported URL scheme %s" % scheme + super(URLSchemeUnknown, self).__init__(message) + + self.scheme = scheme + + class ResponseError(HTTPError): "Used as a container for an error reason supplied in a MaxRetryError." GENERIC_ERROR = "too many error responses" @@ -253,7 +263,7 @@ class InvalidHeader(HTTPError): pass -class ProxySchemeUnknown(AssertionError, ValueError): +class ProxySchemeUnknown(AssertionError, URLSchemeUnknown): "ProxyManager does not support the supplied scheme" # TODO(t-8ch): Stop inheriting from AssertionError in v2.0. diff --git a/src/urllib3/poolmanager.py b/src/urllib3/poolmanager.py index a0e5b974b9..ab183de85d 100644 --- a/src/urllib3/poolmanager.py +++ b/src/urllib3/poolmanager.py @@ -14,6 +14,7 @@ MaxRetryError, ProxySchemeUnknown, ProxySchemeUnsupported, + URLSchemeUnknown, ) from .packages import six from .packages.six.moves.urllib.parse import urljoin @@ -255,7 +256,9 @@ def connection_from_context(self, request_context): value must be a key in ``key_fn_by_scheme`` instance variable. """ scheme = request_context["scheme"].lower() - pool_key_constructor = self.key_fn_by_scheme[scheme] + pool_key_constructor = self.key_fn_by_scheme.get(scheme) + if not pool_key_constructor: + raise URLSchemeUnknown(scheme) pool_key = pool_key_constructor(request_context) return self.connection_from_pool_key(pool_key, request_context=request_context) diff --git a/test/with_dummyserver/test_poolmanager.py b/test/with_dummyserver/test_poolmanager.py index 1160a2b6e5..9d2303e889 100644 --- a/test/with_dummyserver/test_poolmanager.py +++ b/test/with_dummyserver/test_poolmanager.py @@ -6,7 +6,7 @@ from dummyserver.testcase import HTTPDummyServerTestCase, IPv6HTTPDummyServerTestCase from urllib3.poolmanager import PoolManager from urllib3.connectionpool import port_by_scheme -from urllib3.exceptions import MaxRetryError +from urllib3.exceptions import MaxRetryError, URLSchemeUnknown from urllib3.util.retry import Retry from test import LONG_TIMEOUT @@ -223,6 +223,29 @@ def test_redirect_without_preload_releases_connection(self): assert r._pool.num_connections == 1 assert len(http.pools) == 1 + def test_unknown_scheme(self): + with PoolManager() as http: + unknown_scheme = "unknown" + unknown_scheme_url = "%s://host" % unknown_scheme + with pytest.raises(URLSchemeUnknown) as e: + r = http.request("GET", unknown_scheme_url) + assert e.value.scheme == unknown_scheme + r = http.request( + "GET", + "%s/redirect" % self.base_url, + fields={"target": unknown_scheme_url}, + redirect=False, + ) + assert r.status == 303 + assert r.headers.get("Location") == unknown_scheme_url + with pytest.raises(URLSchemeUnknown) as e: + r = http.request( + "GET", + "%s/redirect" % self.base_url, + fields={"target": unknown_scheme_url}, + ) + assert e.value.scheme == unknown_scheme + def test_raise_on_redirect(self): with PoolManager() as http: r = http.request( From 227571794402e44220d4aee07aab16eade2633bb Mon Sep 17 00:00:00 2001 From: DonaCthulhuote <10714867+DonaCthulhuote@users.noreply.github.com> Date: Tue, 30 Jun 2020 01:18:15 -0400 Subject: [PATCH 24/27] Add default user agent header (#1750) Co-authored-by: hodbn --- src/urllib3/connection.py | 19 +++- src/urllib3/util/__init__.py | 3 +- src/urllib3/util/request.py | 4 + .../with_dummyserver/test_chunked_transfer.py | 40 ++++++-- test/with_dummyserver/test_connectionpool.py | 60 ++++++++++++ test/with_dummyserver/test_socketlevel.py | 97 ++++++++----------- 6 files changed, 155 insertions(+), 68 deletions(-) diff --git a/src/urllib3/connection.py b/src/urllib3/connection.py index 52511d4443..215c058fcb 100644 --- a/src/urllib3/connection.py +++ b/src/urllib3/connection.py @@ -56,9 +56,10 @@ class BrokenPipeError(Exception): ) -from .util import connection +from .util import connection, SUPPRESS_USER_AGENT from ._collections import HTTPHeaderDict +from ._version import __version__ log = logging.getLogger(__name__) @@ -209,6 +210,14 @@ def putrequest(self, method, url, *args, **kwargs): return _HTTPConnection.putrequest(self, method, url, *args, **kwargs) + def request(self, method, url, body=None, headers=None): + headers = HTTPHeaderDict(headers if headers is not None else {}) + if "user-agent" not in headers: + headers["User-Agent"] = _get_default_user_agent() + elif headers["user-agent"] == SUPPRESS_USER_AGENT: + del headers["user-agent"] + super(HTTPConnection, self).request(method, url, body=body, headers=headers) + def request_chunked(self, method, url, body=None, headers=None): """ Alternative to the common request method, which sends the @@ -220,6 +229,10 @@ def request_chunked(self, method, url, body=None, headers=None): self.putrequest( method, url, skip_accept_encoding=skip_accept_encoding, skip_host=skip_host ) + if "user-agent" not in headers: + headers["User-Agent"] = _get_default_user_agent() + elif headers["user-agent"] == SUPPRESS_USER_AGENT: + del headers["user-agent"] for header, value in headers.items(): self.putheader(header, value) if "transfer-encoding" not in headers: @@ -427,6 +440,10 @@ def _match_hostname(cert, asserted_hostname): raise +def _get_default_user_agent(): + return "python-urllib3/%s" % __version__ + + if not ssl: HTTPSConnection = DummyConnection # noqa: F811 diff --git a/src/urllib3/util/__init__.py b/src/urllib3/util/__init__.py index a96c73a9d8..3fa98c5355 100644 --- a/src/urllib3/util/__init__.py +++ b/src/urllib3/util/__init__.py @@ -2,7 +2,7 @@ # For backwards compatibility, provide imports that used to be here. from .connection import is_connection_dropped -from .request import make_headers +from .request import make_headers, SUPPRESS_USER_AGENT from .response import is_fp_closed from .ssl_ import ( SSLContext, @@ -43,4 +43,5 @@ "ssl_wrap_socket", "wait_for_read", "wait_for_write", + "SUPPRESS_USER_AGENT", ) diff --git a/src/urllib3/util/request.py b/src/urllib3/util/request.py index 3b7bb54daf..27b6c6cf92 100644 --- a/src/urllib3/util/request.py +++ b/src/urllib3/util/request.py @@ -4,6 +4,10 @@ from ..packages.six import b, integer_types from ..exceptions import UnrewindableBodyError +# Use an invalid User-Agent to represent suppressing of default user agent. +# See https://tools.ietf.org/html/rfc7231#section-5.5.3 and +# https://tools.ietf.org/html/rfc7230#section-3.2.6 +SUPPRESS_USER_AGENT = "@@@INVALID_USER_AGENT@@@" ACCEPT_ENCODING = "gzip,deflate" try: import brotli as _unused_module_brotli # noqa: F401 diff --git a/test/with_dummyserver/test_chunked_transfer.py b/test/with_dummyserver/test_chunked_transfer.py index 76be16e16d..9790bc67b2 100644 --- a/test/with_dummyserver/test_chunked_transfer.py +++ b/test/with_dummyserver/test_chunked_transfer.py @@ -4,6 +4,7 @@ from urllib3 import HTTPConnectionPool from urllib3.util.retry import Retry +from urllib3.util import SUPPRESS_USER_AGENT from dummyserver.testcase import SocketDummyServerTestCase, consume_socket # Retry failed tests @@ -78,16 +79,18 @@ def test_empty_string_body(self): def test_empty_iterable_body(self): self._test_body([]) + def _get_header_lines(self, prefix): + header_block = self.buffer.split(b"\r\n\r\n", 1)[0].lower() + header_lines = header_block.split(b"\r\n")[1:] + return [x for x in header_lines if x.startswith(prefix)] + def test_removes_duplicate_host_header(self): self.start_chunked_handler() chunks = ["foo", "bar", "", "bazzzzzzzzzzzzzzzzzzzzzz"] with HTTPConnectionPool(self.host, self.port, retries=False) as pool: pool.urlopen("GET", "/", chunks, headers={"Host": "test.org"}, chunked=True) - header_block = self.buffer.split(b"\r\n\r\n", 1)[0].lower() - header_lines = header_block.split(b"\r\n")[1:] - - host_headers = [x for x in header_lines if x.startswith(b"host")] + host_headers = self._get_header_lines(b"host") assert len(host_headers) == 1 def test_provides_default_host_header(self): @@ -96,12 +99,33 @@ def test_provides_default_host_header(self): with HTTPConnectionPool(self.host, self.port, retries=False) as pool: pool.urlopen("GET", "/", chunks, chunked=True) - header_block = self.buffer.split(b"\r\n\r\n", 1)[0].lower() - header_lines = header_block.split(b"\r\n")[1:] - - host_headers = [x for x in header_lines if x.startswith(b"host")] + host_headers = self._get_header_lines(b"host") assert len(host_headers) == 1 + def test_provides_default_user_agent_header(self): + self.start_chunked_handler() + chunks = ["foo", "bar", "", "bazzzzzzzzzzzzzzzzzzzzzz"] + with HTTPConnectionPool(self.host, self.port, retries=False) as pool: + pool.urlopen("GET", "/", chunks, chunked=True) + + ua_headers = self._get_header_lines(b"user-agent") + assert len(ua_headers) == 1 + + def test_remove_user_agent_header(self): + self.start_chunked_handler() + chunks = ["foo", "bar", "", "bazzzzzzzzzzzzzzzzzzzzzz"] + with HTTPConnectionPool(self.host, self.port, retries=False) as pool: + pool.urlopen( + "GET", + "/", + chunks, + headers={"User-Agent": SUPPRESS_USER_AGENT}, + chunked=True, + ) + + ua_headers = self._get_header_lines(b"user-agent") + assert len(ua_headers) == 0 + def test_preserve_chunked_on_retry_after(self): self.chunked_requests = 0 self.socks = [] diff --git a/test/with_dummyserver/test_connectionpool.py b/test/with_dummyserver/test_connectionpool.py index 763e3aeb26..289ac66452 100644 --- a/test/with_dummyserver/test_connectionpool.py +++ b/test/with_dummyserver/test_connectionpool.py @@ -1,4 +1,5 @@ import io +import json import logging import socket import sys @@ -11,6 +12,7 @@ from .. import TARPIT_HOST, VALID_SOURCE_ADDRESSES, INVALID_SOURCE_ADDRESSES from ..port_helpers import find_unused_port from urllib3 import encode_multipart_formdata, HTTPConnectionPool +from urllib3.connection import _get_default_user_agent from urllib3.exceptions import ( ConnectTimeoutError, EmptyPoolError, @@ -22,6 +24,7 @@ ) from urllib3.packages.six import b, u from urllib3.packages.six.moves.urllib.parse import urlencode +from urllib3.util import SUPPRESS_USER_AGENT from urllib3.util.retry import Retry, RequestHistory from urllib3.util.timeout import Timeout @@ -793,6 +796,63 @@ def test_preserves_path_dot_segments(self): response = pool.request("GET", "/echo_uri/seg0/../seg2") assert response.data == b"/echo_uri/seg0/../seg2" + def test_default_user_agent_header(self): + """ ConnectionPool has a default user agent """ + default_ua = _get_default_user_agent() + custom_ua = "I'm not a web scraper, what are you talking about?" + custom_ua2 = "Yet Another User Agent" + with HTTPConnectionPool(self.host, self.port) as pool: + # Use default user agent if no user agent was specified. + r = pool.request("GET", "/headers") + request_headers = json.loads(r.data.decode("utf8")) + assert request_headers.get("User-Agent") == _get_default_user_agent() + + # Prefer the request user agent over the default. + headers = {"UsEr-AGENt": custom_ua} + r = pool.request("GET", "/headers", headers=headers) + request_headers = json.loads(r.data.decode("utf8")) + assert request_headers.get("User-Agent") == custom_ua + + # Do not modify pool headers when using the default user agent. + pool_headers = {"foo": "bar"} + pool.headers = pool_headers + r = pool.request("GET", "/headers") + request_headers = json.loads(r.data.decode("utf8")) + assert request_headers.get("User-Agent") == default_ua + assert "User-Agent" not in pool_headers + + pool.headers.update({"User-Agent": custom_ua2}) + r = pool.request("GET", "/headers") + request_headers = json.loads(r.data.decode("utf8")) + assert request_headers.get("User-Agent") == custom_ua2 + + def test_no_user_agent_header(self): + """ ConnectionPool can suppress sending a user agent header """ + custom_ua = "I'm not a web scraper, what are you talking about?" + with HTTPConnectionPool(self.host, self.port) as pool: + # Suppress user agent in the request headers. + no_ua_headers = {"User-Agent": SUPPRESS_USER_AGENT} + r = pool.request("GET", "/headers", headers=no_ua_headers) + request_headers = json.loads(r.data.decode("utf8")) + assert "User-Agent" not in request_headers + assert no_ua_headers["User-Agent"] == SUPPRESS_USER_AGENT + + # Suppress user agent in the pool headers. + pool.headers = no_ua_headers + r = pool.request("GET", "/headers") + request_headers = json.loads(r.data.decode("utf8")) + assert "User-Agent" not in request_headers + assert no_ua_headers["User-Agent"] == SUPPRESS_USER_AGENT + + # Request headers override pool headers. + pool_headers = {"User-Agent": custom_ua} + pool.headers = pool_headers + r = pool.request("GET", "/headers", headers=no_ua_headers) + request_headers = json.loads(r.data.decode("utf8")) + assert "User-Agent" not in request_headers + assert no_ua_headers["User-Agent"] == SUPPRESS_USER_AGENT + assert pool_headers.get("User-Agent") == custom_ua + class TestRetry(HTTPDummyServerTestCase): def test_max_retry(self): diff --git a/test/with_dummyserver/test_socketlevel.py b/test/with_dummyserver/test_socketlevel.py index b5a6a8b41b..de03930ec7 100644 --- a/test/with_dummyserver/test_socketlevel.py +++ b/test/with_dummyserver/test_socketlevel.py @@ -3,6 +3,7 @@ from urllib3 import HTTPConnectionPool, HTTPSConnectionPool from urllib3.connection import HTTPConnection from urllib3.poolmanager import proxy_from_url +from urllib3.connection import _get_default_user_agent from urllib3.exceptions import ( MaxRetryError, ProxyError, @@ -938,6 +939,7 @@ def echo_socket_handler(listener): b"Host: google.com", b"Accept-Encoding: identity", b"Accept: */*", + b"User-Agent: " + _get_default_user_agent().encode("utf-8"), b"", b"", ] @@ -1434,9 +1436,9 @@ def test_httplib_headers_case_insensitive(self): r = pool.request("GET", "/") assert HEADERS == dict(r.headers.items()) # to preserve case sensitivity - def test_headers_are_sent_with_the_original_case(self): - headers = {"foo": "bar", "bAz": "quux"} - parsed_headers = {} + def start_parsing_handler(self): + self.parsed_headers = OrderedDict() + self.received_headers = [] def socket_handler(listener): sock = listener.accept()[0] @@ -1445,11 +1447,13 @@ def socket_handler(listener): while not buf.endswith(b"\r\n\r\n"): buf += sock.recv(65536) - headers_list = [header for header in buf.split(b"\r\n")[1:] if header] + self.received_headers = [ + header for header in buf.split(b"\r\n")[1:] if header + ] - for header in headers_list: + for header in self.received_headers: (key, value) = header.split(b": ") - parsed_headers[key.decode("ascii")] = value.decode("ascii") + self.parsed_headers[key.decode("ascii")] = value.decode("ascii") sock.send( ("HTTP/1.1 204 No Content\r\nContent-Length: 0\r\n\r\n").encode("utf-8") @@ -1458,6 +1462,26 @@ def socket_handler(listener): sock.close() self._start_server(socket_handler) + + def test_headers_are_sent_with_the_original_case(self): + headers = {"foo": "bar", "bAz": "quux"} + + self.start_parsing_handler() + expected_headers = { + "Accept-Encoding": "identity", + "Host": "{0}:{1}".format(self.host, self.port), + "User-Agent": _get_default_user_agent(), + } + expected_headers.update(headers) + + with HTTPConnectionPool(self.host, self.port, retries=False) as pool: + pool.request("GET", "/", headers=HTTPHeaderDict(headers)) + assert expected_headers == self.parsed_headers + + def test_ua_header_can_be_overridden(self): + headers = {"uSeR-AgENt": "Definitely not urllib3!"} + + self.start_parsing_handler() expected_headers = { "Accept-Encoding": "identity", "Host": "{0}:{1}".format(self.host, self.port), @@ -1466,7 +1490,7 @@ def socket_handler(listener): with HTTPConnectionPool(self.host, self.port, retries=False) as pool: pool.request("GET", "/", headers=HTTPHeaderDict(headers)) - assert expected_headers == parsed_headers + assert expected_headers == self.parsed_headers def test_request_headers_are_sent_in_the_original_order(self): # NOTE: Probability this test gives a false negative is 1/(K!) @@ -1478,69 +1502,26 @@ def test_request_headers_are_sent_in_the_original_order(self): (u"X-Header-%d" % i, str(i)) for i in reversed(range(K)) ] - actual_request_headers = [] + def filter_non_x_headers(d): + return [(k, v) for (k, v) in d.items() if k.startswith("X-Header-")] - def socket_handler(listener): - sock = listener.accept()[0] + request_headers = OrderedDict() - buf = b"" - while not buf.endswith(b"\r\n\r\n"): - buf += sock.recv(65536) - - headers_list = [header for header in buf.split(b"\r\n")[1:] if header] - - for header in headers_list: - (key, value) = header.split(b": ") - if not key.decode("ascii").startswith(u"X-Header-"): - continue - actual_request_headers.append( - (key.decode("ascii"), value.decode("ascii")) - ) - - sock.send( - (u"HTTP/1.1 204 No Content\r\nContent-Length: 0\r\n\r\n").encode( - "ascii" - ) - ) - - sock.close() - - self._start_server(socket_handler) + self.start_parsing_handler() with HTTPConnectionPool(self.host, self.port, retries=False) as pool: pool.request("GET", "/", headers=OrderedDict(expected_request_headers)) - assert expected_request_headers == actual_request_headers + request_headers = filter_non_x_headers(self.parsed_headers) + assert expected_request_headers == request_headers @resolvesLocalhostFQDN def test_request_host_header_ignores_fqdn_dot(self): - - received_headers = [] - - def socket_handler(listener): - sock = listener.accept()[0] - - buf = b"" - while not buf.endswith(b"\r\n\r\n"): - buf += sock.recv(65536) - - for header in buf.split(b"\r\n")[1:]: - if header: - received_headers.append(header) - - sock.send( - (u"HTTP/1.1 204 No Content\r\nContent-Length: 0\r\n\r\n").encode( - "ascii" - ) - ) - - sock.close() - - self._start_server(socket_handler) + self.start_parsing_handler() with HTTPConnectionPool(self.host + ".", self.port, retries=False) as pool: pool.request("GET", "/") self.assert_header_received( - received_headers, "Host", "%s:%s" % (self.host, self.port) + self.received_headers, "Host", "%s:%s" % (self.host, self.port) ) def test_response_headers_are_returned_in_the_original_order(self): From 4a7040c1256d6ff8073627a4efb265447c9675fb Mon Sep 17 00:00:00 2001 From: Quentin Pradet Date: Tue, 30 Jun 2020 09:48:24 +0400 Subject: [PATCH 25/27] Clarify that behavior is identical with Python 2 --- src/urllib3/connectionpool.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/urllib3/connectionpool.py b/src/urllib3/connectionpool.py index 35f22b1118..ab74216aef 100644 --- a/src/urllib3/connectionpool.py +++ b/src/urllib3/connectionpool.py @@ -396,13 +396,14 @@ def _make_request( # We are swallowing BrokenPipeError (errno.EPIPE) since the server is # legitimately able to close the connection after sending a valid response. # With this behaviour, the received response is still readable. - except BrokenPipeError: # Python 3 + except BrokenPipeError: + # Python 3 pass except IOError as e: + # EPIPE and ESHUTDOWN are BrokenPipeError on Python 2, and EPROTOTYPE is + # needed on macOS # https://erickt.github.io/blog/2014/11/19/adventures-in-debugging-a-potential-osx-kernel-bug/ - if e.errno == errno.EPROTOTYPE: # macOS - pass - elif e.errno != errno.EPIPE: # Python 2 + if e.errno not in (errno.EPIPE, errno.ESHUTDOWN, errno.EPROTOTYPE): raise # Reset the timeout for the recv() on the socket From c6ae228c8e66d8526f1c489a59c1a4020ec1049f Mon Sep 17 00:00:00 2001 From: Seth Michael Larson Date: Fri, 28 Aug 2020 13:11:37 -0500 Subject: [PATCH 26/27] Skip BrokenPipeError test on Windows --- src/urllib3/connectionpool.py | 10 +++++++--- test/__init__.py | 13 +++++++++++++ test/with_dummyserver/test_socketlevel.py | 14 ++++++++++++-- 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/urllib3/connectionpool.py b/src/urllib3/connectionpool.py index ab74216aef..6f0208915b 100644 --- a/src/urllib3/connectionpool.py +++ b/src/urllib3/connectionpool.py @@ -400,10 +400,14 @@ def _make_request( # Python 3 pass except IOError as e: - # EPIPE and ESHUTDOWN are BrokenPipeError on Python 2, and EPROTOTYPE is - # needed on macOS + # Python 2 and macOS/Linux + # EPIPE and ESHUTDOWN are BrokenPipeError on Python 2, and EPROTOTYPE is needed on macOS # https://erickt.github.io/blog/2014/11/19/adventures-in-debugging-a-potential-osx-kernel-bug/ - if e.errno not in (errno.EPIPE, errno.ESHUTDOWN, errno.EPROTOTYPE): + if e.errno not in { + errno.EPIPE, + errno.ESHUTDOWN, + errno.EPROTOTYPE, + }: raise # Reset the timeout for the recv() on the socket diff --git a/test/__init__.py b/test/__init__.py index 01f02738d0..92a069b52d 100644 --- a/test/__init__.py +++ b/test/__init__.py @@ -152,6 +152,19 @@ def wrapper(*args, **kwargs): return wrapper +def notWindows(test): + """Skips this test when running on Windows""" + + @six.wraps(test) + def wrapper(*args, **kwargs): + msg = "{name} does not run on Windows".format(name=test.__name__) + if platform.system() == "Windows": + pytest.skip(msg) + return test(*args, **kwargs) + + return wrapper + + def notOpenSSL098(test): """Skips this test for Python 3.5 macOS python.org distribution""" diff --git a/test/with_dummyserver/test_socketlevel.py b/test/with_dummyserver/test_socketlevel.py index de03930ec7..3b807aec83 100644 --- a/test/with_dummyserver/test_socketlevel.py +++ b/test/with_dummyserver/test_socketlevel.py @@ -57,6 +57,7 @@ class MimeToolMessage(object): LONG_TIMEOUT, notPyPy2, notSecureTransport, + notWindows, resolvesLocalhostFQDN, ) @@ -1783,7 +1784,10 @@ def socket_handler(listener): class TestBrokenPipe(SocketDummyServerTestCase): - def test_broken_pipe_ignore(self, monkeypatch): + @notWindows + def test_ignore_broken_pipe_errors(self, monkeypatch): + # On Windows an aborted connection raises an error on + # attempts to read data out of a socket that's been closed. sock_shut = Event() orig_connect = HTTPConnection.connect # a buffer that will cause two sendall calls @@ -1800,8 +1804,9 @@ def socket_handler(listener): sock.send( b"HTTP/1.1 404 Not Found\r\n" b"Connection: close\r\n" - b"Content-Length: 0\r\n" + b"Content-Length: 10\r\n" b"\r\n" + b"xxxxxxxxxx" ) sock.shutdown(socket.SHUT_RDWR) sock_shut.set() @@ -1812,5 +1817,10 @@ def socket_handler(listener): with HTTPConnectionPool(self.host, self.port) as pool: r = pool.request("POST", "/", body=buf) assert r.status == 404 + assert r.headers["content-length"] == "10" + assert r.data == b"xxxxxxxxxx" + r = pool.request("POST", "/admin", chunked=True, body=buf) assert r.status == 404 + assert r.headers["content-length"] == "10" + assert r.data == b"xxxxxxxxxx" From 2335967ae977ac60abb3b27ad8f945b7308eaed1 Mon Sep 17 00:00:00 2001 From: Seth Michael Larson Date: Fri, 11 Sep 2020 08:58:41 -0500 Subject: [PATCH 27/27] De-duplicate notWindows decorator --- test/__init__.py | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/test/__init__.py b/test/__init__.py index 180234a96b..5ab3cac1bf 100644 --- a/test/__init__.py +++ b/test/__init__.py @@ -148,7 +148,7 @@ def notWindows(test): @six.wraps(test) def wrapper(*args, **kwargs): - msg = "{} is flaky on Windows".format(test.__name__) + msg = "{name} does not run on Windows".format(name=test.__name__) if platform.system() == "Windows": pytest.skip(msg) return test(*args, **kwargs) @@ -179,19 +179,6 @@ def wrapper(*args, **kwargs): return wrapper -def notWindows(test): - """Skips this test when running on Windows""" - - @six.wraps(test) - def wrapper(*args, **kwargs): - msg = "{name} does not run on Windows".format(name=test.__name__) - if platform.system() == "Windows": - pytest.skip(msg) - return test(*args, **kwargs) - - return wrapper - - def notOpenSSL098(test): """Skips this test for Python 3.5 macOS python.org distribution"""