Destroy Readable.fromWeb on a chunk it cannot push - #7344
Open
jasnell wants to merge 11 commits into
Open
Conversation
A web chunk that cannot become a Buffer (a view over a detached ArrayBuffer) made push() throw inside the read's fulfillment handler, where the throw was lost as an unobserved rejection and _read() was never called again: the Readable hung, neither ending nor erroring (Node's adapter has the same shape). The push is now guarded and the Readable destroyed with the conversion's TypeError, which also cancels the web stream. The same guard is applied to Duplex.fromWeb's readable half. No compatibility flag: a hang becomes an error.
When the node writable had been ended by the caller before writer.close(), close() resolved at once — before the writable had finished, so a slow or failing _final() went unreported to the writer — and cleared the controller, which the end-of-stream callback then dereferenced: every such sequence threw an uncaught TypeError (Node's adapter has the same shape). close() now settles with the node side's finish or error in every case, calling end() only if the caller has not. No compatibility flag: an uncaught TypeError becomes the intended settlement.
The fromWeb adapters register their closed-promise handlers with the live Promise.prototype.then after acquiring the reader or writer; a patched then that throws escaped the constructor with the web stream locked to a reader or writer nobody would ever hold. The lock is now released before the throw propagates — after the never-returned node stream has been destroyed quietly, with the web side marked closed for the adapter's cleanup, as Duplex.fromWeb() does for its pair: a then that registers the handlers before throwing has them run, and releasing the lock rejects the closed promise, whose handler would otherwise destroy that stream with the release TypeError — an 'error' nobody could listen to, escaping after the caller had caught the construction failure. The suite's new then-pollution module pins this, and that a transparent patch or an Object.prototype.then getter leaves the data intact.
The C++ streams implementation's controller does not keep its stream alive, and a toWeb adapter's node side — kept alive by its own pending I/O — holds only the controller. A full GC could therefore collect the web stream while only a pending writer.closed or write() continuation held it, which then never settled, and a Readable.toWeb source whose stream had been collected kept pushing into a controller that dropped every chunk and never signalled backpressure. Each adapter now keeps the web stream alive from the node stream, for exactly as long as it lives. The C++ behavior itself — a controller held while its stream is collected drops enqueues silently and keeps desiredSize at the high-water mark, where the TypeScript implementation counts them — is pinned as readable ledger #19 in src/tests/streams.
The legacy-stream branch chained the 'end' and 'close' subscriptions (`stream.on(...).on(...)`), so a writable-shaped object whose on() does not return itself made Writable.toWeb() and finished() throw a TypeError. Node calls on() twice; do the same.
The adapter's 'data' listener enqueued into the web stream unguarded. An enqueue that fails — a user strategy's size() throwing or returning an invalid size, or the stream having just been cancelled from an earlier 'data' listener — had already errored the stream (or found it closed) and then escaped the source's 'data' emission as an uncaught exception, once per chunk the source kept pushing. Catch it and destroy the source with the error: the stream reports the failure to its reader, the source to its listeners, once. A source the cancel already destroyed is left alone. Under the C++ streams implementation a failing size() is swallowed by enqueue (streams readable ledger #8/#9), so nothing is thrown and the source stays paused; the node:stream suite ledgers the difference.
Readable.toWeb() paused the source and attached its end-of-stream bridge and 'data' listener before constructing the ReadableStream; Writable.toWeb() attached its bridge and 'drain' listener before constructing the WritableStream. Those constructors reject an invalid high-water mark, which a stream misreporting its readableHighWaterMark/writableHighWaterMark supplies, so the throw left the node stream polluted: a paused source whose next 'data' met an undefined controller, a writable whose finish ran the bridge into one. Construct the web stream first (as Node's Readable.toWeb now does) and subscribe afterwards; the first pull() is a microtask away in any case.
…toWeb adapters Readable.toWeb: a source destroying itself from inside the pulled _read() surfaces as the premature-close AbortError; an 'error' a legacy source emits after ending is swallowed. Writable.toWeb: destroy(err) from inside _write rejects the in-flight write and writer.closed once; writer.abort() from inside _write lets the in-flight write resolve before erroring; a web write of a number or plain object errors the stream with ERR_INVALID_ARG_TYPE and leaves the node writable intact, as in Node.
Writable.fromWeb hands chunks to the sink by reference (SAB and WebAssembly.Memory views included; a transferring sink detaches the caller's view; a detached view is refused synchronously). Duplex.fromWeb: a bare destroy() from 'data' with a write in flight cancels the readable at once and aborts the writable after the sink settles that write; end() again from 'finish' reports ERR_STREAM_ALREADY_FINISHED, and end(chunk) a write after end.
A SharedArrayBuffer-backed chunk delivered by reference, empty chunks skipped, a resizable chunk aliasing its buffer until delivery, and a 'data' listener's throw erroring the Readable rather than being lost in the web read's promise.
Ten thousand one-byte pushes through Readable.toWeb as a Response body, alternating 1 B and 64 KiB chunks through Writable.fromWeb, an 8 MiB chunk each way, and ten thousand objectMode chunks through a web source and pipeline(): every byte and object accounted for, in order.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
More streams tests and fixes