uws: cursor-based BackPressure buffer so erase() is a pointer bump - #34824
Conversation
…d compacts instead of reallocating) The std::string-backed BackPressure front-erased and shrink_to_fit() every time pendingRemoval crossed 1/32 of the buffer, so draining N bytes paid ~32 passes of memmove(remaining) + realloc(remaining) + memcpy(remaining), and each realloc briefly held old + new allocations. Replace it with a single malloc'd slab and head/tail cursors: - erase(n) bumps head; on full drain resets to 0,0 and frees. - append()/resize() reuse the drained head gap via one memmove before growing, and use realloc() when head==0 so the allocator can extend in place. - getBufferedAmount() reports unsent bytes (length()); memoryCost() reports allocation footprint (totalLength()). Adds integrity tests that push large direct and corked-frame sends through a backpressured ServerWebSocket and sha1-compare the drained bytes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesThe WebSocket backpressure buffer now uses manually managed contiguous storage with explicit compaction, growth, and release. Buffered amount reports live unsent bytes, memory reporting uses allocated capacity, and paused-client tests validate large and repeated-frame draining. WebSocket backpressure
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:59 PM PT - Jul 20th, 2026
✅ @robobun, your commit 8db78aa00b3d46f2a0c4408d10ae139ec89458fa passed in 🧪 To try this PR locally: bunx bun-pr 34824That installs a local version of the PR into your bun-34824 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nits, I also checked: the getSendBuffer() fallback's resize() + write-at-data() + existingBackpressure pattern still lands at the correct offset after compaction/grow (head resets to 0, live bytes at [0, live)); and every getBufferedAmount() caller in HttpContext.h / WebSocketContext.h / HttpResponse.h is an == 0, > 0, or before/after progress comparison, so the switch from totalLength() to length() doesn't break any of them.
This rewrites a hot-path buffer with hand-rolled malloc/head/tail cursors, changes user-visible getBufferedAmount() semantics, and trades incremental shrink for high-water retention until full drain — worth a maintainer sign-off.
Extended reasoning...
This is a full rewrite of uWS::BackPressure from std::string to a hand-managed malloc'd slab with head/tail cursors, sitting under every buffered HTTP/WebSocket write in Bun.serve. It also changes getBufferedAmount() from totalLength() (which previously included the already-written pendingRemoval prefix) to length() (unsent bytes only) — user-visible via ws.getBufferedAmount() and every maxBackpressure check — and changes memory-retention behavior (capacity held at high-water until full drain rather than shrinking every 1/32). It also overlaps with open PR #34023. That combination is well outside "simple/mechanical", so deferring to a human even though the only findings are nits.
…ject, drop unused local
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bun-uws/src/AsyncSocketData.h`:
- Around line 28-32: Condense the BackPressure comment in
packages/bun-uws/src/AsyncSocketData.h#L28-L32 to no more than three lines while
preserving its key rationale. Also condense the comment above the “many corked
frames” test in
test/js/bun/websocket/websocket-server-backpressure-buffer.test.ts#L143-L146 to
no more than three lines, without changing test behavior.
- Around line 96-125: Guard the size arithmetic in ensureTailRoom before any
capacity checks or growth calculations: validate tail + n, live + n, and cap * 2
against SIZE_MAX and abort or otherwise reject the request on overflow. Ensure
newCap is computed only from checked values, preserving the existing compaction
and allocation behavior for valid sizes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51e9ec5f-c3e0-4e62-b889-cd545bd74eb8
📒 Files selected for processing (4)
packages/bun-uws/src/AsyncSocket.hpackages/bun-uws/src/AsyncSocketData.hpackages/bun-uws/src/WebSocket.htest/js/bun/websocket/websocket-server-backpressure-buffer.test.ts
Winsock's loopback send() accepts the full payload (100MB+ with the client paused) into kernel buffers so the WebSocket fast path never falls back to the uws BackPressure buffer on Windows. The cork-overflow test still exercises the buffer on that platform. Also defer the send until after the client has paused so the assertion that something was buffered is deterministic on POSIX.
These resolve to mimalloc in non-ASAN builds and to libc under ASAN, matching every other allocation in bun-usockets/bun-uws.
There was a problem hiding this comment.
No issues found in this pass — all earlier feedback (OOM handling, overflow guard, test timeouts/failure wiring, comment length, mimalloc routing) is addressed and the Windows test-2 concern was empirically refuted. Deferring only because this is a hand-rolled buffer replacing std::string on the send path for every Bun.serve HTTP/WS socket, plus a semantic shift in getBufferedAmount() (now unsent-only) — worth a human sign-off.
What was reviewed
ensureTailRoom: overflow guard, compact-vs-grow,us_realloc(nullptr, …)first-alloc, null-checked →abort().erase/clear/dtor free viaus_free(mi_* under non-ASAN);us_free(nullptr)is safe.- All
getBufferedAmount()call sites in HttpContext/WebSocketContext — the== 0/ progress checks are correct (arguably more so) underlength()semantics. data()only reached whenlength() > 0or afterresize(>0), sobufis never null there.
Extended reasoning...
Overview
Rewrites uWS::BackPressure from a std::string + pendingRemoval counter to a raw char* buf slab with head/tail/cap cursors, so erase() is a pointer bump and append()/resize() compact into the drained head gap before growing. getBufferedAmount() now returns length() (unsent bytes only) instead of totalLength(); memoryCost() reports cap for GC extra-memory accounting. Allocation is routed through us_malloc/us_realloc/us_free (mimalloc in non-ASAN builds) per Jarred's request. Two new integrity tests push 8MB (direct write2 path) and 2048×4KB (cork-overflow → resize() + compaction) through a paused raw-socket client and sha1-compare payloads.
Security risks
None identified. Inputs to the buffer are outbound data the server itself is sending; sizes come from application code, not the peer. The overflow guard n > SIZE_MAX - tail → abort() covers wrap on tail + n and live + n; cap * 2 wrapping is benign since live + n then wins the max(). OOM aborts, matching the old std::string behavior under -fno-exceptions.
Level of scrutiny
High. This is manual C++ memory management (raw owning pointer, move ctor, explicit free) sitting under every outbound write from Bun.serve — HTTP responses, WebSocket sends, cork-buffer overflow. A double-free, leak, or off-by-one here would affect essentially all server workloads. The getBufferedAmount() semantic change is user-visible (JS ws.getBufferedAmount() now excludes already-written bytes still sitting behind the head cursor) and feeds maxBackpressure gating and drain-progress checks in WebSocketContext.h / HttpContext.h; I audited those call sites and they remain correct (the == 0 and before > after checks behave as intended, arguably more correctly than before), but that's exactly the kind of cross-cutting behavior change a maintainer should confirm.
Other factors
Every prior review thread on this PR is resolved: dead serverWs, handshake failure wiring, unchecked malloc/realloc, size-arithmetic overflow, per-test timeouts, comment-length cap, mimalloc routing. My earlier concern about test 2 failing on Windows was refuted with empirical evidence (5/5 local runs + all three Windows CI lanes on build 76270). The bug-hunting system found nothing new this round. Jarred has already engaged with the PR, so it's on a maintainer's radar; given the blast radius I'd rather they click merge than me.
2 participants
What does this PR do?
Replaces the
std::string-backeduWS::BackPressurewith a single malloc'd slab tracked byhead/tailcursors, so draining a backpressured socket is a pointer bump instead of a front-erasememmovefollowed by ashrink_to_fitrealloc.Why?
The previous shape:
A full drain of N buffered bytes crosses that 1/32 threshold ~32 times, and each crossing moves roughly the entire remaining buffer twice (once for the front-erase memmove, once for the shrink realloc).
append()separately went throughstd::string::append, which reallocates and copies the whole buffer (including the deadpendingRemovalprefix) whenever capacity is exceeded.Fix
BackPressureis nowchar *buf; size_t head, tail, cap;with the data contiguous in[head, tail):erase(n)bumpshead; on full drain it resets both cursors to 0 and frees.append()/resize()write attail. When the tail would overruncap, first try compacting into the drained head gap (onememmove); only grow when the live bytes plus the new bytes genuinely do not fit. Growth usesrealloc()whenhead == 0so mimalloc / glibc can extend in place, and drops dead head bytes otherwise.getBufferedAmount()now reportslength()(unsent bytes);memoryCost()reportstotalLength()(allocation footprint) so GC extra-memory reporting keeps reflecting the real heap allocation.clear()still releases the allocation, matching the previous behaviour on full drain.The API (
data(),length(),size(),resize(),reserve(),append(),erase(),clear(),totalLength()) is unchanged anddata()still spanslength()contiguous bytes, so no call site other thangetBufferedAmount/memoryCosthad to change.Relationship to #34023
That PR raises the 1/32 compaction threshold to 1/2 and drops the
shrink_to_fit, which removes the worst of the repeated realloc. This PR goes further by makingerase()free of any data movement and lettingappend()reuse the drained head space without growing. If #34023 lands first the conflict is trivial (both rewrite the same small struct).Measurements
Release build,
ws.sendBinarythroughBun.servewith a raw-socket drain (linux x64, average of 3):The steady-state streaming case is faster because each drain no longer memmoves and reallocates the live window; the single-send peak is unchanged because both implementations hold one ~256MB buffer and the brief
shrink_to_fit2x spike on main is shorter thanru_maxrsscan observe under mimalloc's mmap-backed large allocations. The new buffer retains its high-water capacity until the next full drain instead of shrinking per 1/32 step, which in the 8MB-window case shows as ~6MB higher steady RSS (56MB vs 50MB peak).How did you verify your code works?
New integrity tests in
test/js/bun/websocket/websocket-server-backpressure-buffer.test.tspush 32MB (directappend+erase) and 4096 x 4KB frames (cork overflow intoresize()while repeatedly compacting) through a backpressuredServerWebSocketto a raw-socket client and sha1-compare every payload byte.Also ran
node-http-backpressure.test.ts,serve-response-gc-backpressure-abort.test.ts,serve.test.ts,node-http.test.ts,websocket-server.test.ts; no new failures relative to main.no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/websocket/websocket-server-backpressure-buffer.test.ts