Skip to content

Propagate timeout through SOCKS5 handshake#1009

Open
mbeijen wants to merge 1 commit into
pydantic:mainfrom
mbeijen:fix/socks5-handshake-timeout
Open

Propagate timeout through SOCKS5 handshake#1009
mbeijen wants to merge 1 commit into
pydantic:mainfrom
mbeijen:fix/socks5-handshake-timeout

Conversation

@mbeijen
Copy link
Copy Markdown
Contributor

@mbeijen mbeijen commented Jun 2, 2026

Summary

During SOCKS5 connection setup, all reads and writes to the proxy socket were called without a timeout. A non-responsive SOCKS5 proxy (one that accepts the TCP connection but never sends a handshake reply) would block the calling thread/task indefinitely, regardless of any timeout configured on the request.

This PR threads the connect timeout (already used for the initial TCP connect) through _init_socks5_connection() to every stream.read() / stream.write() call during auth method negotiation, optional username/password auth, and the SOCKS5 CONNECT request.

Ports encode/httpcore#1055 (drshvik), via codeberg.org/httpxyz/httpcorexyz@0387930.

Notes

  • Only _async/socks_proxy.py was edited by hand; _sync/socks_proxy.py was regenerated by scripts/unasync.py.
  • Neither the upstream PR nor the fork commit added a dedicated unit test (the upstream PR ships reproduction scripts instead — a real-hang test needs an integration-style fixture). The existing MockBackend-based SOCKS5 tests already exercise the new code path with timeout=None. 100% coverage is preserved.

Note: this change was prepared with AI assistance (Claude Code).

During SOCKS5 connection setup, all reads and writes to the proxy
socket were called without a timeout. A non-responsive proxy would
block the calling thread/task indefinitely regardless of any
`timeout` configured on the request.

Thread the `connect` timeout (already used for the initial TCP
connect) through `_init_socks5_connection()` to every `stream.read()`
and `stream.write()` call.

Ported from encode/httpcore#1055 (drshvik), via
codeberg.org/httpxyz/httpcorexyz@0387930.

Co-Authored-By: drshvik <ai23btech11004@iith.ac.in>
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Jun 2, 2026

Merging this PR will not alter performance

✅ 15 untouched benchmarks
⏩ 7 skipped benchmarks1


Comparing mbeijen:fix/socks5-handshake-timeout (f5a995d) with main (04f3804)

Open in CodSpeed

Footnotes

  1. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Re-trigger cubic

mbeijen added a commit to mbeijen/httpx2 that referenced this pull request Jun 2, 2026
Copy link
Copy Markdown
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem correct, is it? We are using the connect timeout for everything.

I think before addressing this we need to make httpcore2 a bit more type safe in the Extensions type - maybe starting by splitting it in RequestExtensions and ResponseExtensions.

@mbeijen
Copy link
Copy Markdown
Contributor Author

mbeijen commented Jun 2, 2026

I think before addressing this we need to make httpcore2 a bit more type safe in the Extensions type - maybe starting by splitting it in RequestExtensions and ResponseExtensions.

That's reasonable! Do you pick that up or would you like me to create a PR?

@Kludex
Copy link
Copy Markdown
Member

Kludex commented Jun 2, 2026

You can do it. You'll probably need extra_items and total=False, so we probably need to include typing extensions.

Change httpcore2 only if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants