Skip to content

gh-157174: Close the socket when HTTPConnection.connect() fails to set TCP_NODELAY - #157175

Open
iamsharduld wants to merge 1 commit into
python:mainfrom
iamsharduld:fix-httpclient-nodelay-leak
Open

gh-157174: Close the socket when HTTPConnection.connect() fails to set TCP_NODELAY#157175
iamsharduld wants to merge 1 commit into
python:mainfrom
iamsharduld:fix-httpclient-nodelay-leak

Conversation

@iamsharduld

@iamsharduld iamsharduld commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

HTTPConnection.connect() assigns the new socket to self.sock and then sets TCP_NODELAY on it. When that setsockopt() fails with anything other than ENOPROTOOPT, the error propagates while self.sock still holds the open socket, and a caller that drops the connection leaks it. _tunnel() handles its own connect-time failure by calling self.close() before raising; this does the same for TCP_NODELAY.

It is what is behind the ResourceWarning from test_ssl.test_https_client_non_tls_response_ignored on macOS CI (details in the issue): on macOS, setsockopt(TCP_NODELAY) raises EINVAL once 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:

  • A harness with a server that accepts and resets at once, and a client whose _create_connection pauses 2 ms before returning (standing in for the client thread being preempted): 150 of 150 attempts raise EINVAL from setsockopt(); without this change all 150 sockets are reported unclosed, with it none.
  • The new test_connect_tcp_nodelay_error_closes_socket fails before the change (conn.sock is not None) and passes after; test_connect_tcp_nodelay_unsupported checks that ENOPROTOOPT still leaves the connection usable.
  • test_httplib, test_urllib2, test_urllib2_localnet and the test_ssl test in question pass.

… 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant