node:stream: take the native handle in Readable.fromWeb only from a stream no consumer has started

Closed
RO
16d ago
+314-35

Problem

  • Readable.fromWeb(stream) drops data when a consumer already started the native stream (getReader(), a partial read, cancel()). A 4 MiB Bun.file().stream() reads as 3932160 bytes, a Blob as 0.
  • tryTransferToNativeReadable (src/js/internal/webstreams_adapters.ts:37) takes the native fast path whenever stream.$bunNativePtr is an object, even after materializeNativeSource started the handle.
  • The first consumer of an errored native stream starts its source. After addAbortSignal(signal, proc.stdout) aborts, getReader() stalls the child.

Fix

  • jsFunctionTransferToNativeReadableStream is the one check-and-claim: it returns the handle only if hasPendingNativeSource() (NativePending, not locked, live handle). Otherwise fromWeb reads through a reader.
  • fromWeb adds signal after the Readable exists, as #42584 does. Inside the constructor, an aborted signal fails the reader path with a TypeError.
  • materializeNativeSource skips a stream that is not readable.
  • Verified: test/js/node/stream/node-stream.test.js (24 new cases, all fail on main).

Background

  • A native ReadableStream (file, blob, pipe) starts NativePending. The first consumer's materializeIfNeeded() calls handle.start() and installs a controller that owns the handle. The fromWeb fast path drives the handle.
  • Considered hiding a started handle in $bunNativePtr: process.stdin reads it.

Downsides

  • A started native stream stays unlocked until the Readable's first read, like any non-native stream (#43713 fixes both). It costs a discarded Readable.
  • An errored, never-read Bun.spawn stdout keeps its background drain: the parent buffers the child's output. Main blocks the child.
  • A signal costs one more native call, and a signal getter that throws cancels the web stream. Binary: .bun_builtins +735 bytes.
Notes

What changed since the first version. The first version claimed the handle before it built the Readable, and cancelled the handle when the constructor threw. After the merge with main the order is simpler. fromWeb adds the signal last, so no user code runs inside the constructor. The claim follows the constructor. A constructor that throws (an invalid highWaterMark) leaves the web stream untouched, as on main.

Why the signal moves. A started native stream now takes the reader path (ReadableFromWeb). Its constructor passed signal to super(). An aborted signal then ran _destroy before the private fields existed: the Readable failed with a TypeError and the source was not cancelled. On main that happens for every stream with no native handle. Without this part, a started native stream with an aborted signal would change from AbortError to TypeError. On the native path the old order ran destroy() before _destroy was installed, so the handle was never cancelled. The hunks in newStreamReadableFromReadableStream and in the ReadableFromWeb constructor are the same as in #42584. The PR that lands second drops them.

The errored stream. addAbortSignal(signal, webStream) errors a stream in place. A NativePending stream has no controller, so only m_state changes. Since #43699, nativePtrForJS() hides the handle of an errored stream, so fromWeb reads it through a reader. getReader() then ran materializeNativeSource, which started the handle. A file source opened its file: getReader() threw ENOENT for a missing file. A Bun.spawn pipe stopped its background drain and read only on pull, so the child blocked. for await, tee() and pipeTo() take the same path. Release 1.4.3 has the getReader() stall too.

  • The Rust callers of ReadableStream::error (Body.rs:1361, Response.rs:152) cancel the native source in the same call (ReadableStream.rs:295). None depends on a later start.
  • A closed NativePending stream is either detached (ReadableStream__closeConsumedAsBody) or locked (ReadableStream__lockNative), so it does not start its source today. The same early return covers it.

The predicate. hasPendingNativeSource() has no state check. nativePtrForJS() hides the handle of an errored stream, and cancel() and the body fast paths leave NativePending before they close the stream. The lock checks make it correct for a stream that ReadableStream__lockNative locked, which stays NativePending.

Not changed.

  • child_process.ts calls constructNativeReadable on the stdio stream of its own subprocess, which nothing has started. The debug assertion there now fires when the stream has a handle and the claim fails.
  • The Rust takers of a native handle (to_any_blob, the body extractors, native sinks) read the raw handle slot and keep their own is_disturbed || is_locked checks.

