streams: eagerly clear ReadableStream source WriteBarriers once terminal - #36666
Conversation
Once a ReadableStream reaches a state where no more user source callbacks
(pull/cancel/close) can run, the WriteBarriers that exist only to feed those
callbacks are dead weight that keeps whatever they point at alive for as long
as the stream object itself is reachable.
Clear them at the same points the spec controllers already clear their
algorithm slots, and at the equivalent terminal points for Bun's direct and
native controllers:
JSReadableStream
m_asyncContext snapshotted ALS context restored around
pull/cancel; useless once those cannot run
m_directUnderlyingSource held only until materialize or cancel
JSDirectStreamController
m_underlyingSource / m_pull / m_deferCloseReason
drop once m_closed is set or the stream
left Readable
New tests hold the stream (and controller) across GC and watch a
FinalizationRegistry for a probe reachable only through the cleared slot;
before this change all 200 probes survived, after it they are collected.
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
@robobun can we also clear the native m_directUnderlyingSource after materialization or will that break $bunNativePtr |
|
For If the question is really about
The adapter already holds its own reference to the handle ( |
|
@robobun if we can route it through the adapter when needed that would be better. The fewer references to these JSCell objects the better. |
![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 traced the two other readers of m_asyncContext after a clear — StreamAsyncContextScope (WebStreamsMisc.cpp:358) and wrapWithAsyncContext (BunStreamSource.cpp:263) both early-return on an empty slot, and readableStreamCancel's Default/Byte arms run the user cancel under StreamAsyncContextScope before ClearAlgorithms clears it, so ALS is still visible in cancel().
Extended reasoning...
This PR eagerly clears WriteBarrier slots on JSReadableStream and JSDirectStreamController once the stream is provably terminal. The change is small in line count but the correctness argument spans every controller kind's state machine, and one inline finding documents a real (if arguably-improving) observable behavior change in the cancel-then-async-pull-rejects race. That plus the GC-lifetime sensitivity puts it outside auto-approval; deferring to a human.
…st consolidation Clearing m_nativePtr (at materialize or at ClearAlgorithms) breaks PendingValue::size_hint(): Bun.inspect(response) re-derives *mut ByteStream via ReadableStreamTag__tagged to read bytes.size_hint, and FetchTasklet's readable_stream_ref path only updates the ByteStream's size_hint (not the PendingValue fallback field). With the slot cleared, tag returns JavaScript and inspect falls back to a stale first-chunk value (test/js/web/fetch/fetch.stream.test.ts 'response inspected size'). m_nativePtr keeps the handle alive past terminal state so inspect can report the final byte count; that is intentional, so leave it alone. What remains: readableStreamCancel Direct arm: set m_closed and call the shared directStreamControllerClearSource helper instead of three inline .clear()s, so a late onDirectPullRejected short-circuits in handleError. directStreamControllerClearSource: declared in WebStreamsInternals.h and defined in Bun::WebStreams so ReadableStreamOperations.cpp can call it. readableStreamReaderGenericRelease: drop the redundant m_nativePtr gate; kind == Native already implies a live adapter with a handle. readable-stream-terminal-barrier-release.test.ts: one subprocess, one GC storm, no per-test timeout.
…ancel) The barrier-release scenarios test behavior that #36666 already shipped, so the gate's without-src build passes them. Add a case for the one new observable change in this PR: readableStreamCancel's Direct arm now sets m_closed, so a captured controller's write() throws 'closed' instead of silently writing into the cleared sink (main returns the byte count, 12).
…ect cancel (#36703) Follow-up to #36666 addressing the review notes there, plus a test consolidation. ## Changes - **`readableStreamCancel` Direct arm**: set `controller->m_closed = true` and call the shared `directStreamControllerClearSource` helper instead of three inline `.clear()`s. `onClose` early-returns in this arm because the stream is already Closed, so `m_closed` was left false; a late `onDirectPullRejected` would then re-enter `handleError` and reach `callUnderlyingSourceClose` on the now-null `m_underlyingSource`. Setting `m_closed` makes the stated invariant hold and short-circuits that path. - **`directStreamControllerClearSource`**: moved into `Bun::WebStreams` and declared in `WebStreamsInternals.h` so `ReadableStreamOperations.cpp` can reuse it. - **`readableStreamReaderGenericRelease`**: drop the redundant `stream->m_nativePtr` gate; `kind == Native` already implies a live adapter whose `handle()` the body reads. - **`readable-stream-terminal-barrier-release.test.ts`**: collapsed into a single subprocess with one shared GC storm and no per-test timeout override (the previous per-case subprocess shape brushed the default timeout under debug+ASAN). ## Why `m_nativePtr` is left alone The first revision of this PR cleared `m_nativePtr` once the adapter owned the handle. That broke `test/js/web/fetch/fetch.stream.test.ts` "response inspected size should reflect stream state": `Bun.inspect(response)` reports the body byte count via `PendingValue::size_hint()`, which re-derives `*mut ByteStream` through `ReadableStreamTag__tagged` on every call; `FetchTasklet::on_body_received` only updates `bytes.size_hint` on the ByteStream (not the `PendingValue` fallback field), so once the slot is cleared inspect falls back to a stale first-chunk value. Adding an adapter fallback to `ReadableStreamTag__tagged` fixed the fetch-body-delivery path but not this one, because the slot is also cleared at ClearAlgorithms (when the adapter has already severed). Handing the handle off cleanly needs the Rust side to cache the final size somewhere tag-independent; left for a separate change. <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 8 · 4 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/web/streams/readable-stream-terminal-barrier-release.test.ts bun test v1.4.0 (eab4848) test/js/web/streams/readable-stream-terminal-barrier-release.test.ts: (pass) ReadableStream releases source-only WriteBarriers once terminal [2752.80ms] 113 | }); 114 | const reader = rs.getReader(); 115 | reader.read().catch(() => {}); 116 | await pullStarted.promise; 117 | await reader.cancel(); 118 | expect(() => ctrl.write("after-cancel")).toThrow(/closed/); ^ error: expect(received).toThrow(expected) Expected pattern: /closed/ Received function did not throw Received value: 12 at <anonymous> (/workspace/bun/test/js/web/streams/readable-stream-terminal-barrier-release.test.ts:118:44) (fail) direct controller write() after reader.cancel() throws closed [43.42ms] 1 pass 1 fail 4 expect() calls Ran 2 tests across 1 file. [4.79s] error: script "bd" exited with code 1 __F:1:S:0 release without fix: all passed bun test v1.4.0-canary.1 (5589d58) test/js/web/streams/readable-stream-terminal-barrier-release.test.ts: (pass) ReadableStream releases source-only WriteBarriers once terminal [27.64ms] (pass) direct controller write() after reader.cancel() throws closed [0.68ms] 2 pass 0 fail 4 expect() calls Ran 2 tests across 1 file. [163.00ms] __F:0:S:0 ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/web/streams/readable-stream-terminal-barrier-release.test.ts bun test v1.4.0 (eab4848) test/js/web/streams/readable-stream-terminal-barrier-release.test.ts: (pass) ReadableStream releases source-only WriteBarriers once terminal [2665.85ms] (pass) direct controller write() after reader.cancel() throws closed [43.81ms] 2 pass 0 fail 4 expect() calls Ran 2 tests across 1 file. [4.72s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 711ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/36] gen cpp.rs (cppbind) [1/36] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) �[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) �[1m�[92m Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp) �[1m�[92m Compiling�[0m bun_brotli v0.0.0 (/workspace/bun/src/brotli) �[1m�[92m Compiling�[0m bun_output v0.0.0 (/workspace/bun/src/output) �[1m�[ ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` .../webcore/streams/JSDirectStreamController.cpp | 20 +- .../webcore/streams/ReadableStreamOperations.cpp | 7 +- .../bindings/webcore/streams/WebStreamsInternals.h | 3 + ...eadable-stream-terminal-barrier-release.test.ts | 243 ++++++++------------- 4 files changed, 108 insertions(+), 165 deletions(-) ``` </details> **gate history** · 4 passed · 1 rejected · iteration 8 <details><summary>evidence per changed file</summary> ``` file reads edits tests …c/bindings/webcore/streams/JSDirectStreamController.cpp 7 6 0 …c/bindings/webcore/streams/ReadableStreamOperations.cpp 8 8 0 src/jsc/bindings/webcore/streams/WebStreamsInternals.h 5 3 0 …treams/readable-stream-terminal-barrier-release.test.ts 4 15 0 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
2 participants
Once a ReadableStream reaches a state where no more user source callbacks (pull / cancel / close) will ever run, several
WriteBarriers on the stream and its controller are dead weight. They keep whatever they point at (notably the construction-timeAsyncLocalStoragesnapshot and, fortype: "direct", the user's underlyingSource object with its closures) alive for as long as the stream itself is reachable.What gets cleared
JSReadableStream(via newreadableStreamClearSourceBarriers):m_asyncContext: the ALS snapshot restored aroundpull/cancel.StreamAsyncContextScopeis only entered from the controller's algorithm dispatch, so once those algorithms are cleared this slot is never read again.m_directUnderlyingSource: only consulted before materialization (setUpDirectStreamController,readDirectStream,consumeDirectStreamToArrayBuffer). On cancel/error it is dead.JSDirectStreamController(via newdirectStreamControllerClearSource):m_underlyingSource,m_pull,m_deferCloseReason: oncem_closedis set (or the stream has left Readable),onPull's state check guaranteescallDirectPullandcallUnderlyingSourceClosenever run again.Where it is called
readableStreamDefaultControllerClearAlgorithms/readableByteStreamControllerClearAlgorithms: the spec's existing "no more callbacks" point for default and byte controllers. Coverscontroller.close(),controller.error(), andcancelSteps.readableStreamError: coversControllerKind::None/ControllerKind::NativeSinkstreams errored viawebStreamControllerError.readableStreamCancel: covers cancel ofNone/Direct/NativeSinkstreams, where no ClearAlgorithms runs. For the Direct arm, also clears the direct controller's own slots (itsonCloseearly-returns because the stream is already Closed).JSDirectStreamController::onClose/handleError: right aftercallUnderlyingSourceClose, the last user callback.All call sites are after the last read of the slot and the helper is idempotent.
Verification
test/js/web/streams/readable-stream-terminal-barrier-release.test.tsspawns a child per case that creates 200 streams under anals.run({ probe })(or, for the direct-pending case, registers the underlyingSource itself), retains the stream objects, drives them to a terminal state, then GC-storms and counts live probes via aFinalizationRegistry.Before this change every case reported
alive == 200. With it, probes are collected.WPT streams suite (1175 tests) and the existing stream/ALS tests still pass; a sanity script confirms
als.getStore()insidepullandcancelstill sees the captured store.