Repository navigation
Client half-close: keep the payload of a FIN segment and keep the write side open - #112
Open
frontierlabs-admin wants to merge 2 commits into
Open
frontierlabs-admin wants to merge 2 commits into
frontierlabs-admin wants to merge 2 commits into
Conversation
A peer that half-closes with data still queued, as Linux does on shutdown(SHUT_WR), puts its FIN on the last data segment. In Established such a segment was handled by its flags alone: - ACK|FIN in sequence took the FIN without its payload, so the last bytes of the stream never reached the reader and the ACK number fell short of them. - ACK|PSH|FIN matched no arm and was dropped without an ACK, every retransmission of it too. - A FIN segment arriving ahead of a gap was dropped without an ACK. RFC 9293 §3.10.7.4 processes the segment text before the FIN bit. The payload is now received like that of any other data segment, PSH or not, and the FIN is taken only once every byte before it has arrived. A FIN that is not yet in sequence leaves its payload queued and is taken when the peer retransmits it. Refs narrowlink#111
A FIN closes only the peer's direction (RFC 9293 §3.6). On a FIN in Established with nothing in flight, the stack answered at once with its own FIN and moved to LastAck, and in CloseWait any ACK with nothing in flight did the same. A client that sends a request and then calls shutdown(SHUT_WR) therefore had the reply cut off as soon as the data already written was acknowledged. A reader in CloseWait also got Pending rather than end of stream, and waited for a timer. After the peer's FIN the stack now: - acknowledges it, wakes the reader, which gets every byte sent before the FIN and then end of stream, and keeps the write side open; - in CloseWait, acknowledges a retransmitted FIN, or segment carrying it, and answers duplicate ACKs with a fast retransmission, as in Established; - sends its own FIN when the application shuts down, once everything written is acknowledged, or when it drops the stream. The FIN sent on shutdown is retransmitted by the LastAck timer, which now lives at module level so poll_shutdown can start it. A write after it fails with BrokenPipe. close_wait_timeout remains the backstop for an application that never shuts down, and it is now armed on every FIN rather than only when data was in flight. It counts time without progress, so a reply that is still being written and acknowledged is not cut after a fixed delay. A writer still waiting for window when the connection ends is woken. Refs narrowlink#111, related to narrowlink#89.
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.
Fixes #111 (client half-close). Two commits, both only in
src/stream/tcp.rs; related to #89 (server closes first), which this does not try to fix.1. Keep the payload of a segment that carries FIN
Linux puts the FIN on the last data segment on
shutdown(SHUT_WR). On currentmain:ACK|FINwith data loses the data and sends a short ACK number;ACK|PSH|FINmatches no arm and is dropped without an ACK, every retransmission too;The payload is now received like any data segment and the FIN is taken only once every earlier byte has arrived (RFC 9293 §3.10.7.4). Most of this diff is re-indentation (
git diff -wis +10/-1 code lines).(Out-of-order
ACK|PSHdata and the Closed-state reader EOF are already fixed onmainby #97/#95, so they are not touched.)2. Keep the write side open after the peer's FIN
A peer FIN closed the whole connection: the stack sent its own FIN at once, cutting off a reply still being written, and a
CloseWaitreader gotPendinginstead of EOF. Now the FIN is acknowledged, the reader gets all bytes then EOF, the write side stays open, and our FIN is sent frompoll_shutdownor on drop. A write after shutdown fails withBrokenPipe.task_last_ackmoves to module level sopoll_shutdowncan start it.Behaviour change to review:
TcpConfig::close_wait_timeoutis still the backstop, but now means "this long with nothing sent or acknowledged" rather than a fixed deadline from the peer's FIN. Without that, a reply taking longer than the timeout after a half-close is still cut. The doc comment is updated.Tests
All in-memory, no TUN needed. Unit tests 36 -> 52 passed, 0 failed;
cargo fmt --checkclean;cargo clippy --all-targets --all-features -D warnings0 warnings; 10 repeat runs green. The 3 commit-1 tests fail onmain; the 13 commit-2 tests fail on commit-1 code. Each hunk was reverted on its own and the tests went red (one exception below).Known limits
task_last_ackresends our FIN atget_seq(), one past the FIN's own sequence number (pre-existing), so a peer that lost the first FIN rejects the resends. The half-close path now reaches it more often. The fix isget_seq() - 1; left out to keep this PR scoped. Happy to add it here or as a follow-up.poll_readcloses a lost-wakeup race; a single-threaded test cannot interleave there, so it is not test-proven. Easy to drop if you prefer.CloseWaitsends our FIN once with no retransmission.