Repository navigation
Conversation
|
No actionable comments were generated in the recent review. ℹ️ Recent review info
WalkthroughPipeline pumps now pass teardown handlers to eligible web-readable sources. Held readers stop and cancel those sources when teardown or destination closure occurs. Tests cover cancellation, destination errors, callback settlement, and socket-backed pipelines. ChangesWeb-stream pipeline teardown
Suggested reviewers: Priority: Merge Risk:
|
|
Status: ready for review. Head e4b7ea8, which merges main 11c3671. No review thread is open.
|
There was a problem hiding this comment.
Beyond the inline findings, two further concerns in pumpToWeb were examined and ruled out: aborting a WritableStream sink under { end: false } after the signal fires (the PR's own grid lists this as an intended change, matching pipeTo()), and cancelling a held web source before reporting a locked-sink getWriter() error (the teardown would cancel it anyway via finishImpl, so the ordering only moves the cancel earlier).
Extended reasoning...
The change reworks pumpToNode/pumpToWeb in src/js/internal/streams/pipeline.ts to iterate a web source through a cancellable HeldReader, adds teardown callbacks, premature-close reporting, and a load-time-captured setImmediate; it touches no auth, crypto, or input-parsing surface. Seven confirmed findings are posted inline, including hang paths that survive the fix and user-overridable cancel()/writable lookups, so a human review is already signalled; this note only records the two candidates investigated and ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
a0b0c47 to
2d49e9c
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
The four new commits address what I raised on the previous version: cancel now goes through a $ReadableStreamDefaultReader rather than a subclass's cancel(), stopWhenGone is gated on isWritableStream, the onreported wait is bounded to end pipelines, a report on a torn pump stops the source, the compose() test asserts the AbortError, and the per-test timeouts are gone.
Beyond the inline nits, I also checked that throw error || torn at src/js/internal/streams/pipeline.ts:249 cannot throw undefined (every path that stops the source first sets error or torn), and that the locked-sink path in pumpToWeb awaits a cancel() that cannot reject (.catch(nop) plus the resolved fallback), so finish(err) is always reached. Only the inline nits remain from this pass.
Extended reasoning...
The change reworks the web-source pumps in src/js/internal/streams/pipeline.ts (about 190 lines of teardown logic: a lazily built HeldReader, destroys registration, close/eos wiring, and a closed-promise reaction for web sinks) plus two $-global type declarations and roughly 1,000 lines of new tests. It touches no auth, crypto or injection surface; the sensitive aspect is tamper-resistance of a built-in module, which the revised code handles by capturing intrinsics at load and cancelling through a reader. The prior round's seven inline findings are all addressed in the new commits except the deliberate load-time setImmediate capture, which now carries an explanatory comment. The remaining findings are two convention nits, and the async ordering of teardown versus destination reports is intricate enough that a maintainer familiar with node:stream should still look before merging.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
pipeline() with a ReadableStream or TransformStream member did not call back after an abort, or after the destination failed, while the web source had nothing to deliver. The pump waits in `for await` over the stream. return() on the stream's own iterator queues behind the pending next(), so nothing could wake that read, and the source was not cancelled. The pump now reads a member web source through a reader that the pipeline can cancel. The teardown of the pipeline, an error of the node destination and the end of a web sink stop the pump. They decide nothing else. The stopped pump fails on the path it takes when the next chunk meets a dead destination, so a pipeline that settled before settles with the same error. The cancel of a source that the pump is reading runs in the next turn of the event loop. An owner of the source that reacts to the same signal closes or feeds its stream first, as before. A pump that waits for 'drain' is left to the report of the destination, as before. A web stream that a function stage returned is not held. Node documents that the stage has to handle `signal`.
pipe() fails the pipeline with ERR_STREAM_PREMATURE_CLOSE when the
destination closes before the source has ended. A pump over a web
source did not. End-of-stream reports no error for a destination that
its owner ended, for an Http2ServerResponse whose client left, or for a
socket that reported its end before it closed. The pump stayed on the
idle source, or on a 'drain' that could not come. The pump now applies
the rule of pipe() on the 'close' event of the destination.
With `{ end: false }`, a pipeline whose destination was ended by its
owner resolved if the web source then ended with no other chunk. It now
rejects, as it does with a node source.
getWriter() threw inside the pump before its first await. The rejection was unhandled, the callback never ran, and no member was torn down. The pump now reports the error through the callback, after pipelineImpl has registered every member, so that finishImpl tears them all down.
A source of a ReadableStream subclass: the pump called the cancel() of the subclass when it had not locked the stream yet, and then `then` on what that returned. The cancel now goes through a reader in every path, as the stream's own iterator cancels. A stream that someone else has locked is left alone. `end: false` and a destination that its owner destroyed and that never reports: the stopped pump waited for that report. It waits only for a destination that the teardown of the pipeline destroys. A TransformStream whose `writable` is not a WritableStream: the watch for the end of the sink threw outside the pump's try. Only a native WritableStream is watched. A report of the destination with no error that comes after the teardown now stops the pump too. A getter of `Symbol.asyncIterator` that throws for the pump is reported by the loop, as before. Tests: no per-test timeouts, the compose() test asserts the error, and one test for each of the above.
The `#stream` field of the reader gets a default in the class body, as its other fields have. The wait for the report of the destination uses Promise.withResolvers() in place of a `new Promise` executor. No change of behaviour.
2d49e9c to
a68b520
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
The new push addresses the points I raised on the earlier revisions (cancel now goes through a $ReadableStreamDefaultReader rather than stream.cancel(), onfinished stops the source on any report once torn, the sink is brand-checked before stopWhenGone, Promise.withResolvers and the #stream default are in place, the compose() test asserts the AbortError). Beyond the inline nit, I also checked the { end: false } WritableStream sink paths: stopWhenGone only retains the HeldReader (whose stream and reader are nulled in release()) for the sink's lifetime, and the sink abort on teardown goes through the pre-existing catch block and matches the downside the description documents, so neither is a defect.
Extended reasoning...
The change reworks node:stream pipeline() in src/js/internal/streams/pipeline.ts so a pump over a plain WHATWG ReadableStream source can be cancelled on abort or destination death, adds two builtin declarations, and adds roughly 1060 lines of tests. It touches no auth, crypto or injection surface. The remaining inline finding is a prototype-tamper nit; the two candidate issues on the end:false WritableStream path were ruled out as bounded retention and documented behavior. The change is large and semantically subtle enough that a maintainer look is still worthwhile, so this is informational rather than an approval.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
…he prototype The pump that holds a web source called read(), cancel() and releaseLock() through ReadableStreamDefaultReader.prototype. A program that patched those methods ran its patch inside pipeline(). Before this branch the pump read through the stream's own iterator, which calls the internal operations. HeldReader now takes the three methods when the class is created and calls them with $call.
The pump of a held web source took read(), cancel() and releaseLock() from ReadableStreamDefaultReader.prototype when the class was created, the built-in iterator of ReadableStream.prototype at the first call, and setImmediate from the global at load. A spy or a fake timer that was in place at that moment stayed in the pump after the program restored it. HeldReader now calls the three host functions of the reader and the native setImmediate through $newCppFunction. hold() compares the iterator of a stream with the current one of ReadableStream.prototype and keeps nothing.
After a teardown with `end`, the pump cancelled its source in the next turn and then waited for the destination to report. A destination that never reports (a destroy() that does not call back, compose() with a web tail) then hung a pipeline that settles on main when the source fails or ends later. The pump now stays on the source until the destination reports, as on main, and the report stops it. The callback still comes after the close of the destination, and the destination's own error still wins. Also: - hold() compares with the built-in iterator through a private name on ReadableStream.prototype. A replacement of the prototype's iterator is called, as on main. - A pump that the teardown stopped does not wait for room in a WritableStream before it aborts it. - A WritableStream that closed before the call is not watched. - The watch on a WritableStream keeps no AsyncLocalStorage store. - stop() schedules one immediate.
src/js/internal/streams/pipeline.ts: main typed the file. `pumpToNode` and `pumpToWeb` keep main's typed signatures and take the optional `destroys` list of this branch. Four lines of the new code get the types that the type check asks for: `#cancelled` holds a `Promise<unknown>`, `destroys!.push` where a held source implies the list, and `finish(err as Error)`.
… of built-ins The build checks every `$newCppFunction` binding and every `$name` of src/js against the table of built-ins (src/codegen/builtins/host). - unseen-code.builtins.ts: the three reader functions and `functionSetImmediate` run no code that the compiler has not seen. - builtin-names.ts: `values` is a private name of JavaScriptCore. The pipeline reads it as `.$values`. - pipeline.ts: `ReadableStream` and `ReadableStreamDefaultReader` without `$`. The bundler turns both into the private globals, as in the other modules, so the two declarations in builtins.d.ts go.
|
A note for the next merge of main into this branch. Nothing is merged or changed here. Main added a build step in 8d5672c (#45631) that stops when
I did not run the step on this branch, because the branch conflicts with main now. #46906 hit it with two other functions of
|
There was a problem hiding this comment.
I reviewed the latest push (the merge of main and the built-ins table entries) and found no new bugs; given the size of the change to a hot-path node:stream builtin plus the C++ stream bindings, a human look is still worthwhile before landing.
What was reviewed:
- The hand-resolved merge in src/js/internal/streams/pipeline.ts: the only non-type changes are dropping
$fromReadableStreamDefaultReaderandReadableStream— src/codegen/replacements.ts lists both inglobalsToPrefix, so the bundler still emits the private@globals and the tamper-resistance is unchanged. - The HeldReader teardown paths in pumpToNode/pumpToWeb (stop/cancel/release idempotence,
tornpropagation intofinish, the$newCppFunctionbindings and their new unseen-code entries) — every exit I traced settles the pending wait and callsfinish. - The
$valuesprivate name in JSReadableStream.cpp and its registration in builtin-names.ts/builtins.d.ts — non-writable, non-deletable, same function object as[Symbol.asyncIterator].
Extended reasoning...
The change reworks how node:stream's pipeline() pumps a web ReadableStream into node or web destinations (about 200 lines in src/js/internal/streams/pipeline.ts), exposes three ReadableStreamDefaultReader host functions and a $values private name from the C++ stream bindings, updates the builtin name and unseen-code tables, and adds roughly 1,300 lines of tests. It touches no auth, crypto, injection or data-exposure surface. Approval is withheld because the logic is intricate teardown/cancellation state across two async pumps in a hot-path builtin, src/js/builtins.d.ts is covered by the *.d.ts CODEOWNERS entry, and the earlier inline threads from prior runs were resolved by the PR author only. Since the last review the only commits are a merge of main (verified behavior-preserving in pipeline.ts) and the table entries, and this run's hunt ran dry with no findings.
Problem
pipeline(upstream.body, res, cb)never calls back while the upstream is idle. An abort hangs the same way, as on Node v26.3.0.for await(pipeline.ts:129,:161on main).return()on that iterator queues behind the pendingnext().Fix
test/js/node/stream/node-stream.test.js(104 new tests, 62 fail on main), 271 vendored node tests.Background
pumpToNode,pumpToWeb) is the loop thatpipeline()runs for a web stream. Only the pump holds the lock and can cancel.Readable.fromWebaround web members (96 more cells per call). node:stream: pipeline() and compose() paths that never settle after an abort or a failed member #44770 tracks what still hangs.Downsides
{ end: false }resolved if the source ended.WritableStreamdestination is aborted, also with{ end: false }. Acancel()that never settles blocks the callback, as withpipeTo().require("node:stream"): +9 cells. Binary: +4,556 bytes. Noperfhere: instructions not measured.Notes
Merged with
mainat 8d5672c (#45631).pipeline.tsis typed there, sopumpToNodeandpumpToWebkeep the typed signatures ofmainand take thedestroyslist. The build ofmainchecks every$newCppFunctionbinding and every$nameofsrc/jsagainst the table of built-ins. The three reader functions andfunctionSetImmediateare insrc/codegen/builtins/host/unseen-code.builtins.tsnow, andvalues(read as.$values) is insrc/codegen/builtin-names.ts. The code spellsReadableStreamandReadableStreamDefaultReaderwithout$: the bundler turns both into the private globals.No issue exists for this. A fuzz run found it. Node v26.3.0 has the same two pumps (
lib/internal/streams/pipeline.js) and hangs in the same cells. Its documentation says thatstream.pipeline()callsstream.destroy(err)on every stream that has not ended, and calls the callback. cloudflare/workerd#7338 gives workerd'spipeline()the same pump-level teardown. One call avoids the hang on every runtime:Readable.fromWeb(body),body.pipeTo(Writable.toWeb(file), { signal }), or the samesignalonfetch().Repro
Node v26.3.0 and Bun main print
1 s later. This branch printssource cancelled,rejected: AbortError,1 s later.How it works
HeldReaderis the async iterable that the pump loops over in place of the stream. It keeps aReadableStreamDefaultReader, so the pipeline can cancel a read that is pending. Bothfor awaitloops keep their text.'close'of that destination before the source has ended (the rule thatpipe()has for node sources), and an error of a web sink (the stream's closed promise rejects).destroysper pump) stops the pump too. Withend, the teardown has destroyed the node destination, and the pump then stays on the source until that destination reports through end-of-stream, as on main. If the source ends or fails first, the pump goes main's way. The report stops the pump. So the callback comes after the'close'of the destination, the destination's own error wins over theAbortError(stream: preserve error over AbortError in pipeline nodejs/node#62113), and a destination that never reports its destroy leaves the pipeline as it is on main.pipeline()call. An owner of the source that reacts to the same signal within that turn closes, errors or feeds its stream first, as it does today.cancel()of the source where the pump had started to read, as the loop waits forreturn()today. Withend: falsethe pipeline does not destroy a node destination and does not wait for it.read(),cancel(),releaseLock()andsetImmediateare$newCppFunctionbindings to the host functions. The cancel goes through a reader, so acancel()that a subclass ofReadableStreamdefines is never called, as on main. A program that replaces those properties (a spy, fake timers), beforenode:streamloads or later, does not reach the pump. WPTstreams/readable-streams/patched-global.any.jsrequires the same of the stream's own iterator, which main's pump used.[Symbol.asyncIterator]is the built-in function.ReadableStream.prototypekeeps that function under a private name ($values) for this comparison. A subclass with an iterator of its own, a stream with an own iterator, and a program that replaced the iterator on the prototype are read through that iterator, as on main.WritableStreambefore it aborts it. A write in flight that ends only on the sink'scontroller.signalthen ends. Node and main abort the sink at once in that cell for a nodeReadable.signal.pipeline()with a web source. Nothing is read at load.What changes for a pipeline that settles today
One process per cell, on Node v26.3.0, on a release build of main (5900354) and on one of this branch. 3,298 cells: 58 kinds of web source (byte streams,
type: "direct",fetchandResponsebodies,Blob,Bun.file, tee branches, subclasses), node and web destinations (files, sockets,http.ServerResponse, zlib,compose(), legacy emitters, sinks that throw, reject or never report), destinations that are gone before the call, an abort or a dead destination, and what the owner of the source does in the next 0 to 5 turns. Node equals main in all but a few races.WritableStreamsink is aborted when the signal stops the pump (main wrote until the source ended, then closed the sink, or left it open and locked with{ end: false }).WritableStreamthat is locked, and the branch reports that error through the callback.TransformStreamand does not handle the promises of itswrite()calls. Those promises reject, withundefined, when the pump cancels the readable side. On main that happens when a chunk meets the dead destination.fetchbody that the server resets, into aWritableStreamwithhighWaterMark: 0. It is a race. Main does not call back either in 3 of 3 reruns.The callbacks that change:
ERR_STREAM_PREMATURE_CLOSE, as with a node source, and as on main when a chunk came next. On main the pump stayed on the source, so the result came from the source's next step: success with{ end: false }if it ended, its own error if it failed, alone or in anAggregateError.AggregateErrorwith theAbortError. The branch has cancelled the source and stopped the pump by then, and reports theAbortError.WritableStreamwhose controller errors with a falsy reason (controller.error()with no argument). The callback gets no error, as on main when a chunk meets that sink. On main a later error of the source was reported.More that a program can see:
cancel()and goes on callingcontroller.enqueue()from a timer: its first call after the cancel throws, and nothing catches it. Into a node destination that is so on main too, once a chunk meets the dead destination. Into aWritableStreamit is new, because main ignored the abort there.cancel()returns a promise that never settles blocks the callback. A tee branch, aResponsebody afterclone()andReadableStream.from()over a generator that waits are not in this class: their cancel settles when the source ends.WritableStreamdestination, with{ end: false }too: the stopped pump fails, and a pump that fails aborts its sink. Node and main do this for a nodeReadablewith{ end: false }. The sink stays locked in every case, on Node too.compose()with a web stream in front of a node stream now closes ondestroy(). It emits the'error'(AbortError) that acompose()of node streams emits.type: "direct"source that flushes every 10 KiB from a microtask loop arrives as 100 chunks, not 51, for the same 1,024,000 bytes.Bun.file().stream(), afetchbody andBlob.stream()keep their chunk counts and theirwrite()counts.Not changed, as on main and Node (#44770): a destination that finished and closed before the call, a
WritableStreamthat was closed before the call, a destination that never reports its destroy, and aWritableStreamwhose write in flight never settles.Measurements
Release builds of main (5900354) and of this branch (7c13a68), Linux x64. Cells: the sum of
heapStats().objectTypeCountsbefore and after a batch of pipelines with no collection in between, one fresh process per case. The branch has merged main 11c3671 since then. That merge adds types topipeline.tsand changes nothing that is measured here.Writable137 to 141, intoWritableStream64 to 70, with aTransformStreambetween 181 to 191,ReadableintoWritableStream131 to 132.Readable.fromWeb(web)intoWritableis 237 on both.Writable), 6 to 6 (WritableStream), 17 to 16 (through aTransformStream). A synchronouspull(): 3 to 4, 6 to 6, 17 to 16. An asyncpull(): 11 to 10, 12 to 13, 23 to 22. A chunk that arrives while the pump waits: 5 to 4, 8 to 7.Writable2 to 1, intoWritableStream3 to 2.ReadableintoWritable): cells per call and per chunk are equal.require("node:stream"): 3,629 to 3,638 live cells after a full collection.{ end: false }pipeline, aWritableStreamthat the caller keeps holds 13 cells, not 8. It keeps noAsyncLocalStoragestore of the call.sizeof the two binaries: text +4,556 bytes (106,753,900 to 106,758,456), data and bss equal. The bundledpipeline.jsis 9,950 and 14,246 bytes. Host functions: none added. Four$newCppFunctionbindings to host functions that exist, and one private property onReadableStream.prototype.bun build --compile --trim: no feature more is kept. For a program that only importsnode:stream,bun check --aot --explain=trimon this branch says that the web streams are kept for the native binding ofinternal/streams/native-readable. Main has that binding too.ReadableintoWritablepath, which the diff does not touch, moves by 14% between the builds in the same runs.perf,valgrind,bloatyandstraceare not installed in the container that ran this, so there are no instruction counts.Tests
test/js/node/stream/node-stream.test.js. On a release build of main 62 fail and 42 pass. The 42 are cells that node settles, pinned so that they stay as they are.test-stream*,test-webstreams*,test-whatwg-webstreams*, and every file that callspipeline(): 266 pass and the same 5 fail with main'spipeline.ts.wpt-streams.test.ts(1,175 tests),test/js/web/streams/streams.test.js(688 tests) andweb-stream-state.test.tspass.Review history
cancel(), aborted the sink from the teardown, and reported a dead sink by itself. Self-review found pipelines that settle today and changed: an error lost behind anAbortError, an early exit through aTextDecoderStreamthat became an error, an{ end: false }node destination that was destroyed. The version that opened this PR is the rework: the teardown only stops the pump. Not taken from that review: a tick-drain hook from child_process: deliver each stdio event after the nextTicks and promise jobs of the one before #43790 for the order of the cancel (the next turn of the event loop needs no hook), and the same stop for pumps with no web source (node:stream: pipeline() and compose() paths that never settle after an abort or a failed member #44770).cancel()returns no promise (the pump called it and thenthenon the result),end: falsewith a destination that never reports its destroy (the stopped pump waited for that report), and aTransformStreamwhosewritableis not aWritableStream(the watch on the sink threw outside the pump'stry). The fourth commit fixes the three and adds one test for each.hold()kept, and for asetImmediatethat a program replaced beforenode:streamloaded: two pipelines that settle on main then hung. The seventh commit keeps nothing from the program.destroy()that does not call back,compose()with a web tail) hung a pipeline that settles on main when the source fails later. A stopped pump waited for room in aWritableStreambefore it aborted it. The watch on aWritableStreamkept theAsyncLocalStoragestore of the call.hold()skipped an iterator that a program put onReadableStream.prototype. The eighth commit fixes the four.WritableStreamdestination with{ end: false }. Node v26.3.0 and main do the same when the source is a nodeReadable. Left out, and in node:stream: pipeline() and compose() paths that never settle after an abort or a failed member #44770: a destination that finished and closed before the call. Node hangs there too, and a check at the start of the pump would fail a pipeline that resolves today when the source ends with no chunk.Commits
'close'rule ofpipe()for a web source. It carries the change for a destination that is gone early, and theHttp2ServerResponsecase.#streamfield, andPromise.withResolvers()for the report wait.read,cancelandreleaseLocktaken when the class is created. Replaced by 7.read,cancel,releaseLockandsetImmediate.$valueson the prototype, the sink that is aborted with no wait for room, the watch that keeps no store, and the tests of the second self-review.valuesis a private name.ReadableStreamandReadableStreamDefaultReaderare written without$: the bundler turns both into the private globals.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