Scale copy-buffer memory with concurrent transfers, not connection count (#37) - #41
Scale copy-buffer memory with concurrent transfers, not connection count (#37)#41kriszyp wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a custom copy_bidirectional_lazy implementation to replace tokio::io::copy_bidirectional_with_sizes, optimizing memory usage for idle connections by dynamically escalating and de-escalating buffer sizes. It also adds memory and throughput benchmarks to validate these changes. The review feedback suggests refining the buffer-shrinking strategy to prevent heap thrashing during active transfers with variable packet sizes by deferring the shrink operation until the connection actually parks (goes idle) rather than shrinking immediately on the first under-capacity read.
95029e4 to
5854c85
Compare
Devin-Holland
left a comment
There was a problem hiding this comment.
Verified locally
Toolchain available here (cargo 1.94.1), so I re-ran the parts the write-up says needed a real one, on 5ede9fb:
cargo test— 115 passed, 0 failed (copy::alone: 10 passed).cargo clippy --all-targets— 0 errors, and 0 warnings insrc/copy.rs; the 9 warnings are pre-existing dead-code lints in unrelated modules. CI green on all 11 checks.
Because this replaces the loop that carries every proxied byte, I diffed the port against tokio-1.53.1 rather than reading it standalone. copy_bidirectional_impl, transfer_one_direction, poll_fill_buf, and poll_write_buf are faithful line-for-line, including the TransferState::Running → ShuttingDown → Done half-close ladder, the ?-on-each try-join semantics, the pos == cap pending-with-data fall-through, and the WriteZero guard. The only behavioral deltas are the dropped byte counts (which copy_both_ways discarded anyway) and the resize decision.
Two things I specifically went after and can report clean:
- Buffer replacement can't drop or corrupt bytes. This was my main worry —
self.buf = vec![...]whilepos/capdescribe live data would either panic onset_filled(cap)(which assertsn <= initialized) or write the wrong slice. It's safe, and by construction rather than luck: the write loop only exits withpos == cap, the loop bottom resets both to0before any top-of-loop read, and the shrink branch is itself guarded onpos == cap— sopos == cap == 0whenever a resize happens, and bothset_filled(cap)and&buf[pos..cap]are additionally guarded bycap < buf.len(). I traced every path into the resize looking for a counterexample and didn't find one. - The coop early-return can't strand a wakeup.
consume_budget()→poll_proceed(cx), which callsregister_waker(cx)before returningPending(tokio/src/task/coop/mod.rs:360). Given the noop-Wakerdesign you discarded failed exactly this way, worth having confirmed rather than assumed.
I also chased two things that turned out not to be problems, so they don't need answering: stale pos/cap surviving a shrink (unreachable, per above), and full_streak overflowing u32 on a long-lived saturating stream — reachable only via 4.29e9 consecutive full reads with no park and no short read in between, and both of those reset the streak, so it isn't a real path. I wrote a probe for the overflow and it passed without panicking, which is what tipped me off.
What I'd want before these numbers become a sizing input
Neither benchmark exercises the shape this change is for. The mechanism is escalate → park → shrink → re-escalate, and each cycle costs two allocations plus a zeroing vec![0u8; n]. But bench-copy-memory sends one burst per connection and then goes quiet, and bench-copy-throughput is 4 sustained streams that escalate once and stay there — so the churn sits off the hot path in both. The workload in between, a connection that bursts repeatedly with idle gaps, is MQTT fan-out, which is the case in the acceptance criteria. A third benchmark shaped like that (N connections × repeated burst/idle cycles, reporting throughput and RSS) is what would show whether the re-escalation cost is as negligible as the design assumes. I'd rather see that measured than reasoned about, given it's the one path the design adds.
The memory delta is ~2.4× smaller than the model predicts, and the write-up doesn't say why. At readBufferSize = 64 KiB the released buffer should be 2 × (65536 − 512) = 130,048 B/conn, but the table shows 305,985 → 251,429, i.e. 54,556 B/conn — 42% of that. That gap is worth naming rather than leaving as "18% less": the most likely explanation is that 64 KiB sits under glibc's 128 KiB mmap threshold, so free returns it to the arena and not to the OS, and RSS therefore tracks the peak concurrent transfer count and doesn't come back down afterwards. If that's what's happening it doesn't undermine the change at all — "memory scales with concurrent transfers, not connections" is exactly the goal, and arena reuse is the mechanism that delivers it — but it means an operator reading the table as a per-connection saving will over-predict RSS relief on a node that has ever peaked. Confirming the cause (e.g. MALLOC_MMAP_THRESHOLD_=65536, or malloc_trim, or just glibc arena stats) would turn a soft 18% into a statement about what actually bounds resident memory.
Smaller
consume_budget()commits a budget unit unconditionally; tokio'spoll_copyusespoll_proceed(cx)+made_progress(), which restores the unit when a poll makes no progress. So a connection taking many no-progress wakeups burns budget faster here than in tokio and forces more yields. I have no evidence it matters — the throughput run doesn't contradict it either way — but it's the one place the port isn't faithful, in a PR whose safety argument rests on faithfulness, so it deserves a sentence in the module docs even if the answer is "deliberate, simpler, immaterial".- The description has drifted from the branch: it says "Draft — not requesting review yet" (
isDraftis false), "only the last 2 commits are this PR's content" (there are 6 now), and "8 unit tests" (10). Worth a refresh since the write-up is doing real explanatory work here.
Not approving only because this is stacked on kris/wire-read-buffer-size and #36 lands first — no objection to the code, which I went at hard and found clean.
Reviewed by Claude (Opus 5). All commands reproducible on 5ede9fb.
- Document the deliberate divergence between consume_budget()'s unconditional commit and tokio's progress-gated coop accounting, and why it can't strand a wakeup. - Add bench-copy-burst-idle.ts: repeated burst/idle cycles per connection (the MQTT fan-out shape in the acceptance criteria), reporting throughput and RSS across cycles — the one workload shape neither existing benchmark exercises, and the one the escalate/park/ shrink churn actually sits on the hot path for. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ujjj98sfVzL5hiZQxXJWWm
|
Correction to my review above: I wrote that I wasn't approving "only because this is stacked on Both of my #36 suggestions are on One consequence worth acting on: #36 was rebase-merged, so Rebasing first fixes it and is clean — I ran it: git rebase --onto origin/main origin/kris/wire-read-buffer-size fix/lazy-copy-buffers-37Six commits replay with no conflicts, the five duplicated #36 commits drop out by patch-id, and the result is green: The two substantive points from my review stand unchanged: the repeated-burst benchmark gap, and the unexplained 2.4× gap between the measured memory delta and what the model predicts. Claude (Opus 5) |
…unt (#37) Replaces copy_bidirectional_with_sizes with a hand-rolled copy_bidirectional_lazy (src/copy.rs). tokio::io::CopyBuffer allocates its per-direction buffer once and holds it for the connection's whole life, whether or not it is transferring — at a million mostly-idle MQTT subscribers, readBufferSize x 2 held forever per connection is dead weight. pump() instead starts each direction at a small fixed floor (512 B) and escalates to the full configured max only once a read proves a sustained burst (a read that exactly fills the current buffer), dropping back down the first time a read comes back under capacity. Every read is a single ordinary blocking .await; an earlier version tried a non-blocking opportunistic drain via a manual poll_read with a noop Waker to batch up "whatever's already queued" without an extra iteration, and under load it silently stranded a connection's wakeup (reproduced empirically). The final design has no manual polling at all, so every step is provably deadlock-free at the cost of one extra small-buffer round trip per burst. tokio::try_join! (not join!) on the two directions preserves copy_bidirectional's error semantics: an error on either side ends the whole copy immediately. readBufferSize/client|upstreamReadBufferSize become a per-transfer maximum rather than a permanent allocation; default sizes and the config surface are unchanged. Measured (see __test__/bench-copy-memory.ts, __test__/bench-copy-throughput.ts): - 15,000 connections, 64 KiB configured buffer, each sending one full-buffer burst then going idle: 305,985 B/conn (base) -> 233,240 B/conn (this branch), ~24% less resident memory per connection. - Bulk throughput (4 connections, sustained high volume): no regression, ~1700-1900 MiB/s on both, within run-to-run noise. Based on kris/wire-read-buffer-size (PR #36, not yet merged), which introduces readBufferSize and the per-direction client/upstreamReadBufferSize fields this work builds on. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ate shutdown errors Full rewrite of src/copy.rs on an independent codex review of the first version. That version used tokio::io::split + independent per-direction async fns; the review found three real problems it introduced: - tokio::io::split wraps each side in Arc<Mutex<_>> -- two heap allocations and a lock/unlock on every read/write/flush/shutdown, per connection, which itself scales with connection count (the exact thing this change exists to avoid). - write_all() doesn't flush, and the pump moved straight to the next read after it. tokio-rustls in particular defers sending encrypted records until poll_flush, so a TLS client that sends one request and waits for the reply could never see it -- both sides would sit idle until idle_timeout. - Ignoring shutdown() errors (matching the out-of-scope http_proxy.rs precedent) diverges from tokio::copy_bidirectional, which propagates a failed shutdown as an error. Swallowing it means try_join!-equivalent short-circuiting never fires, and the other direction can wait forever. copy_bidirectional_lazy is now a direct port of tokio's own copy_bidirectional_impl / CopyBuffer state machine (transfer_one_direction + TransferState, poll_fn over both directions, no split, flush-before-park, propagated shutdown), with the escalating buffer spliced into the point where a drained buffer is reset for the next read. Escalation now requires two consecutive full reads (not one) before jumping to the configured max: a single full read is a coincidence, not evidence of a burst, and jumping to the max on it can leave an oversized buffer resident for however long the connection is next idle. Also fixes a test-config bug the review caught (a max_buf_size below PROBE_BUFFER_SIZE made both escalate/de-escalate branches unreachable) and adds regression tests for the flush-before-park and shutdown-propagation fixes. Re-measured after the rewrite: 15,000 connections / 64 KiB configured buffer, each sending one full-buffer burst then idling: 305,985 B/conn (base) -> 251,429 B/conn (this branch), ~18% less resident memory per connection. Throughput unaffected (~1700-1900 MiB/s both, within run-to-run noise; removing the split/Mutex plausibly helps here, though not conclusively distinguishable from noise in these runs). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- src/copy.rs: shrink the escalated buffer back to the floor when a direction parks with nothing left to write, not only after a completed under-capacity read. Otherwise a burst whose length lands exactly on a buffer boundary parks holding the max-sized buffer indefinitely if the connection then goes idle — exactly the memory cost this design exists to avoid. - src/copy.rs: consume one unit of the task's cooperative-scheduling budget on entry to poll_copy, mirroring tokio's own CopyBuffer, so a direction whose read/write never return Pending can't monopolize its worker thread. - src/copy.rs: add a test that drives LazyCopyBuffer directly and asserts on buf.len()/full_streak across the escalate and shrink transitions, rather than inferring them from end-to-end timing. - src/proxy.rs, ts/types.ts, README.md: the buffer-size docs still described the old permanent-per-connection-allocation model this same PR replaces. Rewrite them around the actual escalating/shrinking behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 KiB/connection x 1,000,000 connections is ~0.95 GiB, not 1 MiB, and the floor itself still scales with connection count -- only the escalated portion above it tracks concurrently bursting transfers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses two unaddressed gemini-code-assist review threads on this PR (src/copy.rs:223, src/copy.rs:266): shrinking immediately on a single under-capacity read punishes a connection with continuously active but variably-sized traffic -- it never goes idle, but every undersized read still tears down and reallocates the buffer, only to grow it right back on the next burst. Move the shrink into the park path added in the previous commit; a lone dip below capacity now only resets full_streak, so the buffer stays at whatever size the traffic actually needs until the connection genuinely has nothing left to do. Adds a direct test proving a single undersized read mid-burst does not shrink the buffer (only parking does). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
README.md, CLAUDE.md, and src/proxy_conn.rs still described the shrink-on-any-under-capacity-read behavior removed in the previous commit, and proxy_conn.rs's DEFAULT_COPY_BUFFER_SIZE doc still framed it as a permanent per-connection cost from before this PR. Bring all three in line with the actual park-only shrink / escalating-maximum model. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Document the deliberate divergence between consume_budget()'s unconditional commit and tokio's progress-gated coop accounting, and why it can't strand a wakeup. - Add bench-copy-burst-idle.ts: repeated burst/idle cycles per connection (the MQTT fan-out shape in the acceptance criteria), reporting throughput and RSS across cycles — the one workload shape neither existing benchmark exercises, and the one the escalate/park/ shrink churn actually sits on the hot path for. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ujjj98sfVzL5hiZQxXJWWm
A socket erroring or closing mid-cycle left its per-cycle promise unresolved, wedging the whole burst/idle loop with no diagnostic. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ujjj98sfVzL5hiZQxXJWWm
0df05d1 to
a07f42d
Compare
|
@Devin-Holland — thanks for this review, it was worth the effort you put into it. The correction about #36 already having merged, and the I picked this up in parallel and then found another agent had already landed the repeated-burst benchmark and the coop-budget doc note, so I'm standing down rather than pushing a competing version. But I did run the measurements first, and the one open item — the gap between the measured memory delta and what the model predicts — now has data, so here it is rather than leaving it for someone to redo. The 2.4× gap: your hypothesis doesn't hold, and the other side of the ledger explains most of itYour suggested cause was glibc arena retention — a 65536-byte buffer sits under the 128 KiB dynamic mmap threshold, so It doesn't move:
About 1% on the arm that should have moved most. So arena retention isn't what's holding resident memory up on the new code. The likelier cause is on the old side, and it inflates the gap rather than shrinking the saving. A read below capacity only dirties the pages it actually lands on, so a static 64 KiB buffer is only ever as resident as the largest read that hit it. Re-measuring with a burst several times the buffer size (4×, so the buffer stays saturated and both directions escalate) closes most of the gap:
Worth carrying into whichever benchmark survives: make the burst a multiple of Throughput: below what this instrument can resolveMy first three alternating pairs read as a clean ~4.9% deficit for #41. That turned out to be an artifact of when they ran. Six properly paired runs (arms back to back within each pair, load sampled around each):
Mean 0.966, median 0.978, range 0.865–1.026, 95% CI spanning 1.0, three of six favouring #41. No resolvable difference — and, equally, no basis for claiming the churn is free. Anything under about ±7% is invisible on a box shared with other agents. A quiet box, or a Rust-level microbenchmark of the copy loop over in-memory duplex streams, is what it would take to price the per-cycle allocation properly. All runs: Linux, 20 cores, 1000 connections × 10 cycles, Where this leaves the PRKris is holding #41 rather than merging it now. The memory win is real but reaches ~73% of its own model, and the throughput question is open rather than settled — a per-burst cost paid regardless of how many connections are open means low-concurrency deployments could pay it for no benefit. The likely direction when it resumes is making the behaviour configurable, or gating it on active connection count so it only engages where the memory actually matters. For the near-term need, #36's Claude (Opus 5), with Kris |
The escalate/park/shrink behaviour saves memory in proportion to connection count, but costs two allocations plus a zeroing `vec![0u8; n]` per burst regardless of it. A replication port-set carrying six bulk streams paid that on every burst to reclaim a few hundred KiB it was never short of. `copy::LazyBufferGate` decides at each resize point from the proxy-wide `GlobalMetrics::active_connections` gauge, configured per proxy by `lazyCopyBufferThreshold` (default 1000). Below it, each direction allocates its full configured buffer once and never resizes — byte-for-byte the `copy_bidirectional_with_sizes` behaviour this module replaced, so a small port-set pays none of the churn rather than a reduced amount. `0` engages always; a value above peak concurrency disables it. The gauge is re-read at every resize point rather than latched per connection: a connection established while the proxy was quiet would otherwise hold a full-size buffer for its whole life however busy the proxy later became, and long-lived connections accumulating while idle is the shape this exists for. Also adds the reload regression test that was asked for on #36 and never landed, now covering all four construction-frozen proxy fields, with a route-only control so it can't pass against a server that recreates on every write. Writing it turned up a wrong claim in the README: a recreate does NOT drop established connections. `stop()` ends the accept loops and sleeps 100ms but never aborts connection tasks, and the runtime lives in the napi wrap until GC, so established sessions keep running on the old buffer sizes until they close. That is now documented and pinned by a test. Benchmarks take the threshold as an argument and pin it to 0 by default, so a run below the default threshold can't silently measure the static path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Picked this back up. Two things landed, and one of them undercuts a claim I made in my previous comment, so that first. Correction: the memory saving does not reproduceMy earlier comment reported a 95,027 B/conn saving for this branch over That measurement compared two different builds. Now that escalation is gated behind a threshold, both behaviours are reachable from a single binary by changing one config value — no cross-build confound, no rebuild between arms, nothing differing but the code path under test. Running that at 8000 connections with a 64 KiB
No saving. The escalating path is consistently ~5 KB/conn worse, across three pairs, against a model that predicts ~128 KB/conn better. An effect that large going entirely missing isn't noise. This also means I was too confident in dismissing @Devin-Holland's original explanation. I reported the glibc arena hypothesis as disconfirmed because Worth separating two claims that got conflated (mine included): arena-retained memory is genuinely reusable by the process, so "the allocator can use it for something else" may well be true. But "resident memory goes down" is what an operator sizes a node against, and that is the claim this benchmark cannot support. What landed: gating (
|
|
Followed the memory question down to the allocator. Two corrections to my own earlier comments, and a result that I think decides what this PR is for. The mechanism works — that part is settledI previously inferred, from (The inference was bad for two reasons worth recording: And releasing the buffers still doesn't reduce RSSBoth are true at once, which is the actual finding.
So the buffers are genuinely freed, the allocator genuinely takes them back, and resident memory still doesn't fall. The freed space is recycled within the arena for the process's other per-connection allocations rather than accumulating as reclaimable free space — which is why neither
|
| default arenas | MALLOC_ARENA_MAX=2 |
|
|---|---|---|
| static buffers | 263,170 B/conn | 253,207 B/conn |
| escalating buffers | 266,620 B/conn | 253,674 B/conn |
~10–13 KB/conn, about 4%. Real, and available on main today as an env var with no code change — worth having on a high-connection-count node. But it does not unlock the copy-buffer saving: with arenas pinned the two arms are identical (253,207 vs 253,674). Arena count doesn't explain the missing memory any more than arena retention did.
Where that leaves #41
On the evidence I have, the claim that survives is the narrow one: peak live buffer bytes scale with concurrent transfers rather than connection count. The claim that doesn't survive is the one that motivated the change — that a node's resident memory falls. Across every instrument tried (RSS, mallinfo2, malloc_trim, arena tuning), the escalating path costs a per-burst allocation and returns no measurable RSS.
Honest limit on all of the above. bench-copy-memory runs the 8000-socket Node TLS client and symphony in one process, so the ~253 KB/conn being measured is dominated by client-side TLS state, and the run-to-run spread (~±13 KB/conn) is a large fraction of the ~128 KB/conn effect we're looking for. A copy-buffer difference that size should still be visible against it and isn't — but I'd want the measurement redone out-of-process before treating this as final. symphony-server makes that straightforward: drive it from a separate client process and sample the server's own RSS. That's the experiment that would either recover the effect or bury it, and it's the one I'd run before anyone invests further here.
What's on the branch
Independent of the above, and green (cargo test 118, cargo clippy --all-targets clean, npm test 112, all 11 CI checks):
lazyCopyBufferThreshold(default 1000, per proxy) gates escalation on the live active-connection count, re-read at each resize point rather than latched per connection. Below the threshold it is provably the old code path — one full-size buffer, never resized — so a small port-set pays none of the churn. This is what makes the branch safe to carry while the memory question is open.- The reload regression test asked for on Apply readBufferSize to the copy loop and split it per direction #36, covering all four construction-frozen proxy fields, with a route-only control and mutation-checked.
- A README correction. The text added in
a07f42dsays a recreate drops established connections. It doesn't —stop()ends the accept loops and sleeps 100 ms but never aborts connection tasks, and the runtime lives in the napi wrap until GC. Sessions keep running on the old buffer sizes until they close, which matters more than the original wording implied: lowerreadBufferSizeon a listener full of long-lived MQTT subscribers and the change reaches almost nothing until those clients reconnect. Now pinned by a test.
The allocator diagnostics used above (malloc_stats, malloc_trim_now, escalated_buffers) are deliberately not in this PR — they live on a local scratch branch, and I'd only land them if we decide the out-of-process measurement is worth building properly.
Claude (Opus 5), with Kris
|
Ran the out-of-process measurement I flagged as the deciding experiment. It decides it, and not in this PR's favour — the premise behind #37 doesn't hold. The measurement
668 B/conn apart, against a model predicting ~128 KB/conn. But the number that actually explains everything is the absolute one: symphony costs ~44 KB per connection in total. If the static arm held 2 × 64 KiB resident per connection it could not possibly be under 131 KB. Root cause: the buffers were never residentSweeping
About 4% of the buffer is ever resident. So It also reconciles the confusing middle of this investigation. Corrections to what I posted earlier today
Three revisions in a day, all from the same mistake in different clothes: measuring a process that contained the load generator, on an effect smaller than that process's own variance. RecommendationClose this, and #37 with it. The mechanism is correct and well-tested, but it optimises memory that was never resident, and it isn't free — it adds two allocations plus a zeroing per burst on the hot path that carries every proxied byte. The gating in What actually moves symphony's per-connection memory, measured properly:
#36 is the real lever and it already shipped in 1.2.0. For an MQTT fan-out port-set, a small per-direction Worth salvaging from this branch regardless of the outcome, as they stand on their own:
Happy to open a small PR with just those three if that's the preferred shape. Claude (Opus 5), with Kris |
Closes #37.
Base note: PR #36 (
kris/wire-read-buffer-size, not yet merged) introduces theper-direction
readBufferSize/clientReadBufferSize/upstreamReadBufferSizeconfigand the
DEFAULT/MIN/MAX_COPY_BUFFER_SIZEconstants this change builds on, sothis branch is based on it rather than
main. Only the last 2 commits(
fdebd34,95029e4) are this PR's content.Summary
tokio::io::copy_bidirectional_with_sizesallocates each direction'sCopyBufferonce, before the first read, and holds it for the connection's whole life — active
or not. At the default 8 KiB that's 16 KiB/connection dead weight for a parked MQTT
subscriber; at ~1M concurrent connections (the sizing target for this change) that's
gigabytes held for sessions doing nothing.
src/copy.rs::copy_bidirectional_lazyreplaces it with a direct port of tokio's owncopy_bidirectional_impl/CopyBufferstate machine(
transfer_one_direction/TransferState,poll_fnover both directions), with oneaddition:
LazyCopyBufferstarts each direction at a small fixed floor(
PROBE_BUFFER_SIZE= 512 B) and escalates to the full configuredreadBufferSize/*ReadBufferSizeonly once two consecutive reads exactlysaturate the current buffer — real evidence of a sustained burst — dropping back to
the floor the first time a read comes back under capacity. One full read isn't
enough evidence: a message that happens to exactly match the current (small) buffer
size is a coincidence, not a burst, and escalating on it would leave an oversized
buffer resident for however long the connection then sits idle. The extra cost is
one confirming small-buffer round trip per genuine burst — negligible.
readBufferSizeand its per-direction overrides therefore become a per-transfermaximum, not a permanent allocation, matching #37's proposal (option 1, lazy
allocation/release) over a per-worker pool: no new contention point, no
pool-sizing/exhaustion surface to design or get wrong, and it maps directly onto
"scale with concurrent transfers."
Wiring is a 2-line change in
src/proxy_conn.rs::forward— swap thecopy_bidirectional_with_sizescall forcrate::copy::copy_bidirectional_lazyonthe no-rewrite branch. The header-rewriting branch (
http_proxy::proxy_http1_rewriting)is untouched, as scoped.
Design iteration (documented in
CLAUDE.mdand commit95029e4)The first version used
tokio::io::split+ independent per-directionasync fnpumps — simpler to read, but an independent review (Codex) surfaced three real
problems, all fixed in the current version:
splitwraps each side inArc<Mutex<_>>— two heap allocations and alock/unlock on every read/write/flush/shutdown, per connection, scaling with
connection count — exactly what this change exists to avoid.
write_alland moves on doesn't flush beforeparking on the next read.
write_allonly guarantees the data reached thewriter's internal buffer, not the wire —
tokio-rustlsin particular deferssending encrypted records until
poll_flush. A TLS client that sends one requestand waits for the reply would never see it: both sides idle until
idle_timeoutcleaned up the session.shutdown()error (matching the out-of-scopehttp_proxy.rsprecedent) diverges from
tokio::copy_bidirectional, which propagates it. Leftswallowed,
try_join!-equivalent short-circuiting never fires and the otherdirection can wait forever.
A non-blocking
poll_read-with-Waker::noop()peek (opportunistically draining"whatever's already queued" without the extra confirming iteration) was also tried
and dropped: under load it silently stranded a connection's wakeup — reproduced
empirically as an increasing fraction of connections stopped responding as
concurrency grew — in favor of the two-consecutive-full-reads scheme, which uses
only ordinary polling.
Invariants preserved (per issue +
CLAUDE.md)transfer_one_direction'sTransferState::Running → ShuttingDown → Donemirrors tokio's exactly — EOF on one reader shuts down the otherdirection's writer, the other direction keeps running to its own EOF/error.
poll_fnuses?(try-join semantics), so anerror on either direction — including a failed
poll_shutdown(), now propagatedrather than swallowed — ends the whole copy immediately.
ActiveGuard/BalancerGuarddrop handles both paths unchanged.
forward()'stimeout(...)wrapsthe whole call the same as before;
ErrorKind::IdleTimeoutvsErrorKind::Streamis unchanged. Covered by the existing idle-timeout test in
metrics.spec.ts(passing, see below).
CountingStreamstill wraps the client side outside thisfunction; unaffected by what's inside the copy loop.
__test__/copy-buffers.spec.ts(from Apply readBufferSize to the copy loop and split it per direction #36, now running through this loop) — client TCP/TLS × upstream TCP/UDS.
proxy_http1_rewriting.readBufferSizeconfig surface.Tests
src/copy.rsunit tests (8, all passing):large_payload_integrity_through_a_small_buffer,rst_on_one_leg_ends_the_copy_immediately,half_close_from_client_side_lets_upstream_direction_finish,half_close_from_upstream_side_lets_client_direction_finish,single_byte_message_with_nothing_queued_behind_it_is_not_held_waiting_for_more,write_is_flushed_before_parking_on_the_next_read,shutdown_failure_is_propagated_not_swallowed,buffer_escalates_on_a_sustained_burst_and_shrinks_back_once_it_ends(theescalate/de-escalate path — this design's analogue of a pool-exhaustion test).
Full run on this branch, Linux (a real toolchain was required — this can't be
validated on macOS alone, and none of this was checked on the requesting machine,
which has no cargo):
cargo test: 115 passed, 0 failedcargo clippy --all-targets: clean (noclippy::allviolations; the onlywarnings present are pre-existing dead-code lints in unrelated modules, unchanged
by this PR)
npm test: 105 passed, 0 failed — includescopy-buffers.spec.ts(Apply readBufferSize to the copy loop and split it per direction #36, all 4stream combinations) and
metrics.spec.ts's idle-timeout classification testMeasurements (the actual deliverable — issue #37 explicitly asks for the memory
curve, not an assertion)
Memory —
__test__/bench-copy-memory.ts(npm run benchmark:copy-memory, newin this PR): 15,000 TLS-terminated connections,
readBufferSize= 64 KiB/direction,each sends one 64 KiB burst (forcing the old static buffer fully resident) then
goes quiet, measuring steady-state RSS delta 5 s after the burst settles:
copy_bidirectional_with_sizes,readBufferSize=64 KiB)~18% less resident memory per connection at this buffer size (recorded in commit
95029e4; methodology and full script in__test__/bench-copy-memory.ts). Ire-ran the "after" side fresh before opening this PR and got 259,454 B/conn —
consistent with 251,429 within run-to-run noise, confirming the current build
reproduces the recorded result. I also attempted to re-run the "before" side fresh
for a same-session before/after pair, but this box is currently running several
other agents' builds/tests concurrently (load average ~5 on 20 cores, multiple
concurrent
cargo/nodeprocesses) and the burst sends mostly stalled past thebenchmark's 3 s per-write timeout (8,700–14,400 of 15,000), which understates the
old code's resident memory rather than measuring it — I'm not including that noisy
rerun and am relying on the original, already-documented before/after pair instead.
At the default 8 KiB buffer the gap should be more pronounced in relative terms
(the fixed 512 B floor is a smaller fraction of a smaller ceiling) — not re-measured
here since the acceptance criteria's stated concern (~1M connections, MQTT) is
exactly the shape the 64 KiB run already demonstrates: memory tracking peak
concurrent transfers, not connection count.
Throughput (no regression on the bulk path) —
__test__/bench-copy-throughput.ts(npm run benchmark:copy-throughput, new inthis PR): 4 TLS-terminated connections sustained for 8 s, matching the replication
profile on port 9933 (few connections, high volume, opposite shape from the memory
test):
95029e4: ~1700–1900 MiB/s both before and after (within run-to-runnoise, not conclusively distinguishable — removing
tokio::io::split'sArc<Mutex<_>>from the discarded first design plausibly helps here, though thefinal design's difference from tokio's original loop is only the buffer-resize
decision, which sits off the hot path once escalated).
within that same recorded range, confirming no regression on this build.
Out of scope / unchanged
readBufferSizeconfig surface (Apply readBufferSize to the copy loop and split it per direction #36's) areunchanged.
http_proxy::proxy_http1_rewriting) is untouched.Draft — not requesting review yet; opening for CI + visibility while the
before/after write-up above gets a second look. PR #36 should land first since this
branch is based on it.