Left to other changes.

  • The reader path takes its reader in the first _read(), so the web stream stays unlocked until then. Main does that for every stream with no native handle, and a started native stream now shares it. #43713 takes the reader inside fromWeb. An invalid signal or highWaterMark throws with the stream unlocked, as on main.
  • A web stream that is errored after fromWeb claimed its handle still delivers its data, as on main. #42584 makes the native Readable watch the stream.
  • An errored native stream does not release its source. A never-started one keeps the background drain of Bun.spawn, so the parent buffers the child's output (maxBuffer bounds it when set). One that was started before the error stalls the child, as on main. A release at the time of the error covers both cases. It changes webStreamControllerError for every native stream, so it is a separate change. A cancel inside materializeNativeSource would only run when a consumer touches the stream after the error.

Claim order. Considered a claim before the constructor: an invalid highWaterMark then spends the stream and needs a cancel. With the claim after the constructor, the web stream is untouched when the constructor throws.

Related open PRs on the same lines. #42584 (closes or errors the web stream when a native fromWeb ends), #43713 (stacked on it), #43748, #43732, #44590.

Node v26.3.0. fromWeb reads each option once. The web stream is locked when the getters of the signal run. An aborted signal fails the Readable with an AbortError and cancels the source with that error. The new tests assert the same.

Review. Self-reviewed: 8 concerns raised on the first version, 8 addressed. Review comments on this PR: cancel the claimed handle when the constructor throws (nothing is claimed at that point now, and a signal that throws destroys the Readable), an aborted signal that destroys the Readable inside the constructor (fixed by the signal order), long comments (each is one line now). On the reworked version: a responseBody finding that its author withdrew, and three optional findings that the section above answers.

Fail-before. With the src/ of main (5c6d4e24cd), all 24 new cases fail. A file after releaseLock() gives 786432 of 1048576 bytes. A blob after a partial read gives 16384. A spawn stdout never ends. A cancelled file stream yields 65536 bytes. getReader() on the errored missing file throws ENOENT. The child of the errored stdout blocks until the test times out.

Measurements. Release builds of main 5c6d4e24cd and of this branch: .bun_builtins 2,710,513 to 2,711,248 bytes (+735), .text 82,231,705 in both, file size 107,808,480 in both. perf and valgrind are not installed in the build container, so there are no instruction counts. Counts from the diff, per fromWeb call: a stream with no native handle is unchanged (one $bunNativePtr read). A fresh native stream does one $bunNativePtr read less. With a signal there is one validateAbortSignal call more. child_process stdio does one $bunNativePtr read less.

Other suites run on the debug build. The rest of node-stream.test.js, test/js/web/streams/streams.test.js, test/js/web/fetch/{body,fetch-abort-stream-body,body-clone,body-mixin-errors}.test.ts, process-stdin.test.ts, node-stream-uint8array.test.ts, node-fetch.test.js, undici.test.ts, spawn-streaming-stdout.test.ts, readablestream-helpers.test.ts, child_process.test.ts, and upstream test-stream-add-abort-signal, test-webstreams-abort-controller, test-whatwg-readablestream, test-filehandle-readablestream, test-stream-readable-aborted, test-readable-from-web-enqueue-then-close, test-stream-readable-from-web-termination, test-webstream-readable-from, test-whatwg-webstreams-adapters-to-{readablestream,streamduplex,readablewritablepair}, test-whatwg-webstreams-coverage, test-stream-readable-to-web, test-stream-duplex-from, test-child-process-{stdio,spawn-event,stdout-flush,stdout-flush-exit,stdio-big-write-end,exec-maxbuf,flush-stdio,stdio-inherit}, test-stdin-from-file-spawn.

Pre-existing in this container. child_process.test.ts "spawn still works with more than 10240 fds open" fails with or without the change: a debug build cannot load its bundled modules when file descriptors run out.


no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/stream/node-stream.test.js