fix(tcpclient): close SSLIOStream on TLS handshake timeout to prevent socket leak#3615
fix(tcpclient): close SSLIOStream on TLS handshake timeout to prevent socket leak#3615armorbreak001 wants to merge 3 commits into
Conversation
… socket leak When TCPClient.connect() times out during the TLS handshake, the socket is transferred to an SSLIOStream inside start_tls() but never cleaned up because gen.with_timeout does not cancel the wrapped future. This leaves the socket fd permanently leaked. Save the start_tls future and cancel it on TimeoutError, which triggers cleanup of the underlying SSLIOStream via a new cancel callback registered in IOStream.start_tls(). Fixes tornadoweb#3614
| ssl_stream.read_chunk_size = self.read_chunk_size | ||
|
|
||
| def _on_cancel(fut: Future[SSLIOStream]) -> None: | ||
| if fut.cancelled(): |
There was a problem hiding this comment.
This needs a test to make sure we are in fact closing the stream here. I think that when tornado.gen.with_timeout reaches its timeout, the Future is terminated with a TimeoutError, but not cancelled, so this condition is never true (the call to tls_future.cancel() in tcpclient is a no-op because the future has already been terminated).
What we probably want to do here is close the stream in a done_callback if fut.exception() is None.
…out test The _on_cancel callback in start_tls() calls ssl_stream.close() when the TLS future is cancelled (timeout). However, _signal_closed() calls _ssl_connect_future.exception() which raises CancelledError on cancelled futures, preventing proper cleanup. Wrap the exception() call in try/except CancelledError to ensure close() completes successfully even when the SSL connect future was cancelled. Add test_tls_handshake_timeout_closes_stream to verify that: - TimeoutError is raised on TLS handshake timeout - The SSLIOStream is properly closed (no fd leak) - Addresses bdarnell's review feedback about needing a test
|
Thanks again for the review, @bdarnell. I've added a test and also found/fixed a related bug: New test: Additional fix: While writing the test, I discovered that The test passes along with all existing tcpclient tests (27 passed). |
…mport cleanup - Cancel TLS future on timeout to prevent stale coroutine (bdarnell) - Add @skipIfNonUnix for /proc/self/fd test (platform-specific) - Move ssl import to module level (style)
|
Hi @bdarnell, thanks for the review on #3636. I've updated this PR (#3615) with the following changes based on your feedback:
Closed #3636 as superseded. |
| # under the License. | ||
| import getpass | ||
| import os | ||
| import ssl as _ssl |
There was a problem hiding this comment.
Good catch — changed import ssl as _ssl to import ssl to match the codebase convention (all other files use plain import ssl).
| except gen.TimeoutError: | ||
| # Cancel the TLS handshake coroutine so it doesn't | ||
| # continue running with a stale _tls_stream reference. | ||
| tls_future.cancel() |
There was a problem hiding this comment.
Do we need both tls_future.cancel and tls_stream.close? I think we'd only need tls_future.cancel if start_tls had a try/finally to do its own cleanup. Or, as I said in #3636 (comment), I think it might be cleaner to move timeout awareness into start_tls instead of trying to spread it across layers like this.
There was a problem hiding this comment.
You're right — tls_future.cancel() is a no-op here since gen.with_timeout resolves the future with TimeoutError (doesn't cancel it). Removed it. The stream.close() is what actually prevents the fd leak.
I agree that moving timeout awareness into start_tls would be cleaner architecturally — that feels like a follow-up/refactor since it's a bigger change. Want me to do that in this PR or keep it scoped to the fix?
|
I'm not going to babysit someone else's bot. |
2 participants
Summary
When
TCPClient.connect()is called with bothssl_optionsandtimeout, a TLS handshake timeout causes the underlying socket to leak. The socket becomes unreachable from the caller and is never closed.Root cause:
IOStream.start_tls()transfers socket ownership to a newSSLIOStreambefore the TLS handshake completes (the raw socket is extracted, the original stream setsself.socket = None, and an SSLIOStream wrapping the socket is created as a local variable insidestart_tls()). Whengen.with_timeoutfires, theTimeoutErroris raised but the wrapped future is not cancelled (by design ofgen.with_timeout), leaving theSSLIOStreamregistered on the IOLoop with no external reference — permanently leaking the file descriptor.Fix
tcpclient.py: Save the future returned bystart_tls()and calltls_future.cancel()onTimeoutErrorbefore re-raising.iostream.py: Register a done callback on the TLS connect future insidestart_tls()that closes theSSLIOStreamwhen the future is cancelled, ensuring the socket fd is released and the handler is removed from the IOLoop.Fixes #3614