Release the in-flight SSLIOStream when its parent IOStream closes#3672
Open
HrachShah wants to merge 1 commit into
Open
Release the in-flight SSLIOStream when its parent IOStream closes#3672HrachShah wants to merge 1 commit into
HrachShah wants to merge 1 commit into
Conversation
IOStream.start_tls transfers ownership of the underlying socket to a new SSLIOStream before the TLS handshake completes, and returns a Future that resolves with that SSLIOStream. If a caller (typically TCPClient.connect with a timeout) gives up while the handshake is in flight, the original IOStream is the only reference still reachable, but its .socket is already None and close() on it is a no-op; the SSLIOStream (and the socket it owns) becomes unreachable and leaks permanently. A gen.with_timeout caller is hit by this because with_timeout explicitly does not cancel the wrapped future. Expose the in-flight SSLIOStream on the original IOStream via _pending_ssl_stream, and close it from IOStream.close so its socket is released. When the start_tls future eventually resolves, the done callback clears the back-reference without closing the now-owned SSLIOStream. Clear _state in start_tls and tolerate a None socket in close_fd so a post-start_tls close() does not raise AttributeError on the detached socket. TCPClient.connect wraps the start_tls future in a try/except so that on TimeoutError the original stream is closed (and the SSLIOStream released via the new close path) before the exception is re-raised. Regression test in tornado/test/tcpclient_test.py stands up a listener that accepts but never speaks TLS, connects with ssl_options and a short timeout, and asserts that no ESTABLISHED TCP entry remains on the local port after the deadline fires.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3614.
IOStream.start_tlstransfers ownership of the underlying socket to a newSSLIOStreambefore the TLS handshake completes and returns aFuturethat resolves with thatSSLIOStream. If a caller (typicallyTCPClient.connectwith atimeout) gives up while the handshake is in flight, the originalIOStreamis the only reference still reachable, but its.socketis alreadyNoneandclose()on it is a no-op; theSSLIOStream(and the socket it owns) becomes unreachable and the file descriptor leaks permanently.gen.with_timeoutcallers are hit by this becausewith_timeoutexplicitly does not cancel the wrapped future.This change:
SSLIOStreamon the originalIOStreamvia_pending_ssl_streamand closes it fromIOStream.closeso its socket is released.start_tlsfuture that just clears the back-reference (without closing) when the future eventually resolves with the now-ownedSSLIOStream._stateinstart_tlsand tolerates aNonesocket inIOStream.close_fdso a post-start_tlsclose()does not raiseAttributeErroron the detached socket.TCPClient.connectwraps thestart_tlsfuture in atry/exceptso that onTimeoutErrorthe original stream is closed (and theSSLIOStreamreleased via the new close path) before the exception is re-raised.Regression test (
tornado/test/tcpclient_test.py):TCPClientSSLTimeoutTest.test_ssl_handshake_timeout_releases_socketstands up a listener that accepts but never speaks TLS, connects withssl_optionsand a shorttimeout, and asserts that noESTABLISHEDTCP entry remains on the local port after the deadline fires. Without the fix, anESTABLISHEDentry is left behind and the test fails.