gh-157174: Close the socket when HTTPConnection.connect() fails to set TCP_NODELAY - #157175
Open
iamsharduld wants to merge 1 commit into
Open
gh-157174: Close the socket when HTTPConnection.connect() fails to set TCP_NODELAY#157175iamsharduld wants to merge 1 commit into
iamsharduld wants to merge 1 commit into
Conversation
… to set TCP_NODELAY connect() left self.sock holding the open socket when setsockopt() raised anything but ENOPROTOOPT, so a caller that dropped the connection leaked it. On macOS, setsockopt(TCP_NODELAY) raises EINVAL once the peer has reset the connection, which is what test_ssl.test_https_client_non_tls_response_ignored provokes on purpose; on a slow machine the reset wins the race and the test leaves an unclosed socket behind. Close the connection before re-raising, as _tunnel() already does.
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.
HTTPConnection.connect()assigns the new socket toself.sockand then setsTCP_NODELAYon it. When thatsetsockopt()fails with anything other thanENOPROTOOPT, the error propagates whileself.sockstill holds the open socket, and a caller that drops the connection leaks it._tunnel()handles its own connect-time failure by callingself.close()before raising; this does the same forTCP_NODELAY.It is what is behind the
ResourceWarningfromtest_ssl.test_https_client_non_tls_response_ignoredon macOS CI (details in the issue): on macOS,setsockopt(TCP_NODELAY)raisesEINVALonce the peer has reset the connection, that test's server resets on purpose, and on a slow runner the reset wins the race.Verified on macOS 26:
_create_connectionpauses 2 ms before returning (standing in for the client thread being preempted): 150 of 150 attempts raiseEINVALfromsetsockopt(); without this change all 150 sockets are reported unclosed, with it none.test_connect_tcp_nodelay_error_closes_socketfails before the change (conn.sockis not None) and passes after;test_connect_tcp_nodelay_unsupportedchecks thatENOPROTOOPTstill leaves the connection usable.test_httplib,test_urllib2,test_urllib2_localnetand thetest_ssltest in question pass.