streams: direct controller methods no-op (not throw) once closed - #36779
Conversation
….x write-after-cancel) #36703 set m_closed=true in readableStreamCancel's Direct arm so the bound controller methods would throw after cancel. That is a user-visible behavior change from v1.3.x, where a producer's in-flight pull() could still call write()/end() after the consumer cancelled and get the byte count back without a throw. Drop the m_closed assignment. The source-clearing call is kept, and the handleError path #36703 was guarding is already safe: callUnderlyingSourceClose and closeDirectSinkForError both early-return on a cleared source, and readableStreamError is gated on stream state == Readable. The test that asserted a throw is flipped to pin the non-throwing contract.
WalkthroughChangesDirect stream cancellation
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/web/streams/readable-stream-terminal-barrier-release.test.ts`:
- Around line 119-120: Extend the regression test around ctrl.write and ctrl.end
to assert that ctrl.flush() and ctrl.error(...) do not throw after cancellation,
placing the ctrl.error(...) assertion last. Preserve the existing write
return-value and end no-throw assertions.
🪄 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: 2f9dd812-b7d0-421a-a129-8d98e720d6cf
📒 Files selected for processing (2)
src/jsc/bindings/webcore/streams/ReadableStreamOperations.cpptest/js/web/streams/readable-stream-terminal-barrier-release.test.ts
ab67561 to
c376c09
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 8:44 AM PT - Aug 2nd, 2026
✅ @robobun, your commit c8be033d38e691e41a2fdce8f1690289f5612de4 passed in 🧪 To try this PR locally: bunx bun-pr 36779That installs a local version of the PR into your bun-36779 --bun |
…ink__doClose leak) ctrl.error() after cancel reaches handleError -> closeDirectSinkForError -> sink.close() -> ArrayBufferSink__doClose, which nulls m_sinkPtr before calling ArrayBufferSink__close (end only). ~JSArrayBufferSink then skips __finalize and the 48-byte native struct + its Vec<u8> leak under LSAN with BUN_DESTRUCT_VM_ON_EXIT=1. That codegen bug is orthogonal to this PR; the write()/end()/flush() assertions cover the v1.3.x contract being restored.
3879308 to
a6fb7a6
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found — the safety claims check out (onClose early-returns on state != Readable, callUnderlyingSourceClose/closeDirectSinkForError null-guard, handleError gates readableStreamError on Readable). Deferring to a human because this flips the contract that #36703's test was explicitly pinning (throw → no-throw) and #34854 is cited as making the same choice; a maintainer should sign off on which direction the direct-controller post-cancel semantics land.
What was reviewed
- Traced post-cancel
write()/end()/flush()/error()throughboundDirect*→onClose/onFlush/handleErrorwithm_closed=falseand a cleared source; each path early-returns or no-ops safely. - Confirmed
onClosereturns before settingm_closed(stream isClosed), so the test'send()-then-flush()ordering is sound. - Checked
directStreamControllerClearSourcedoes not clearm_arrayBufferSink, sowrite()still returns a byte count as asserted.
Extended reasoning...
Overview
One-line C++ change in readableStreamCancel's ControllerKind::Direct arm: the controller->m_closed = true assignment added by #36703 is replaced with a comment. The accompanying test in readable-stream-terminal-barrier-release.test.ts is flipped from asserting that ctrl.write() after reader.cancel() throws /closed/ to asserting it returns the byte count and that end()/flush() don't throw. directStreamControllerClearSource(controller) is retained.
Security risks
None. This is internal stream-controller state management; no untrusted-input parsing, auth, or resource-limit surfaces are touched.
Level of scrutiny
Medium-high. The mechanical change is trivial and I verified the paths the PR description names: after cancel, the stream is Closed, so onClose (via end()) early-returns at the state != Readable gate without touching m_closed; onFlush finds no pending read and no queued requests and returns; handleError's closeDirectSinkForError/callUnderlyingSourceClose both null-guard, and its readableStreamError call is gated on Readable. So the safety concern #36703 might have been guarding is indeed handled without m_closed.
What I can't sign off on unilaterally is the API decision. #36703 shipped with a test that explicitly asserted the throw, and this PR flips that test to assert the opposite. The description also flags open PR #34854 as making the same m_closed choice. That's design churn on a Bun-specific extension's user-visible contract — whether a producer's late write() after consumer cancel should throw or silently succeed — and a maintainer should confirm "restore v1.3.x" is the intended direction rather than #36703's tightening being the intended one.
Other factors
All inline review threads (comment-cop long-comment, CodeRabbit's flush()/error() coverage ask) are resolved; the ctrl.error() assertion was dropped for a documented pre-existing LSAN leak in ArrayBufferSink__doClose, which is tracked separately and unrelated to this change. The evidence block shows the test failing on main and passing with the fix under both debug/ASAN and release.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
closed should be true when closed. We just don't want to throw because of it.
…tead of throw Per review: m_closed should accurately track the closed state; the fix is to stop the bound write/end/close/flush/error handlers from throwing on it. write() returns 0 (no bytes written), the rest return undefined. Applies uniformly whether m_closed was reached via reader.cancel(), end(), or error().
|
Reworked in 11d1a73:
|
robobun
changed the title
There was a problem hiding this comment.
I didn't find any bugs, but this changes the user-visible contract of the type: "direct" controller (throw → no-op, with write() now returning 0 rather than v1.3.x's byte count), so a maintainer should sign off on the API choice.
What was reviewed:
- Traced all four bound handlers — the
m_closedearly return is hit before any sink call, so the earlier LSAN concern onctrl.error()no longer applies. - Confirmed
ReadableStreamOperations.cpp's cancel arm still setsm_closed = trueand clears the source; the barrier-release test's invariants are preserved. - Checked that
onClose/onFlush/handleErroralready guardm_closedinternally, so folding the check into the bound wrapper doesn't skip any cleanup.
Extended reasoning...
Overview
Two files: JSDirectStreamController.cpp swaps the throwVMTypeError on m_closed in the four bound direct-controller handlers (write/close+end/flush/error) for early no-op returns (write → jsNumber(0), the rest → jsUndefined()), and drops the now-unused directControllerClosedMessage constant. The regression test is rewritten from asserting a throw to asserting the no-op contract on both the reader.cancel() and controller.end() paths.
Security risks
None. No untrusted input parsing, allocation, or lifetime changes; the early returns strictly do less work than before.
Level of scrutiny
This is core streams C++ and a user-facing Bun-specific API (ReadableStream({ type: "direct" })). The mechanical change is small and clearly correct, but it embeds an API design decision: post-close controller calls now silently no-op instead of throwing. That is neither the pre-#36703 behavior (post-cancel write() returned the byte count because m_closed wasn't set; post-end() write() threw once the deferred close ran) nor the WHATWG default-controller convention (enqueue() on a closed stream throws). The uniform no-op is defensible — a late call from an in-flight async pull() after consumer cancel would otherwise surface as an unhandled rejection — but per the repo's review guidance, changes to user-facing API surface warrant maintainer sign-off rather than bot approval.
Other factors
The PR went through two designs (drop m_closed on cancel vs. keep it and no-op the handlers); the final one is cleaner and keeps ReadableStreamOperations.cpp untouched. All prior bot threads (comment-cop, CodeRabbit coverage nit, LSAN concern) are resolved — the ctrl.error() assertion is now safe because boundDirectError returns before ever reaching closeDirectSinkForError. The [[unlikely]] hint now also covers m_closed, which is reasonable for a post-terminal call. Test verification (fail-on-main / pass-on-PR, ASAN + release) is documented in the description.
There was a problem hiding this comment.
No bugs found. Deferring because this defines a new user-visible contract for Bun's type: "direct" controller: post-close write() now returns 0 (neither the v1.3.x byte-count nor the post-#36703 throw), and end/close/flush/error silently return undefined. That's a reasonable choice, but it's an API-surface decision a maintainer should sign off on.
What was reviewed:
- All four bound handlers change uniformly;
directControllerClosedMessagehas no remaining references anywhere (src or tests). - Post-cancel
ctrl.error()in the test is safe now —m_closedshort-circuits beforehandleError, so theArrayBufferSink__doCloseleak path discussed earlier is not reached. - The now-unused
DECLARE_THROW_SCOPEin close/flush/error is still consumed byRETURN_IF_EXCEPTION, so no dead scope.
Extended reasoning...
Overview
Two files. In JSDirectStreamController.cpp, the four jsWebStreamsHandler_boundDirect{Write,Close,Flush,Error} host functions are changed so that when controller->m_closed is set they return early (write → jsNumber(0), the rest → jsUndefined()) instead of throwVMTypeError(..., directControllerClosedMessage). The now-unused error-message constant is deleted. The test file replaces the previous "throws /closed/" assertion with exact-value assertions on all four methods, for both the reader.cancel() path and the controller.end() path.
Security risks
None. No untrusted-input parsing, no allocation, no new code paths — the change strictly reduces the work done on the m_closed branch to an immediate return. Exception-scope discipline is preserved (each function still declares a throw scope and checks it after the underlying call on the not-closed branch).
Level of scrutiny
Medium. The C++ change itself is mechanical and low-risk. What raises it above rubber-stamp territory is that it fixes a regression by picking a third behavior rather than a straight revert: pre-#36703 release builds returned the written byte count (because m_closed was never set on cancel), post-#36703 throws, and this PR returns 0. Returning 0 is defensible (the bytes are discarded, and it matches the !controller fallback shape), but it is a new observable contract on a Bun-native API and per the repo's API-design guidance should get maintainer eyes.
Other factors
- The PR was reworked mid-review from "drop
m_closed = trueon cancel" to the current "keepm_closed, make handlers no-op", which is the cleaner layering (fix at the layer that owns the invariant).ReadableStreamOperations.cppis untouched in the final diff. - The earlier LSAN concern about
ctrl.error()no longer applies: withm_closedset before the assertion runs,boundDirectErrorreturns beforehandleError→closeDirectSinkForError, so re-adding theerror()assertion is safe. - Grepped for other consumers of the removed
directControllerClosedMessagestring and constant — none in src/ or test/, so no test will break on the message change. - All bot review threads are resolved; comment-cop's length complaints were addressed by trimming to one-line comments.
2 participants
#36703 set
m_closed = trueinreadableStreamCancel'sControllerKind::Directarm, and the boundwrite/end/close/flush/errorhandlers throwTypeError: ReadableStreamDirectController is now closedwheneverm_closedis set. That is a user-visible change from v1.3.x for a producer whose in-flightpull()keeps calling the controller after the consumer has cancelled (or after its ownend()/error()).Fix
Keep
m_closed = trueon cancel (the state is accurate) and change the bound handlers to no-op instead of throw oncem_closedis set:write()returns0,end()/close()/flush()/error()returnundefined.ReadableStreamOperations.cppis unchanged from main; the now-unuseddirectControllerClosedMessageconstant is removed.Verification
The test covers both the
reader.cancel()path and thecontroller.end()path. Open PR #34854 also throws onm_closedafter cancel and would need the same treatment if it lands.[stamp-90s] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file