send_error: skip the default error page for 204 No Content (issue #3360)

tornado/tcpclient.py

@@ -273,12 +273,39 @@ async def connect(
         # the same host. (http://tools.ietf.org/html/rfc6555#section-4.2)
         if ssl_options is not None:
             if timeout is not None:
-                stream = await gen.with_timeout(
-                    timeout,
-                    stream.start_tls(
-                        False, ssl_options=ssl_options, server_hostname=host
-                    ),
+                # Issue #3614: IOStream.start_tls transfers socket ownership
+                # to a new SSLIOStream *before* the handshake completes and
+                # sets self.socket to None on the original stream. If the
+                # TLS handshake times out, gen.with_timeout raises
+                # TimeoutError to the caller; the original stream is now a
+                # no-op (its socket is gone) and the new SSLIOStream that
+                # actually owns the socket is only reachable through the
+                # inner future -- which gen.with_timeout deliberately
+                # leaves running. Without an explicit close, the socket
+                # is leaked.
+                #
+                # Capture the future returned by start_tls so that, on
+                # timeout, we can close the SSLIOStream that owns the
+                # underlying socket.
+                tls_future = stream.start_tls(
+                    False, ssl_options=ssl_options, server_hostname=host
                 )
+                try:
+                    stream = await gen.with_timeout(timeout, tls_future)
+                except TimeoutError:
+                    # The handshake is still pending (with_timeout does not
+                    # cancel the wrapped future). When it eventually
+                    # completes, the result is the SSLIOStream that owns
+                    # the underlying socket -- closing it releases the fd.
+                    # If the future ends up failing, the SSLIOStream
+                    # already closes its socket as part of the failure
+                    # path, so there is nothing to do.
+                    def _close_if_succeeded(f: Future[IOStream]) -> None:
+                        if f.exception() is None:
+                            f.result().close()
+
+                    future_add_done_callback(tls_future, _close_if_succeeded)
+                    raise
             else:
                 stream = await stream.start_tls(
                     False, ssl_options=ssl_options, server_hostname=host

tornado/test/tcpclient_test.py

@@ -14,10 +14,12 @@
 # under the License.
 import getpass
 import socket
+import ssl
 import typing
 import unittest
 from contextlib import closing
 
+from tornado import gen
 from tornado.concurrent import Future
 from tornado.gen import TimeoutError
 from tornado.iostream import IOStream
@@ -173,6 +175,68 @@ def resolve(self, *args, **kwargs):
                 "1.2.3.4", 12345, timeout=timeout
             )
 
+    @gen_test
+    def test_tls_handshake_timeout_closes_ssl_stream(self):
+        # Regression test for issue #3614: when TCPClient.connect is
+        # called with both ssl_options and a timeout, a TLS handshake
+        # timeout must close the SSLIOStream that owns the underlying
+        # socket, not leak it.
+        port = self.start_server(socket.AF_INET)
+
+        original_start_tls = IOStream.start_tls
+        closed_streams: list[IOStream] = []
+        tls_future_holder: list[Future[IOStream]] = []
+
+        def fake_start_tls(
+            self: IOStream,
+            server_side: bool,
+            ssl_options: typing.Any = None,
+            server_hostname: typing.Any = None,
+        ) -> Future[IOStream]:
+            from tornado.iostream import SSLIOStream
+
+            real_socket = self.socket
+            self.io_loop.remove_handler(real_socket)
+            self.socket = None  # type: ignore[assignment]
+            ssl_stream = SSLIOStream(real_socket, ssl_options=ssl_options)
+            original = self._close_callback
+            if original is not None:
+                ssl_stream.set_close_callback(original)
+            real_close = ssl_stream.close
+
+            def tracking_close(*args: typing.Any, **kwargs: typing.Any) -> None:
+                closed_streams.append(ssl_stream)
+                real_close(*args, **kwargs)
+
+            ssl_stream.close = tracking_close  # type: ignore[method-assign]
+            tls_future: Future[IOStream] = Future()
+            tls_future_holder.append(tls_future)
+            return tls_future
+
+        IOStream.start_tls = fake_start_tls  # type: ignore[method-assign]
+        try:
+            with self.assertRaises(TimeoutError):
+                yield self.client.connect(
+                    "127.0.0.1",
+                    port,
+                    ssl_options=dict(cert_reqs=ssl.CERT_NONE),
+                    timeout=0.05,
+                )
+            # The handshake future is still pending. Resolve it so the
+            # cleanup callback registered by the timeout handler runs
+            # and closes the SSLIOStream that owns the socket.
+            self.assertEqual(len(tls_future_holder), 1)
+            tls_future_holder[0].set_result(None)  # type: ignore[arg-type]
+            yield gen.sleep(0)
+        finally:
+            IOStream.start_tls = original_start_tls  # type: ignore[method-assign]
+        self.assertEqual(
+            len(closed_streams),
+            1,
+            "SSLIOStream owning the underlying socket was not closed on "
+            "TLS handshake timeout (issue #3614)",
+        )
+
 
 class TestConnectorSplit(unittest.TestCase):
     def test_one_family(self):

tornado/test/web_test.py

@@ -1809,6 +1809,27 @@ def test_204_headers(self):
         self.assertNotIn("Transfer-Encoding", response.headers)
 
 
+class HTTPError204Test(SimpleHandlerTestCase):
+    """Regression test for issue #3360: raising HTTPError(204) used to
+    route through send_error -> write_error -> default HTML body, then
+    trip the "Cannot send body with 204" assertion in finish() (or, under
+    ``python -O``, produce a malformed response with a body and no
+    Content-Length header). send_error must skip write_error for status
+    codes that must not carry a body.
+    """
+
+    class Handler(RequestHandler):
+        def get(self):
+            raise HTTPError(204)
+
+    def test_http_error_204(self):
+        response = self.fetch("/")
+        self.assertEqual(response.code, 204)
+        self.assertNotIn("Content-Length", response.headers)
+        self.assertNotIn("Transfer-Encoding", response.headers)
+        self.assertEqual(response.body, b"")
+
+
 class Header304Test(SimpleHandlerTestCase):
     class Handler(RequestHandler):
         def get(self):

tornado/web.py

@@ -1357,7 +1357,7 @@ def send_error(self, status_code: int = 500, **kwargs: Any) -> None:
                 reason = exception.reason
         self.set_status(status_code, reason=reason)
         try:
-            if status_code != 304:
+            if status_code != 304 and status_code != 204:
                 self.write_error(status_code, **kwargs)
         except Exception:
             app_log.error("Uncaught exception in write_error", exc_info=True)