HarperFast / HarperFast/symphony

Copy-buffer memory scales with connection count instead of concurrent transfers

Open
#37 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
1
Forks
0
PR merge metrics
No merged PRs in 30d

Description

## Problem

Every proxied connection holds two copy buffers for its entire lifetime, whether or not it is transferring anything. `tokio::io::CopyBuffer` allocates its buffer eagerly and keeps it until the copy future is dropped, so for a fleet of mostly-idle MQTT subscribers the buffer memory is almost entirely dead weight:

```
buffer bytes = 2 × per-direction size × connections (regardless of activity)
```

At the 8192 default that is 16 KiB per connection — ~4 GiB at 262k connections, ~5.2 GiB at 333k. #36 makes the size configurable, which lets an operator trade this down to ~5 KiB/conn for MQTT, but it does not change the shape of the problem: memory still scales with *connection count* rather than with *concurrent transfers*.

For a broker fan-out workload the two are wildly different. Nearly every subscriber is parked between messages; the number of connections with bytes actually in flight at any instant is orders of magnitude smaller than the number of connections.

## Proposal

Make copy-buffer memory scale with concurrent transfers instead of connections. Two shapes, in increasing ambition:

1. **Lazy allocation** — allocate on the first readable event, release when the connection goes quiet. Simple, and captures most of the win for a park-and-wait workload.
2. **Per-worker buffer pool** — check a buffer out around a read→write cycle and return it when the cycle completes. Bounds total buffer memory by in-flight concurrency. A buffer must be *retained* while a write is partially complete (backpressure), so the true worst case is "connections with a pending partial write", which is self-limiting.

Either way this means replacing `copy_bidirectional_with_sizes` with a hand-rolled bidirectional copy. That is not foreign territory — `http_proxy.rs::proxy_http1_rewriting` is already a hand-rolled bidirectional loop.

Expected saving: ~16 KiB/conn at the default, or ~5 KiB/conn if MQTT is already tuned per #36 — roughly 5.3 GB at 333k connections, and the tuning knob stops mattering for idle connections.

## Constraints that must survive

- **Half-close**: the copy returns when *either* side closes, including on RST (documented in `CLAUDE.md`'s gotchas). Both directions must still shut down cleanly.
- **Idle timeout**: `forward()` wraps the copy in `timeout(ctx.idle_timeout, ...)` and reports `ErrorKind::IdleTimeout` distinctly from `ErrorKind::Stream`. Both classifications must stay correct.
- **Byte accounting**: `CountingStream` wraps the client side and publishes to shared counters every `COUNTER_FLUSH_BYTES`; every proxied byte must still be counted, including on sessions that end by idle timeout or reset.
- **All four stream combinations**: client side is either a raw `TcpStream` (passthrough) or a `TlsStream`; upstream is either TCP or UDS.
- **`readBufferSize` / `clientReadBufferSize` / `upstreamReadBufferSize`** (from #36) become the *maximum* per-transfer buffer size rather than a permanent allocation.
- No added latency on the small-payload path — MQTT publishes are hundreds of bytes and must not wait on a pool.
- Bulk streams must not regress: replication (`9933`) is ~6 connections carrying high-throughput transaction streams, the exact inverse profile.

## Acceptance

- Unit tests: large-payload integrity through a small buffer, half-close in each direction, RST mid-transfer, idle timeout still fires and is still classified as `IdleTimeout`, and whatever pool-exhaustion path the chosen design has.
- **A measurement, not an assertion**: process RSS at a few tens of thousands of parked connections, before vs after. The whole point of the change is the memory curve, so the PR should show it.
- A throughput check on the bulk path so pooling doesn't cost the replication profile.

Contributor guide

Open the contributing guide

Research direction

Start by reading forward() and copy_bidirectional_with_sizes, then compare their behavior with the hand-rolled loop in http_proxy.rs::proxy_http1_rewriting. Trace timeout classification, CountingStream accounting, half-close behavior, and all four stream combinations before choosing a design. Done means the listed unit tests pass, bulk throughput does not regress, and an RSS measurement shows the intended parked-connection memory improvement.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
networking
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.