anytls: send stream SYNACK before dialing the upstream target#144
Open
zytakeshi wants to merge 1 commit into
Open
anytls: send stream SYNACK before dialing the upstream target#144zytakeshi wants to merge 1 commit into
zytakeshi wants to merge 1 commit into
Conversation
The success SYNACK was sent only after connect_tcp completed. sing-box's AnyTLS client pools idle sessions and, when it opens a stream on a reused session, arms a 3s watcher that closes the entire session if the SYNACK is late. On a high-RTT link the connect latency could exceed that budget, so reused sessions were repeatedly killed and re-dialed, collapsing upload throughput (downloads were immune -- the first stream arms no watcher). Send the SYNACK at stream-open so the watcher window is ~1 RTT regardless of connect latency; on connect failure report it with a follow-up error SYNACK (the client maps it to closeWithError) and the per-stream FIN still fires. send_synack is a no-op for protocol v1 peers.
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
1 participant
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.
Summary
handle_tcp_forwardsent the success SYNACK only afterchain_group.connect_tcp(...)returned. This change moves the success SYNACK to before the upstream dial, keeping the connect-failure error SYNACK (now sent as a follow-up).Why
sing-box's AnyTLS client pools idle sessions and reuses them for later streams. When it opens a stream on a reused session (sid >= 2) it arms a 3-second
synDonewatcher that closes the entire session if the SYNACK is late (see sing-anytlssession.OpenStream). A post-connect SYNACK on a high-RTT link, plus variable connect latency, can exceed that 3s budget, so every reused session gets killed and re-dialed -> session-pool churn -> upload throughput collapses. Downloads are immune because the first stream (sid == 1) arms no watcher.Acking at stream-open bounds the watcher window to roughly one RTT regardless of connect latency.
Trade-off (please weigh in)
The reference AnyTLS server (anytls-go / sing-anytls) sends its handshake report after the upstream dial; this change acks at stream-open instead.
synDonewatcher to ~1 RTT, preventing session-pool churn and upload collapse on high-RTT links.closeWithError, and the per-stream FIN still closes only that sub-stream — the session is unaffected. Clients that ignore a second SYNACK simply see the per-stream FIN, which is benign.send_synackis already a v2-gated no-op for v1 peers, so v1 clients are unaffected.An alternative, if you prefer to stay closer to the reference behaviour, is to drop the success ack at stream-open entirely (and rely only on the per-stream FIN). I'm happy to adjust to whichever you'd like.
Testing
cargo checkpasses (only pre-existing warnings unrelated to this change).