streams: detach ArrayBuffers supplied to byte stream reads and enqueues - #32406
streams: detach ArrayBuffers supplied to byte stream reads and enqueues#32406robobun wants to merge 9 commits into
Conversation
|
Updated 2:25 PM PT - Jun 16th, 2026
❌ @robobun, your commit 551db26 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32406That installs a local version of the PR into your bun-32406 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a native ChangesReadableStream BYOB ArrayBuffer Detachment
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/builtins/ReadableByteStreamInternals.ts (1)
378-382:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate readable-zero and closed-nonzero responses before detaching.
Line 381 and Line 404 transfer the buffer before checking the state-specific byte-count rules. As written,
respond(0)/ a zero-lengthrespondWithNewView()while readable can invalidate the BYOB request and leave the read pending, while a non-zero response after close is transferred and then ignored by the closed-state path.🐛 Proposed fix
export function readableByteStreamControllerRespondWithNewView(controller, view) { $assert($getByIdDirectPrivate(controller, "pendingPullIntos").isNotEmpty()); let firstDescriptor: PullIntoDescriptor | undefined = $getByIdDirectPrivate(controller, "pendingPullIntos").peek(); @@ // Spec: transfer the supplied view's buffer (detaching it). Capture byteLength // first, since detaching zeroes it. const viewByteLength = view.byteLength; + const stream = $getByIdDirectPrivate(controller, "controlledReadableStream"); + const state = $getByIdDirectPrivate(stream, "state"); + if (state === $streamClosed) { + if (viewByteLength !== 0) throw $makeTypeError("view.byteLength must be 0 when the stream is closed"); + } else if (viewByteLength === 0) { + throw $makeTypeError("view.byteLength must be greater than 0"); + } firstDescriptor!.buffer = $transferBufferToCurrentRealm(view.buffer); $readableByteStreamControllerRespondInternal(controller, viewByteLength); } @@ const firstDescriptor = $getByIdDirectPrivate(controller, "pendingPullIntos").peek(); + const stream = $getByIdDirectPrivate(controller, "controlledReadableStream"); + const state = $getByIdDirectPrivate(stream, "state"); // Validate before transferring so an out-of-range respond does not detach the // buffer (matches the spec, which checks the range before TransferArrayBuffer). - if ( - $getByIdDirectPrivate($getByIdDirectPrivate(controller, "controlledReadableStream"), "state") === $streamReadable && - firstDescriptor.bytesFilled + bytesWritten > firstDescriptor.byteLength - ) - throw new RangeError("bytesWritten value is too great"); + if (state === $streamClosed) { + if (bytesWritten !== 0) throw $makeTypeError("bytesWritten must be 0 when the stream is closed"); + } else { + if (bytesWritten === 0) throw $makeTypeError("bytesWritten must be greater than 0"); + if (firstDescriptor.bytesFilled + bytesWritten > firstDescriptor.byteLength) + throw new RangeError("bytesWritten value is too great"); + } // Spec (ReadableByteStreamControllerRespond step 6): transfer the descriptor's // buffer, detaching the view that was vended through byobRequest. firstDescriptor.buffer = $transferBufferToCurrentRealm(firstDescriptor.buffer);Also applies to: 393-404
🤖 Prompt for 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. In `@src/js/builtins/ReadableByteStreamInternals.ts` around lines 378 - 382, The buffer transfer via $transferBufferToCurrentRealm should occur only after validating the byte-length response against state-specific rules. Move the validation logic that checks for invalid zero-length responses in readable state and non-zero responses in closed state to occur before the $transferBufferToCurrentRealm call and before capturing viewByteLength. This prevents the buffer from being detached when the response would be invalid. Apply this fix at both affected sites: the first location near line 378-382 in the respond method path and the second location near lines 393-404 in the respondWithNewView method path, ensuring validation happens before any buffer transfer in both cases.
🤖 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.
Outside diff comments:
In `@src/js/builtins/ReadableByteStreamInternals.ts`:
- Around line 378-382: The buffer transfer via $transferBufferToCurrentRealm
should occur only after validating the byte-length response against
state-specific rules. Move the validation logic that checks for invalid
zero-length responses in readable state and non-zero responses in closed state
to occur before the $transferBufferToCurrentRealm call and before capturing
viewByteLength. This prevents the buffer from being detached when the response
would be invalid. Apply this fix at both affected sites: the first location near
line 378-382 in the respond method path and the second location near lines
393-404 in the respondWithNewView method path, ensuring validation happens
before any buffer transfer in both cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd0feafc-505e-429b-a3d1-3e6cf14b49b8
📒 Files selected for processing (3)
src/js/builtins/ReadableByteStreamInternals.tssrc/jsc/bindings/ZigGlobalObject.cpptest/js/web/streams/streams.test.js
|
Addressed the review finding on `respond()`/`respondWithNewView()` in 5077025: both now apply the spec's state-specific byte-count checks before `TransferArrayBuffer`. A `respond(0)` (or zero-length `respondWithNewView()`) on a readable stream, and a non-zero response after close, now throw a TypeError without detaching the buffer, matching the spec and Node. Added regression tests for the readable `respond(0)` and zero-length `respondWithNewView()` cases. |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/builtins/ReadableByteStreamInternals.ts:393-404— The new validate-before-transfer block implements only one of the three pre-transfer checks the spec performs inReadableByteStreamControllerRespond: it has step 4c (bytesFilled + bytesWritten > byteLength→ RangeError) but not step 3a (closed +bytesWritten !== 0→ TypeError) or step 4b (readable +bytesWritten === 0→ TypeError). SobyobRequest.respond(0)on a readable stream — which per spec must throw TypeError without side effects — now reaches line 404 and detaches the vendedbyobRequest.view, then silently returns; same shape forrespond(n>0)afterclose(). The missing throws themselves are pre-existing, but since this commit's explicit goal is "validate before transferring so a rejected respond does not detach the buffer", consider adding the two missing checks alongside the range check.Extended reasoning...
What the bug is
Commit 7805e67 added a pre-transfer guard to
readableByteStreamControllerRespondwith the comment "Validate before transferring so an out-of-range respond does not detach the buffer (matches the spec, which checks the range before TransferArrayBuffer)", plus an unconditionalfirstDescriptor.buffer = $transferBufferToCurrentRealm(firstDescriptor.buffer)at line 404. However, the spec'sReadableByteStreamControllerRespondhas three checks before theTransferArrayBufferstep, not one:- Step 3a (state =
"closed"): ifbytesWrittenis not 0, throw aTypeError. - Step 4b (state =
"readable"): ifbytesWrittenis 0, throw aTypeError. - Step 4c (state =
"readable"): iffirstDescriptor.bytesFilled + bytesWritten > firstDescriptor.byteLength, throw aRangeError. ← only this one was added.
Because steps 3a/4b are absent (and have never existed in Bun), calls the spec rejects now fall through to the new transfer at line 404 and detach the vended
byobRequest.view.The code path that triggers it
For
respond(0)on a readable stream:ReadableStreamBYOBRequest.prototype.respond(0)checks only$isReadableStreamBYOBRequestand that the controller is set, then forwards to$readableByteStreamControllerRespond(controller, 0). NobytesWritten === 0guard.- Line 388:
0is not NaN/Infinity/negative — passes. - Lines 396–400: state is
$streamReadable, but0 + 0 > byteLengthis false — passes. - Line 404:
firstDescriptor.buffer = $transferBufferToCurrentRealm(firstDescriptor.buffer)runs unconditionally, detaching the buffer that the source's savedbyobRequest.viewpoints at. RespondInternal→RespondInReadableState:InvalidateBYOBRequest,bytesFilled += 0, thenif (bytesFilled < elementSize) return— early-returns without throwing.
For
respond(n > 0)on a closed stream: the line-397 guard is gated onstate === $streamReadable, so it is skipped entirely; line 404 detaches;RespondInClosedStateignoresbytesWrittenand never throws.Why existing code doesn't prevent it
There is no
bytesWritten === 0/bytesWritten !== 0check anywhere on therespond()path — not inReadableStreamBYOBRequest.prototype.respond, not inreadableByteStreamControllerRespond, andRespondInClosedStateignores the argument. Those checks have simply never been implemented in Bun (a pre-existing spec gap). What changed in this PR is that line 404 now performs a real transfer; before this PR,transferBufferToCurrentRealmwas a no-op stub and there was no transfer call inrespond()at all, sorespond(0)was a pure no-op aside from invalidating the BYOB request.Step-by-step proof
let savedView, lenAfter; const stream = new ReadableStream({ type: "bytes", pull(c) { savedView = c.byobRequest.view; // Uint8Array over firstDescriptor.buffer c.byobRequest.respond(0); // spec: TypeError (step 4b). Bun: no throw. lenAfter = savedView.byteLength; // pre-PR: 4. post-PR: 0 (buffer was detached at line 404) c.error(new Error("stop")); }, }); const reader = stream.getReader({ mode: "byob" }); await reader.read(new Uint8Array(4)).catch(() => {}); console.log(lenAfter); // pre-PR: 4 post-PR: 0 spec/Node: never reached (respond(0) threw)
Trace:
pullIntotransfers the caller's buffer →firstDescriptor.buffer = B1. ThebyobRequestgetter createssavedViewoverB1.respond(0)passes both guards, line 404 transfersB1(detachingsavedView),RespondInReadableStatedoesbytesFilled += 0and early-returns. No throw;savedView.byteLengthis now0.Impact
The missing TypeError throws are a pre-existing spec deviation; this PR neither introduced nor removed them. The PR-introduced change is that the vended view's buffer is now detached on these spec-invalid calls, where pre-PR it stayed attached. That's a minor observable difference (a saved
byobRequest.viewbecomes unusable vs. staying live) and the trigger is already-incorrect user code, so practical impact is small. But it's squarely in the theme this commit explicitly addressed — "a rejected respond does not detach the buffer" — and it added a test for exactly that property in the over-fill case, so completing the set seems in scope.How to fix
Add the two missing checks alongside the existing one, before the transfer:
const state = $getByIdDirectPrivate($getByIdDirectPrivate(controller, "controlledReadableStream"), "state"); if (state === $streamClosed) { if (bytesWritten !== 0) throw new TypeError("bytesWritten must be 0 when responding on a closed stream"); } else { $assert(state === $streamReadable); if (bytesWritten === 0) throw new TypeError("bytesWritten must be greater than 0 when responding on a readable stream"); if (firstDescriptor.bytesFilled + bytesWritten > firstDescriptor.byteLength) throw new RangeError("bytesWritten value is too great"); } firstDescriptor.buffer = $transferBufferToCurrentRealm(firstDescriptor.buffer);
- Step 3a (state =
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/builtins/ReadableByteStreamInternals.ts (1)
338-342:⚠️ Potential issue | 🟠 MajorThe NativeReader enqueue path needs implementation or clarification.
readableStreamReaderKind()explicitly returns case 3 when a reader has bothreadRequestsand$bunNativePtr(line 290:return reader.$bunNativePtr ? 3 : 1). TheshouldCallPull()function also checks for this condition (line 278:if (reader && ($getByIdDirectPrivate(reader, "readRequests")?.isNotEmpty() || !!reader.$bunNativePtr))), confirming that native readers can be attached to byte streams alongside read requests.However, case 3 in
readableByteStreamControllerEnqueue()(lines 339-342) has no implementation—only a commented-outreader.$enqueueNative()call and a barebreak. Meanwhile, cases 1, 2, and the default path all transfer the chunk's buffer via$transferBufferToCurrentRealm(), upholding the spec's detachment guarantee. If a native reader is attached, the buffer is never transferred, violating this invariant.Either implement native reader buffer delivery (uncommenting or completing the
$enqueueNativecall and ensuring buffer transfer), or add an explicit guard preventing case 3 from being reached with byte stream enqueue operations.🤖 Prompt for 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. In `@src/js/builtins/ReadableByteStreamInternals.ts` around lines 338 - 342, The case 3 NativeReader branch in the `readableByteStreamControllerEnqueue()` function (lines 339-342) lacks implementation for handling native reader buffer enqueue operations. The commented-out `reader.$enqueueNative()` call must be either uncommented and completed to properly deliver the chunk to the native reader while ensuring the buffer is transferred via `$transferBufferToCurrentRealm()` (consistent with cases 1, 2, and the default path to maintain the spec's buffer detachment guarantee), or alternatively, add an explicit guard or condition earlier in the function to prevent native readers (case 3) from reaching this enqueue path if native reader handling should be managed elsewhere. Choose the implementation approach based on the intended design for native reader support in byte streams.
🤖 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/streams.test.js`:
- Around line 1490-1538: Add test cases for closed-state validation of invalid
responses that are currently untested in the implementation. Create two new test
cases (similar in structure to the existing "respond(0)" and
"respondWithNewView(0)" tests) that: establish a pending BYOB read on a
ReadableStream, call controller.close() to close the stream, attempt to call
respond() or respondWithNewView() with non-zero values on the closed stream, and
verify that a TypeError is thrown without detaching the buffer (checking that
vendoredView.byteLength remains unchanged). These tests mirror the
readable-state tests but validate the closed-state branches in the respond and
respondWithNewView implementations.
---
Outside diff comments:
In `@src/js/builtins/ReadableByteStreamInternals.ts`:
- Around line 338-342: The case 3 NativeReader branch in the
`readableByteStreamControllerEnqueue()` function (lines 339-342) lacks
implementation for handling native reader buffer enqueue operations. The
commented-out `reader.$enqueueNative()` call must be either uncommented and
completed to properly deliver the chunk to the native reader while ensuring the
buffer is transferred via `$transferBufferToCurrentRealm()` (consistent with
cases 1, 2, and the default path to maintain the spec's buffer detachment
guarantee), or alternatively, add an explicit guard or condition earlier in the
function to prevent native readers (case 3) from reaching this enqueue path if
native reader handling should be managed elsewhere. Choose the implementation
approach based on the intended design for native reader support in byte streams.
🪄 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: 10723c5f-fd9d-4176-84df-54b0051d70d6
📒 Files selected for processing (2)
src/js/builtins/ReadableByteStreamInternals.tstest/js/web/streams/streams.test.js
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Can you do this without causing it to slowDownAndWasteMemory()?
|
Short answer: What this PR adds, in byte terms, is the transfer itself: it detaches the caller's view in Two levers to cut that, both your call:
The inline-storage copy for a bare |
transferBufferToCurrentRealm in ReadableByteStreamInternals was a no-op stub, so a ReadableStream byte source never performed the spec's TransferArrayBuffer. As a result a BYOB read(view), controller.enqueue(), and byobRequest.respondWithNewView() all left the caller-supplied ArrayBuffer attached and accessible, diverging from the WHATWG Streams spec and Node. Implement TransferArrayBuffer as a non-overridable native (), which detaches the source buffer and returns a new ArrayBuffer that owns the same bytes (zero-copy move). Transfer the view's buffer up front in readableByteStreamControllerPullInto so every path (pending, closed, queue-satisfied, async) detaches, and transfer in respondWithNewView and the enqueue paths. Geometry (byteOffset/byteLength) is read before the transfer since detaching zeroes it. Fixes #32402
Follow-up to the byte-stream detach work addressing three gaps: - byobRequest.respond(n) now performs the spec's step-6 TransferArrayBuffer on the descriptor's buffer, detaching the view vended through byobRequest (including on a partial respond). The range is validated before the transfer so an out-of-range respond throws without detaching. - A BYOB read() whose view is backed by a non-transferable buffer (SharedArrayBuffer, WebAssembly.Memory) now returns a rejected promise instead of throwing synchronously, matching the Streams spec and Node. - $transferArrayBuffer reports distinct errors for a non-ArrayBuffer argument, a SharedArrayBuffer, and a detached buffer, and guards WebAssembly.Memory buffers the way ArrayBuffer.prototype.transfer does.
readableByteStreamControllerRespond and respondWithNewView now apply the spec's state-specific byte-count checks before TransferArrayBuffer: a respond(0) (or zero-length respondWithNewView) on a readable stream, and a non-zero response after close, throw a TypeError without detaching the buffer. Previously the buffer was transferred first and the invalid response either left the read pending or was silently ignored. Matches the Streams spec and Node.
…thNewView - enqueue() on a BYOB reader with a pending pull-into now transfers that descriptor's buffer (spec step 8), detaching the view vended through byobRequest when a source enqueues instead of responding. - respondWithNewView() now rejects a view that exceeds the remaining space (accounting for bytes already filled, spec step 9) before the transfer, so a failed call does not detach the supplied buffer. Adds tests for both, plus closed-state validation of respond()/ respondWithNewView() (a non-zero response on a closed stream throws a TypeError without detaching).
Adds the spec step-8 check to readableByteStreamControllerRespondWithNewView: if the supplied view's buffer is not the same byte length as the pending descriptor's buffer, throw a RangeError before the transfer. Without it, a mismatched buffer was transferred into the descriptor, leaving its byteOffset/byteLength geometry invalid and making the next byobRequest getter throw out of bounds. Matches the spec and Node.
… per spec Two follow-ups to the byte-stream detach work: - respondWithNewView() compared the live firstDescriptor.buffer.byteLength against the new view's buffer. When a source transfers the vended buffer out and responds with a view over it (the canonical pattern), that buffer is detached (byteLength 0) and the check wrongly threw. Cache the descriptor's buffer byte length at creation (spec's "buffer byte length" field) and compare against that, so the check survives the caller detaching the buffer. - enqueue() on a BYOB reader transferred the pending descriptor's buffer and invalidated the byobRequest before transferring the chunk's buffer. Since the chunk transfer is the fallible step (spec step 7, before step 8), a chunk that cannot be transferred (SharedArrayBuffer) detached the vended view as a side effect of a failed enqueue. Transfer the chunk first so a failed enqueue has no side effects. Both match the spec and Node.
…View over-fill Two more spec-ordering/consistency fixes from review: - enqueue() case 2 now transfers the pending descriptor's buffer (spec step 8.d, which throws on an already-detached buffer per step 8.b) before invalidating the byobRequest (step 8.c). A source that detaches the vended buffer itself and then enqueues a valid chunk now gets the throw with the byobRequest still valid for recovery, instead of a pre-invalidated request. - respondWithNewView()'s over-fill check (spec step 9) now throws a RangeError instead of a TypeError, matching the spec, Node, and the identical check in respond(). Both verified against Node; tests updated/added.
…d over-fill - respondWithNewView() now captures view.buffer into a local and uses it for both the size check and the transfer, so a tampered buffer getter cannot pass validation with one buffer and transfer a different one (matches the spec's single [[ViewedArrayBuffer]] read). - The over-fill check at the top of readableByteStreamControllerRespondInReadableState is unreachable now that both callers validate before transferring, so it is a spec Assert rather than a throw. Added a test that the supplied view's buffer is read only once.
fd82b00 to
551db26
Compare
|
Rebased onto main (now includes the WebKit upgrade in #31796 and #32407); the branch is mergeable again. Only conflict was in Verified against the upgraded WebKit: debug build is clean and the byte-source detach suite passes (23/23). The two |
|
Covered by the web streams rewrite in #33193, which landed on main and closed #32402. Main now detaches the supplied buffer on BYOB read() and on byte stream enqueue(), and the builtins this PR patches (ReadableByteStreamInternals.ts) no longer exist. This PR's tests pass against main as-is, apart from one that only checks the wording of the SharedArrayBuffer error message (main still throws a TypeError there). Closing. |
2 participants
What
Fixes #32402. A
ReadableStreambyte source (type: "bytes") did not transfer (detach) the caller-suppliedArrayBuffer, so after a BYOBread(view)the original view and its buffer stayed attached and readable.Cause
transferBufferToCurrentRealminsrc/js/builtins/ReadableByteStreamInternals.tswas a no-op stub that returned the buffer unchanged, so the spec operationTransferArrayBufferwas never performed. Every call site that should detach (BYOBread,controller.enqueue,byobRequest.respondWithNewView) was affected.Fix
TransferArrayBufferas a non-overridable native$transferArrayBuffer(inZigGlobalObject.cpp, modeled on the existing$createUninitializedArrayBuffer). It detaches the source buffer and returns a newArrayBufferowning the same data block viaArrayBuffer::transferTo(zero-copy move).readableByteStreamControllerPullIntoso every path (pending pull-into, closed stream, queue-satisfied, async) detaches the caller's view, and transfer inrespondWithNewViewand theenqueuepaths. Buffer geometry (byteOffset/byteLength) is read before the transfer, since detaching zeroes it. The now-redundant transfers in the respond paths are removed.Internal native/direct streams (
Bun.file().stream(),Response.body, fetch/serve bodies) are unaffected: they use theReadableStreamDefaultControllerpath, which never routed through this helper. The native-reader enqueue case is left as a no-op as before.Verification
New tests in
test/js/web/streams/streams.test.jscover BYOB read, queue-satisfied BYOB read, closed-stream BYOB read,respondWithNewView,controller.enqueue, and enqueue with a waiting default reader. Each asserts the supplied buffer is detached and the delivered bytes are correct. All fail on the released binary and pass with the fix; the full streams suite (76 tests) and the HTTP body-stream suite (9086 tests) stay green.