# HG changeset patch # Parent eaa38b75cc7812dbf079801657f6eae9ebb85157 Issue 3566: Improve support for persistent connections * Add http.client.ConnectionClosed exception * HTTPConnection.close() implicitly called * request() ConnectionError does not imply server did not send a response * ConnectionClosed wraps ECONNRESET from first recv() of status line * Added HTTPConnection.closed, determining if a fresh connection will be made * Fixed http.client.__all__ and added a test diff -r eaa38b75cc78 Doc/library/http.client.rst --- a/Doc/library/http.client.rst Fri Jan 23 17:30:26 2015 -0500 +++ b/Doc/library/http.client.rst Sat Jan 31 12:35:55 2015 +0000 @@ -169,6 +169,18 @@ status code that we don't understand. +.. exception:: ConnectionClosed + + A subclass of :exc:`ConnectionError`. Raised by + :meth:`HTTPConnection.getresponse` when a connection is closed before + any response is received. + + .. versionadded:: 3.5 + Previously, :exc:`BadStatusLine` or :exc:`ConnectionError` was raised. + This is also a subclass of :exc:`BadStatusLine` for + backwards compatibility. + + The constants defined in this module are: .. data:: HTTP_PORT @@ -226,11 +238,19 @@ Should be called after a request is sent to get the response from the server. Returns an :class:`HTTPResponse` instance. + If the connection was closed without any response received, + :exc:`ConnectionClosed` is raised, and the :class:`HTTPConnection` object + will be ready to reconnect when a new request is sent. + .. note:: Note that you must have read the whole response before you can send a new request to the server. + .. versionchanged:: 3.5 + The :meth:`close` method no longer needs to be called to reconnect + after a connection is dropped without a response. + .. method:: HTTPConnection.set_debuglevel(level) @@ -269,13 +289,23 @@ .. method:: HTTPConnection.connect() - Connect to the server specified when the object was created. + Connect to the server specified when the object was created. Normally + called automatically when making a request and the client is + not connected. .. method:: HTTPConnection.close() Close the connection to the server. + +.. attribute:: HTTPConnection.closed + + Is ``True`` if the connection is closed, or if the connection is only open + to receive the body of the last response. When ``True``, a + fresh connection will be automatically opened when the next request + is made, otherwise the existing connection will be used. + As an alternative to using the :meth:`request` method described above, you can also send your request step by step, by using the four functions below. diff -r eaa38b75cc78 Lib/http/client.py --- a/Lib/http/client.py Fri Jan 23 17:30:26 2015 -0500 +++ b/Lib/http/client.py Sat Jan 31 12:35:55 2015 +0000 @@ -20,10 +20,12 @@ | ( putheader() )* endheaders() v Request-sent - | - | response = getresponse() - v - Unread-response [Response-headers-read] + |\_____________________________ + | | getresponse() raises + | response = getresponse() | ConnectionError + v v + Unread-response Idle + [Response-headers-read] |\____________________ | | | response.read() | putrequest() @@ -75,12 +77,12 @@ import collections from urllib.parse import urlsplit -__all__ = ["HTTPResponse", "HTTPConnection", +__all__ = ["HTTPResponse", "HTTPConnection", "HTTPMessage", "HTTPException", "NotConnected", "UnknownProtocol", "UnknownTransferEncoding", "UnimplementedFileMode", "IncompleteRead", "InvalidURL", "ImproperConnectionState", "CannotSendRequest", "CannotSendHeader", "ResponseNotReady", - "BadStatusLine", "error", "responses"] + "BadStatusLine", "ConnectionClosed", "error", "responses"] HTTP_PORT = 80 HTTPS_PORT = 443 @@ -238,6 +240,15 @@ # we've already started reading the response return + # Peek at response before parsing it to differentiate ECONNRESET with + # no response from ECONNRESET after a few bytes of response + try: + response = self.fp.peek(1) + except ConnectionError as err: + raise ConnectionClosed(err) from err + if not response: + raise ConnectionClosed("Connection closed without response") + # read until we get a non-100 response while True: version, status, reason = self._read_status() @@ -795,13 +806,18 @@ self.__response = None self.__state = _CS_IDLE + @property + def closed(self): + """True if no connection is open""" + return self.sock is None + def send(self, data): """Send `data' to the server. ``data`` can be a string object, a bytes object, an array object, a file-like object that supports a .read() method, or an iterable object. """ - if self.sock is None: + if self.closed: if self.auto_open: self.connect() else: @@ -1107,7 +1123,11 @@ response = self.response_class(self.sock, method=self._method) try: - response.begin() + try: + response.begin() + except ConnectionClosed: + self.close() + raise assert response.will_close != _UNKNOWN self.__state = _CS_IDLE @@ -1239,5 +1259,10 @@ HTTPException.__init__(self, "got more than %d bytes when reading %s" % (_MAXLINE, line_type)) +class ConnectionClosed(ConnectionError, BadStatusLine): + def __init__(self, *pos, **kw): + BadStatusLine.__init__(self, "") + ConnectionError.__init__(self, *pos, **kw) + # for backwards compatibility error = HTTPException diff -r eaa38b75cc78 Lib/test/test_httplib.py --- a/Lib/test/test_httplib.py Fri Jan 23 17:30:26 2015 -0500 +++ b/Lib/test/test_httplib.py Sat Jan 31 12:35:55 2015 +0000 @@ -40,12 +40,21 @@ HOST = support.HOST +class RecvStream(io.RawIOBase): + def __init__(self, text): + self.reader = io.BytesIO(text) + + def readable(self): + return True + + def readinto(self, buffer): + return self.reader.readinto(buffer) + class FakeSocket: - def __init__(self, text, fileclass=io.BytesIO, host=None, port=None): + def __init__(self, text, fileclass=RecvStream, host=None, port=None): if isinstance(text, str): text = text.encode("ascii") - self.text = text - self.fileclass = fileclass + self.recv_stream = fileclass(text) self.data = b'' self.sendall_calls = 0 self.file_closed = False @@ -60,7 +69,7 @@ if mode != 'r' and mode != 'rb': raise client.UnimplementedFileMode() # keep the file around so we can check how much was read from it - self.file = self.fileclass(self.text) + self.file = io.BufferedReader(self.recv_stream) self.file.close = self.file_close #nerf close () return self.file @@ -88,23 +97,41 @@ def close(self): pass -class NoEOFBytesIO(io.BytesIO): - """Like BytesIO, but raises AssertionError on EOF. +class NoEOFRecvStream(RecvStream): + """Like RecvStream, but raises AssertionError on EOF. This is used below to test that http.client doesn't try to read more from the underlying file than it should. """ - def read(self, n=-1): - data = io.BytesIO.read(self, n) - if data == b'': + def readinto(self, buffer): + length = super().readinto(buffer) + if length == 0: raise AssertionError('caller tried to read past EOF') - return data + return length - def readline(self, length=None): - data = io.BytesIO.readline(self, length) - if data == b'': - raise AssertionError('caller tried to read past EOF') - return data +class TestHTTPConnection(client.HTTPConnection): + """HTTPConnection subclass using FakeSocket; counts connect() calls""" + + def __init__(self, *args): + self.connections = 0 + super().__init__('example.com') + self.fake_socket_args = args + self._create_connection = self.create_connection + + def connect(self): + """Count the number of times connect() is invoked""" + self.connections += 1 + return super().connect() + + def create_connection(self, *pos, **kw): + return FakeSocket(*self.fake_socket_args) + +BASIC_RESPONSE = ( + b'HTTP/1.1 200 OK\r\n' + b'Content-Length: 12\r\n' + b'\r\n' + b'Dummy body\r\n' +) class HeaderTests(TestCase): def test_auto_headers(self): @@ -192,6 +219,18 @@ class BasicTest(TestCase): + def test_all(self): + # Documented objects defined in the module should be in __all__ + expected = ["responses"] # White-list documented dict() object + blacklist = {"parse_headers", "LineTooLong"} # Undocumented objects + for name in dir(client): + if name in blacklist: + continue + object = getattr(client, name) + if getattr(object, "__module__", None) == "http.client": + expected.append(name) + self.assertCountEqual(client.__all__, expected) + def test_status_lines(self): # Test HTTP status lines @@ -364,7 +403,7 @@ 'HTTP/1.1 200 OK\r\n' 'Content-Length: 14432\r\n' '\r\n', - NoEOFBytesIO) + NoEOFRecvStream) resp = client.HTTPResponse(sock, method="HEAD") resp.begin() if resp.read(): @@ -377,7 +416,7 @@ 'HTTP/1.1 200 OK\r\n' 'Content-Length: 14432\r\n' '\r\n', - NoEOFBytesIO) + NoEOFRecvStream) resp = client.HTTPResponse(sock, method="HEAD") resp.begin() b = bytearray(5) @@ -400,7 +439,7 @@ with open(__file__, 'rb') as body: conn = client.HTTPConnection('example.com') - sock = FakeSocket(body) + sock = FakeSocket(b'') conn.sock = sock conn.request('GET', '/foo', body) self.assertTrue(sock.data.startswith(expected), '%r != %r' % @@ -464,6 +503,17 @@ with self.assertRaises(TypeError): conn.request('POST', 'test', conn) + def test_close(self): + conn = TestHTTPConnection(BASIC_RESPONSE) + self.assertTrue(conn.closed) + conn.request('GET', '/') + with conn.getresponse() as resp: + self.assertFalse(conn.closed) + resp.read() + self.assertFalse(conn.closed) + conn.close() + self.assertTrue(conn.closed) + def test_chunked(self): expected = chunked_expected sock = FakeSocket(chunked_start + last_chunk + chunked_end) @@ -671,7 +721,7 @@ response = self # Avoid garbage collector closing the socket client.HTTPResponse.__init__(self, *pos, **kw) conn.response_class = Response - conn.sock = FakeSocket('') # Emulate server dropping connection + conn.sock = FakeSocket('Invalid status line') conn.request('GET', '/') self.assertRaises(client.BadStatusLine, conn.getresponse) self.assertTrue(response.closed) @@ -992,6 +1042,91 @@ httpConn.close() +class PersistenceTest(TestCase): + def test_reuse_reconnect(self): + # Should reuse or reconnect depending on header from server + tests = ( + ('1.0', None, False), + ('1.0', 'keep-alive', True), + ('1.0', 'keep-ALIVE', True), # Test case insensitivity + ('1.1', None, True), + ('1.1', 'close', False), + ('1.1', 'cloSE', False), # Test case insensitivity + ) + for version, header, reuse in tests: + with self.subTest(version=version, header=header): + if header: + header = 'Connection: {}\r\n'.format(header) + else: + header = '' + msg = ( + 'HTTP/{} 200 OK\r\n' + '{}' + 'Content-Length: 12\r\n' + '\r\n' + 'Dummy body\r\n' + ) + msg = msg.format(version, header) + conn = TestHTTPConnection(msg) + self.assertTrue(conn.closed) + conn.request('GET', '/open-connection') + with conn.getresponse() as response: + self.assertEqual(conn.closed, not reuse) + response.read() + self.assertEqual(conn.closed, not reuse) + self.assertEqual(conn.connections, 1) + conn.request('GET', '/subsequent-request') + if reuse: + self.assertEqual(conn.connections, 1) + else: + self.assertEqual(conn.connections, 2) + + def test_connection_closed(self): + class RecvOrResetStream(RecvStream): + def readinto(self, buffer): + length = super().readinto(buffer) + if not length: + raise ConnectionResetError() + return length + nonpersist_exception = (client.BadStatusLine, ConnectionError)### + for stream_class in (RecvStream, RecvOrResetStream): + with self.subTest(stream_class): + conn = TestHTTPConnection(b'', stream_class) + conn.request('GET', '/eof-response') + self.assertRaises(client.ConnectionClosed, conn.getresponse) + self.assertTrue(conn.closed) + + # HTTPConnection.connect() should be automatically invoked + conn.request('GET', '/reconnect') + self.assertEqual(conn.connections, 2) + + # No ConnectionClosed if status line partly received + conn = TestHTTPConnection(b'H', stream_class) + conn.request('GET', '/partial-status') + with self.assertRaises(nonpersist_exception) as context: + conn.getresponse() + self.assertNotIsInstance(context.exception, + client.ConnectionClosed) + + def test_100_close(self): + # No ConnectionClosed after 100 Continue response + conn = TestHTTPConnection( + b'HTTP/1.1 100 Continue\r\n' + b'\r\n' + # Missing final response + ) + conn.request('GET', '/100-continue-response', + headers={'Expect': '100-continue'}) + try: + conn.getresponse() + except client.ConnectionClosed: + raise + except client.HTTPException: + pass + else: + self.fail("Expected non-ConnectionClosed HTTPException") + + class HTTPSTest(TestCase): def setUp(self): @@ -1298,6 +1433,7 @@ @support.reap_threads def test_main(verbose=None): support.run_unittest(HeaderTests, OfflineTest, BasicTest, TimeoutTest, + PersistenceTest, HTTPSTest, RequestBodyTest, SourceAddressTest, HTTPResponseTest, ExtendedReadTest, ExtendedReadTestChunked, TunnelTests) diff -r eaa38b75cc78 Lib/test/test_urllib.py --- a/Lib/test/test_urllib.py Fri Jan 23 17:30:26 2015 -0500 +++ b/Lib/test/test_urllib.py Sat Jan 31 12:35:55 2015 +0000 @@ -9,6 +9,7 @@ import unittest from unittest.mock import patch from test import support +from test import test_httplib import os try: import ssl @@ -53,9 +54,12 @@ def fakehttp(fakedata): - class FakeSocket(io.BytesIO): + class FakeSocket(io.BufferedReader): io_refs = 1 + def __init__(self, data): + super().__init__(test_httplib.RecvStream(data)) + def sendall(self, data): FakeHTTPConnection.buf = data @@ -63,20 +67,10 @@ self.io_refs += 1 return self - def read(self, amt=None): - if self.closed: - return b"" - return io.BytesIO.read(self, amt) - - def readline(self, length=None): - if self.closed: - return b"" - return io.BytesIO.readline(self, length) - def close(self): self.io_refs -= 1 if self.io_refs == 0: - io.BytesIO.close(self) + super().close() class FakeHTTPConnection(http.client.HTTPConnection): diff -r eaa38b75cc78 Lib/xmlrpc/client.py --- a/Lib/xmlrpc/client.py Fri Jan 23 17:30:26 2015 -0500 +++ b/Lib/xmlrpc/client.py Sat Jan 31 12:35:55 2015 +0000 @@ -1143,7 +1143,7 @@ if i or e.errno not in (errno.ECONNRESET, errno.ECONNABORTED, errno.EPIPE): raise - except http.client.BadStatusLine: #close after we sent request + except http.client.ConnectionClosed: #close without response if i: raise