Rework node.js stream utilities (readAsync, sliceStream, readSubstream, Splice)

[AArnott]

Resurrects this long-stale PR: merged with main and reworked to fix the hazards that made it break consumer scenarios.

What went wrong originally

I reproduced two silent data/liveness bugs in the original implementation:

1. readAsync left streams permanently paused. It read via flowing mode (stream.resume() + once('data') + stream.pause()). Node tracks readableFlowing as null (never flowed) / true / false (explicitly paused), and .on('data') only auto-resumes when it is not false. So after a single readAsync call, the stream was stuck at false and any consumer that subsequently attached a data handler received nothing, forever:

   initial readableFlowing: null
   read: <Buffer 01 02 03>
   readableFlowing after readAsync: false
   data received by later .on('data') consumer: null   // <-- silent hang
   

   This is almost certainly what broke @lifengl's scenarios. It also threw 'Stream must not be in flowing mode.' synchronously from a promise-returning function.

2. unshift of the unconsumed remainder could run after end. sliceStream/getBufferFrom read a whole chunk and unshifted what they didn't need. Node drains its nextTick queue before promise microtasks, so when the read was satisfied by an already-ended stream, the end event fired before the await resumed — and the unshift then either silently dropped the remainder or raised ERR_STREAM_UNSHIFT_AFTER_END_EVENT.

What this version does instead

• readAsync uses the readable event (paused mode). Node restores the prior flowing state when the last readable handler is removed, so it has no lasting effect on the stream. No flowing-mode check needed.
• Reads are capped by readableLength so no more bytes are ever consumed than were requested. unshift is gone entirely.
• A synchronous fast path avoids resuming on a later microtask when data is already buffered.
• readSubstream streams each length-prefixed chunk incrementally instead of buffering it whole (the original goal of the PR).
• Errors thrown inside the async _read of sliceStream/readSubstream now destroy() the stream instead of becoming an unhandled promise rejection that hangs the reader forever.
• In-flight reads are cancelled when sliceStream/readSubstream is destroyed, so a discarded wrapper no longer steals data from the underlying stream.
• FullDuplexStream.Splice returns a real Duplex rather than one whose read and on members were monkey-patched onto the source stream, so unshift, readableLength, readableEnded, off, pipe, async iteration etc. all work. It pulls from the source on demand (rather than push-pumping via data), which preserves back-pressure, avoids eagerly draining the source, and keeps data that is already available synchronously readable. Errors and premature close from both underlying streams are forwarded.
• No new dependency: the plexer package the original PR pulled in is not used.

Behavior change to be aware of

Events raised on a spliced duplex are now its own events instead of the source stream's. Concretely, finish is now raised on the stream that was ended rather than on its peer. FullDuplexStream.spec.ts was updated accordingly.

Testing

Test count went from 120 to 184, including regression tests for both hazards above (flowing-state preservation, no over-consumption), plus coverage of pipe, async iteration, back-pressure, cancellation, listener leaks, error propagation, premature close, destroyed-wrapper cleanup, truncated substreams, and vscode-jsonrpc over a spliced duplex.

All tests pass, including the .NET interop suite (v1/v2/v3), and eslint is clean.

[AArnott]

@lifengl This is what I would still like to merge (eventually), once we figure out what causes your scenarios to fail with it and add tests to demonstrate it within the repo.

[Copilot]

Pull request overview

This PR reworks the Node.js stream helper utilities to avoid over-consumption and flowing-mode hazards, improves substream/slice streaming behavior, and updates FullDuplexStream.Splice to return a proper Duplex that preserves backpressure and standard stream semantics.

Changes:

• Reimplements readAsync/buffer reads to use paused-mode (readable) and cap consumption to requested bytes; updates getBufferFrom accordingly.
• Reworks sliceStream and readSubstream to stream incrementally, add cancellation on destroy, and avoid unshift/post-end hazards.
• Replaces FullDuplexStream.Splice monkey-patching with a real pull-based Duplex and expands test coverage for these behaviors.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/nerdbank-streams/src/Utilities.ts	New readAsync + capped read helper; updated sliceStream/readSubstream and getBufferFrom to avoid over-consumption and improve cancellation/error handling.
src/nerdbank-streams/src/tests/Utilities.spec.ts	Adds focused regression and behavior tests for readAsync, sliceStream, and getBufferFrom.
src/nerdbank-streams/src/tests/Substream.spec.ts	Adds streaming/interop/cancellation/error-propagation tests for readSubstream.
src/nerdbank-streams/src/tests/FullDuplexStream.spec.ts	Updates finish-event expectations and adds extensive coverage for Splice semantics (backpressure, async iteration, pipe, unshift, interop).
src/nerdbank-streams/src/index.ts	Exports readAsync and sliceStream from the package entrypoint.
src/nerdbank-streams/src/FullDuplexStream.ts	Implements pull-based Splice that returns a real Duplex and forwards end/close/error appropriately.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.