Close a native stream at the handoff of its data - #7502
Conversation
A native-backed stream whose source was extracted (a native-to-native pipe, the C++ bridge's pumpTo) or detached (new Request(request), socket upgrades), the source of a native tee, and a teed queued branch stayed readable forever with their controllers. The Node.js interop closed-promise never settled, so finished() never fired. The error hook on a detached native body reached the moved source and broke the detached stream with an internal TypeError. These streams now close at the handoff and drop their controller, as the legacy C++ streams leave them. The closed-promise settles then, and the interop error hook, which acts only on a readable stream, can no longer reach the moved source. A queued detached stream is unchanged: it stays the controller's stream, closing and erroring with the source. C++ has no interop hooks. Behind the experimental typescript_implemented_streams flag.
|
LGTM |
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
LGTM. Checked out and verified locally: stream-ts@, stream-ts@all-compat-flags, stream-cpp@, sockets-ts@, sockets-ts@all-compat-flags, sockets-cpp@ all pass, and with readable.ts reverted to main both new tests fail, so they pin the fix. Legacy parity checks out too (ReadableStreamInternalController::removeSource leaves Locked + disturbed + Closed; ReadableStreamJsController::tee closes the teed stream as well).
One stale comment not in the diff: the [kControllerErrorFunction] doc comment (readable.ts ~L4609-4612) still says a branch that has itself been teed "stays what tee() left it: a permanently locked, inert shell". It is now closed at tee, so the hook returns at the #state !== 'readable' check; worth updating to match AGENTS.md.
The Windows CI failure (connect-neuter-test@ / @all-compat-flags plus two timeouts) looks unrelated: that test runs with nodejs_compat_v2 only, without the TS streams flag, and depends on a 10ms wait. A rerun should clear it.
A branch that has itself been teed is now closed at the tee, so the comments on #consumer and on the interop error hook no longer describe it as an inert shell. The native arm of detachReadableStream no longer calls neutralize(): closeReadableStreamHusk already drops the consumer, so the arm marks the stream disturbed and locks it directly, as the native tee arm does. No behavior change.
2 participants
A native-backed stream whose source was extracted (a native-to-native pipe, the C++ bridge's pumpTo) or detached (new Request(request), socket upgrades), the source of a native tee, and a teed queued branch stayed readable forever with their controllers. The Node.js interop closed-promise never settled, so finished() never fired. The error hook on a detached native body reached the moved source and broke the detached stream with an internal TypeError.
These streams now close at the handoff and drop their controller, as the legacy C++ streams leave them. The closed-promise settles then, and the interop error hook, which acts only on a readable stream, can no longer reach the moved source. A queued detached stream is unchanged: it stays the controller's stream, closing and erroring with the source.
C++ has no interop hooks. Behind the experimental
typescript_implemented_streams flag.