From: Ben Darnell Date: Sun, 10 Aug 2014 18:02:39 +0000 (-0400) Subject: Correctly handle 204 response codes that do not include a content-length. X-Git-Tag: v4.0.1~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c7ae10d668f10cb5797fa88eeca9b3c63644b1cb;p=thirdparty%2Ftornado.git Correctly handle 204 response codes that do not include a content-length. If a server sends a 204 with no content-length we would wait for the server to close the connection instead of ending the request (any such servers are ignoring the "Connection: close" header we send, but apparently exist). Move some content-length logic from simple_httpclient.py to http1connection.py. Fix the client-side use of on_connection_close; this affects the handling of any HTTPInputException. This fixes regressions relative to Tornado 3.2. Conflicts: tornado/websocket.py --- diff --git a/tornado/http1connection.py b/tornado/http1connection.py index 72f729d78..1ac24f520 100644 --- a/tornado/http1connection.py +++ b/tornado/http1connection.py @@ -21,6 +21,8 @@ from __future__ import absolute_import, division, print_function, with_statement +import re + from tornado.concurrent import Future from tornado.escape import native_str, utf8 from tornado import gen @@ -191,8 +193,17 @@ class HTTP1Connection(httputil.HTTPConnection): skip_body = True code = start_line.code if code == 304: + # 304 responses may include the content-length header + # but do not actually have a body. + # http://tools.ietf.org/html/rfc7230#section-3.3 skip_body = True if code >= 100 and code < 200: + # 1xx responses should never indicate the presence of + # a body. + if ('Content-Length' in headers or + 'Transfer-Encoding' in headers): + raise httputil.HTTPInputError( + "Response code %d cannot have body" % code) # TODO: client delegates will get headers_received twice # in the case of a 100-continue. Document or change? yield self._read_message(delegate) @@ -201,7 +212,8 @@ class HTTP1Connection(httputil.HTTPConnection): not self._write_finished): self.stream.write(b"HTTP/1.1 100 (Continue)\r\n\r\n") if not skip_body: - body_future = self._read_body(headers, delegate) + body_future = self._read_body( + start_line.code if self.is_client else 0, headers, delegate) if body_future is not None: if self._body_timeout is None: yield body_future @@ -482,12 +494,36 @@ class HTTP1Connection(httputil.HTTPConnection): data[eol:100]) return start_line, headers - def _read_body(self, headers, delegate): - content_length = headers.get("Content-Length") - if content_length: - content_length = int(content_length) + def _read_body(self, code, headers, delegate): + if "Content-Length" in headers: + if "," in headers["Content-Length"]: + # Proxies sometimes cause Content-Length headers to get + # duplicated. If all the values are identical then we can + # use them but if they differ it's an error. + pieces = re.split(r',\s*', headers["Content-Length"]) + if any(i != pieces[0] for i in pieces): + raise httputil.HTTPInputError( + "Multiple unequal Content-Lengths: %r" % + headers["Content-Length"]) + headers["Content-Length"] = pieces[0] + content_length = int(headers["Content-Length"]) + if content_length > self._max_body_size: raise httputil.HTTPInputError("Content-Length too long") + else: + content_length = None + + if code == 204: + # This response code is not allowed to have a non-empty body, + # and has an implicit length of zero instead of read-until-close. + # http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.3 + if ("Transfer-Encoding" in headers or + content_length not in (None, 0)): + raise httputil.HTTPInputError( + "Response with code %d should not have body" % code) + content_length = 0 + + if content_length is not None: return self._read_fixed_body(content_length, delegate) if headers.get("Transfer-Encoding") == "chunked": return self._read_chunked_body(delegate) @@ -582,6 +618,9 @@ class _GzipMessageDelegate(httputil.HTTPMessageDelegate): self._delegate.data_received(tail) return self._delegate.finish() + def on_connection_close(self): + return self._delegate.on_connection_close() + class HTTP1ServerConnection(object): """An HTTP/1.x server.""" diff --git a/tornado/simple_httpclient.py b/tornado/simple_httpclient.py index 99f1c1a78..516dc20b6 100644 --- a/tornado/simple_httpclient.py +++ b/tornado/simple_httpclient.py @@ -277,7 +277,7 @@ class _HTTPConnection(httputil.HTTPMessageDelegate): stream.close() return self.stream = stream - self.stream.set_close_callback(self._on_close) + self.stream.set_close_callback(self.on_connection_close) self._remove_timeout() if self.final_callback is None: return @@ -418,12 +418,15 @@ class _HTTPConnection(httputil.HTTPMessageDelegate): # pass it along, unless it's just the stream being closed. return isinstance(value, StreamClosedError) - def _on_close(self): + def on_connection_close(self): if self.final_callback is not None: message = "Connection closed" if self.stream.error: raise self.stream.error - raise HTTPError(599, message) + try: + raise HTTPError(599, message) + except HTTPError: + self._handle_exception(*sys.exc_info()) def headers_received(self, first_line, headers): if self.request.expect_100_continue and first_line.code == 100: @@ -433,20 +436,6 @@ class _HTTPConnection(httputil.HTTPMessageDelegate): self.code = first_line.code self.reason = first_line.reason - if "Content-Length" in self.headers: - if "," in self.headers["Content-Length"]: - # Proxies sometimes cause Content-Length headers to get - # duplicated. If all the values are identical then we can - # use them but if they differ it's an error. - pieces = re.split(r',\s*', self.headers["Content-Length"]) - if any(i != pieces[0] for i in pieces): - raise ValueError("Multiple unequal Content-Lengths: %r" % - self.headers["Content-Length"]) - self.headers["Content-Length"] = pieces[0] - content_length = int(self.headers["Content-Length"]) - else: - content_length = None - if self.request.header_callback is not None: # Reassemble the start line. self.request.header_callback('%s %s %s\r\n' % first_line) @@ -454,14 +443,6 @@ class _HTTPConnection(httputil.HTTPMessageDelegate): self.request.header_callback("%s: %s\r\n" % (k, v)) self.request.header_callback('\r\n') - if 100 <= self.code < 200 or self.code == 204: - # These response codes never have bodies - # http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.3 - if ("Transfer-Encoding" in self.headers or - content_length not in (None, 0)): - raise ValueError("Response with code %d should not have body" % - self.code) - def finish(self): data = b''.join(self.chunks) self._remove_timeout() diff --git a/tornado/test/simple_httpclient_test.py b/tornado/test/simple_httpclient_test.py index f17da7e02..d3af70f1c 100644 --- a/tornado/test/simple_httpclient_test.py +++ b/tornado/test/simple_httpclient_test.py @@ -294,10 +294,13 @@ class SimpleHTTPClientTestMixin(object): self.assertEqual(response.code, 204) # 204 status doesn't need a content-length, but tornado will # add a zero content-length anyway. + # + # A test without a content-length header is included below + # in HTTP204NoContentTestCase. self.assertEqual(response.headers["Content-length"], "0") # 204 status with non-zero content length is malformed - with ExpectLog(app_log, "Uncaught exception"): + with ExpectLog(gen_log, "Malformed HTTP message"): response = self.fetch("/no_content?error=1") self.assertEqual(response.code, 599) @@ -476,6 +479,27 @@ class HTTP100ContinueTestCase(AsyncHTTPTestCase): self.assertEqual(res.body, b'A') +class HTTP204NoContentTestCase(AsyncHTTPTestCase): + def respond_204(self, request): + # A 204 response never has a body, even if doesn't have a content-length + # (which would otherwise mean read-until-close). Tornado always + # sends a content-length, so we simulate here a server that sends + # no content length and does not close the connection. + # + # Tests of a 204 response with a Content-Length header are included + # in SimpleHTTPClientTestMixin. + request.connection.stream.write( + b"HTTP/1.1 204 No content\r\n\r\n") + + def get_app(self): + return self.respond_204 + + def test_204_no_content(self): + resp = self.fetch('/') + self.assertEqual(resp.code, 204) + self.assertEqual(resp.body, b'') + + class HostnameMappingTestCase(AsyncHTTPTestCase): def setUp(self): super(HostnameMappingTestCase, self).setUp() diff --git a/tornado/websocket.py b/tornado/websocket.py index bc1a76660..ed520d586 100644 --- a/tornado/websocket.py +++ b/tornado/websocket.py @@ -701,10 +701,12 @@ class WebSocketClientConnection(simple_httpclient._HTTPConnection): self.protocol.close(code, reason) self.protocol = None - def _on_close(self): + def on_connection_close(self): + if not self.connect_future.done(): + self.connect_future.set_exception(StreamClosedError()) self.on_message(None) self.tcp_client.close() - super(WebSocketClientConnection, self)._on_close() + super(WebSocketClientConnection, self).on_connection_close() def _on_http_response(self, response): if not self.connect_future.done(): @@ -733,7 +735,7 @@ class WebSocketClientConnection(simple_httpclient._HTTPConnection): self._timeout = None self.stream = self.connection.detach() - self.stream.set_close_callback(self._on_close) + self.stream.set_close_callback(self.on_connection_close) # Once we've taken over the connection, clear the final callback # we set on the http request. This deactivates the error handling # in simple_httpclient that would otherwise interfere with our