fix(jetsocat): flush JMUX messages without waiting on a timer - #1939
fix(jetsocat): flush JMUX messages without waiting on a timer#1939Richard Markiewicz (thenextman) wants to merge 1 commit into
Conversation
Let maintainers know that an action is required on their side
|
There was a problem hiding this comment.
Pull request overview
Improves JMUX relay latency for jetsocat traffic.
Changes:
- Flushes drained JMUX queues promptly with bounded coalescing.
- Disables Nagle’s algorithm on jetsocat relay sockets.
- Adds HTTP flow-control regression tests.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
crates/jmux-proxy/src/lib.rs |
Revises JMUX flushing and target socket handling. |
jetsocat/src/utils.rs |
Adds TCP connections with TCP_NODELAY. |
jetsocat/src/listener.rs |
Disables Nagle on accepted relay sockets. |
testsuite/tests/jmux_flow_control.rs |
Adds HTTP/1.1 and HTTP/2 latency tests. |
testsuite/tests/main.rs |
Registers the new tests. |
testsuite/Cargo.toml |
Adds test dependencies. |
Cargo.lock |
Locks the added dependencies. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Disable Nagle's algorithm: this is a relay, so the write pattern is | ||
| // dictated by the peer, and holding back a sub-MSS segment until the | ||
| // target ACKs only adds latency. Coalescing already happens upstream, | ||
| // where JMUX messages are batched before hitting the pipe. | ||
|
|
There was a problem hiding this comment.
The comment was actually orphaned, TCP_NODELAY was removed because it costs about 25% throughput and 40% CPU per GB on this socket. Current tests are only on loopback so on a real world LAN, it likely matters more. I don't know the benefit but it has a measured cost. Comment is replaced with one explaining the situation.
| assert!( | ||
| through_jmux < H2_BUDGET, | ||
| "HTTP/2 upload through JMUX took {through_jmux:?}, over the {H2_BUDGET:?} budget \ | ||
| (direct took {direct:?}); JMUX is likely delaying flow control updates" |
There was a problem hiding this comment.
Fixed as per the suggestion.
3d40d3e to
8d7a7c8
Compare
8d7a7c8 to
cbb6a3d
Compare
cbb6a3d to
ba9866c
Compare
Background
We have concrete bug reports of users experiencing a 50x slowdown when uploading files to VMWare over Gateway versus direct connection. The actual cause is not clear but we know that it affects both OVF and generic file uploads. We know that VMWare's frontend is HTTP/2 and we suspect that they use the default initial flow control window of 64KB for streams.
With no-way to reproduce locally, we look for issues in the code base and perform local testing; streaming a file over both HTTP/1 and HTTP/2 through a JMUX tunnel.
The testing and changes are AI written but human guided and tuned.
Results
Traffic that depends on round trips rather than raw bandwidth could run orders of magnitude slower through a Gateway tunnel than the same traffic made directly.
The JMUX sender batches outgoing messages behind a write buffer, flushing once the message stream goes quiet. That batching was introduced in #976 to cut the number of syscalls, and it worked: bulk throughput improved by about 28%. The blind spot was that the flush timer restarted on every message, so it only ever elapsed once traffic
stopped entirely. Bulk transfers never notice, because they keep the queue busy and fill the write buffer on their own. But traffic whose progress depends on a small message coming back waited out the full delay on every single round trip, at both ends of the pipe and in both directions.
HTTP/2 uploads are where this hits. HTTP/2 limits how much of a request body may be in flight before the server grants more credit, and that limit is commonly 64 KiB, so a large upload becomes a long sequence of round trips rather than one continuous stream. Every one of those round trips absorbed the delay. That is why HTTP/1.1 transfers to the very same host stay fast while HTTP/2 uploads crawl.
Messages are now flushed as soon as the send queue runs dry, with a short minimum spacing between flushes so that a sustained transfer still fills the write buffer instead of writing out partial ones. The syscall batching that #976 introduced is kept intact: bulk throughput and CPU cost per gigabyte are unchanged.
Measured on loopback, so no network latency is involved. An 8 MiB HTTP/2 upload against a 64 KiB window, and a 500 MB bulk transfer:
Round trip cost through the tunnel drops from roughly 25 ms to under 1 ms, with no throughput or CPU cost.
Implementation notes:
JmuxSenderTaskis now measured from the first unflushed byte and never reset by later messages, so a steady stream cannot postpone it. Previously it was reset on every message, which made it an idle timer rather than a coalescing one. Note that a recent commit pinned thisSleepinstead of recreating it per iteration: that removed the repeated allocation but kept the per-message reset, so the latency behaviour was unchanged.JMUX_FLUSH_MIN_SPACING(50 µs), which bounds how often a drain can trigger a flush.last_flushis anOption<tokio::time::Instant>,Noneuntil the first flush, so the first message a sender emits is never subject to the spacing check. Initializing it toInstant::now()instead makes the first message look recently-flushed, which defers it to the coalescing backstop; a real 1 ms penalty for any lone message early in a channel's life. The paused-clock test covers this specific case.TCP_NODELAYis set on jetsocat's accepted listener sockets and on every connect site inimpl_tcp_connect!via a newconnect_nodelayhelper. The helper matters for the proxied variants, where the flag has to be set on the innerTcpStreambefore the SOCKS/HTTP proxy stream wraps it.DataWriterTaskwrites each ~4 KiB chunk straight to the socket with no buffering, so disabling Nagle turns every chunk into its own segment. Giving that writer aBufWriterwould make nodelay cheap there, and nodelay is the change most likely to help on a real WAN with delayed ACKs; worth a separate issue, since loopback cannot measure the benefit either way.testsuite/tests/jmux_flow_control.rsruns a real hyper HTTP/2 server withinitial_stream_window_size(64 KiB)and a JMUX pair wired over loopback. The HTTP/1.1 counterpart is kept deliberately: it passes both before and after, so a failure in the HTTP/2 test alone identifies a round-trip regression rather than a bandwidth one. The 2 s budget sits ~25x above the current result and well below the old one.