Repository navigation
fix(server): disable Nagle on accepted connections - #1799
Marc-André Moreau (mamoreau-devolutions) merged 1 commit into
Conversation
RDP output is a stream of small writes the peer is waiting on: a frame, a pointer update, a channel PDU. Nagle holds the trailing partial segment of each until the previous one is acknowledged, so against a peer using delayed acknowledgements every such write waits on a timer that has nothing to do with the encoder or the link. `run` sets `SO_REUSEADDR` on the listening socket and nothing on the accepted ones. A failure is logged rather than propagated: the session works either way, only less promptly, and refusing a connection over a socket option would cost more than the latency it avoids. `run_connection` and `run_connection_with` take a stream from the caller and cannot set this themselves, so their documentation now says whose job it is. For reference, `ironrdp-client` sets it on its RDCleanPath transport and not on a direct connection.
GitHub Actions
Inactive
github-actions
Bot
added
needs-review
|
The write path ( Because writes are already this unbatched (a busy frame fragments into 600+ separate Philosophically: Nagle and delayed ACK are throughput-era defaults (1984 and 1989, scarce links, bulk transfer). RDP optimizes for perceived immediacy, not bytes per packet, and every interactive protocol re-litigates this fight for the same reason. IronRDP already made this call once, at the UDP layer: RDPEUDP2 cut its own delayed-ACK timer from 300ms to 50ms for exactly this. This PR is the TCP-side sibling of a decision the protocol's own designers already made. Scope is honest: Server's outgoing direction only, correctly leaves caller-owned streams alone. No downside found. Approve, no reservations. |
915e681
into
Devolutions:master
…volutions#1799, Devolutions#1812) Two upstream commits since the last sync: Devolutions#1812 fixed the exact result_large_err lint I'd already fixed on this branch last turn -- identical attribute, identical reason text. Merged with NO conflict: git recognized both sides made the same change to the same region relative to the common ancestor. Devolutions#1799 disables TCP_NODELAY on accepted connections, landing where this branch's preemption race already restructured the plain accept loop (the same conflict class as every prior sync -- resolved by keeping this branch's superset). But this one wasn't safe to resolve by taking HEAD's side and discarding upstream's alone, the way the purely structural instances of this conflict have been: Devolutions#1799 carries a real behavior change (RDP is small latency-sensitive writes; Nagle holds the trailing partial segment of each until the peer's ack, which against delayed-ACK peers is dead time on every write), not just a different shape of the same logic. Upstream's fix has exactly one accept path to patch. This branch has two: the primary listener.accept() (no live session), and the second one inside the preemption race (PreemptRace::Accepted, a candidate connecting WHILE a session is live). A candidate that wins the race becomes the live session exactly like a primary accept does, so it needs TCP_NODELAY too -- applying upstream's fix only where their single code path put it would have silently left every preemption takeover without it. Added the identical set_nodelay(true) + warn-on-failure logic at both accept sites, matching upstream's wording where the comment is describing the same fact (Nagle/delayed-ACK) and adding a line at the second site noting why it needs the same fix. 14/14 tests pass in both default and --features egfx, clippy --all-targets clean in both configs AND under CI's exact stricter command (--features helper,__bench --locked -- -D warnings, the one that caught the result_large_err regression two syncs ago), fmt clean, full cargo build --workspace green.
RDP output is a stream of small writes the peer is waiting on: a frame, a pointer update, a channel PDU. Nagle holds the trailing partial segment of each until the previous one is acknowledged, so against a peer using delayed acknowledgements every such write waits on a timer that has nothing to do with the encoder or the link.
runsetsSO_REUSEADDRon the listening socket and nothing on the accepted ones.A failure is logged rather than propagated: the session works either way, only less promptly, and refusing a connection over a socket option would cost more than the latency it avoids.
run_connectionandrun_connection_withtake a stream from the caller and cannot set this themselves, so their documentation now says whose job it is. For reference,ironrdp-clientsets it on its RDCleanPath transport and not on a direct connection.