Fix: AsyncTunnelHTTPConnection leaks connection on handshake failure#1056
Fix: AsyncTunnelHTTPConnection leaks connection on handshake failure#1056drshvik wants to merge 4 commits into
Conversation
|
This relates to #1049 but provides a broader fix by wrapping the entire handshake process, ensuring cleanup even if the failure occurs before the TLS upgrade step. |
|
@simonw Please review this PR and let me know your opinion. |
|
Thanks for looking into this. I believe the broader fix overlaps with existing exception handling:
The only gap is TLS handshake After CONNECT 200 returns, start_tls() (http_proxy.py#L317) sits outside the exception handler above. When TLS fails, the underlying socket is closed but _state remains ACTIVE → zombie connection. My PR #1049 adds minimal try-except for this specific gap. Wrapping the entire handshake duplicates HTTP/1.1 layer's existing cleanup logic. Detailed analysis: https://www.reddit.com/r/Python/comments/1qs7crk/bug_fix_connection_pool_exhaustion_in_httpcore/ Is there a specific edge case I'm missing that isn't covered by the existing handle_async_request try-except? |
|
Downstream field evidence from a long-lived Telegram gateway behind an explicit HTTP CONNECT proxy (Tinyproxy) on a restricted network. Environment:
We had a repeatable production symptom where proxy-facing I then added a focused downstream regression at the After applying only this PR's async I restarted the real gateway so it imported the patched module and sampled the same PID every 15 seconds for nine minutes (37 samples), spanning several polling/heartbeat cycles: No Telegram network/pool/protocol error appeared during that interval. A later check on the same PID also still had zero Caveat: the live run also followed a downstream Hermes update, so it is not a perfectly controlled same-version field A/B. The regression itself is a direct RED -> GREEN check of this PR's cleanup behavior; the live result is supporting evidence, not proof that this PR is preferable to the narrower TLS-only approach discussed in #1049. It does, however, match the downstream failure class and is the first mitigation we tested that passed the previously fast live reproduction window. |
Summary
Fixes #1023.
When using an HTTP proxy with tunneling (HTTPS requests), if the
CONNECThandshake fails (e.g.RemoteProtocolError, timeouts, or TLS errors during the tunnel setup), the underlying network stream was not explicitly closed.This caused the connection to remain in the pool as "active" but dead. If
max_connectionsis set, this eventually leads to pool starvation where no new requests can be made.The Fix
AsyncTunnelHTTPConnection.handle_async_requestwith atry...exceptblock.await self._connection.aclose()if any exception occurs during the handshake or TLS upgrade steps.Verification
I verified this with a local reproduction script that simulates a proxy server accepting a connection and then immediately closing it (crashing the handshake).
Before the fix:
max_connections=1).After the fix: