Skip to content

Release the pipe source when validation fails after locking - #7498

Merged
jasnell merged 1 commit into
mainfrom
jasnell/ts-streams-pipethrough-lock-leak
Sep 24, 2026
Merged

jasnell merged 1 commit into
mainfrom
jasnell/ts-streams-pipethrough-lock-leak

Conversation

@jasnell

@jasnell jasnell commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

pipeThrough() took the source's reader before acquiring the writer, and nothing released it if that threw. A non-WritableStream writable, an option getter that locks the destination, or a shadowed locked left the source locked, and the first threw "Cannot read private member #writer". pipeTo() leaked the same way through an option getter.

Both methods now brand-check the destination and convert the options (one shared convertPipeOptions) before the locked checks, as WebIDL does, and check locks internally. pipeToInternal also releases the reader if acquiring the writer throws.

Parity with C++. Behind the experimental typescript_implemented_streams flag.

@jasnell
jasnell requested review from guybedford and npaun September 24, 2026 16:51
@jasnell
jasnell requested review from a team as code owners September 24, 2026 16:51
@ask-bonk

ask-bonk Bot commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

LGTM

github run

@guybedford guybedford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Verified the new tests fail on main with the leaked #writer TypeError and pass here across all four piping configs; //src/wpt:streams-ts@ still passes (throwing-options getter order and pipe-through readable-before-writable are unaffected by the reordering).

Comment thread src/per_isolate/webstreams/readable.ts
Comment thread src/per_isolate/webstreams/readable.ts
@jasnell
jasnell enabled auto-merge (squash) September 24, 2026 19:00
pipeThrough() took the source's reader before acquiring the writer, and
nothing released it if that threw. A non-WritableStream writable, an
option getter that locks the destination, or a shadowed `locked` left
the source locked, and the first threw "Cannot read private member

Both methods now brand-check the destination and convert the options
(one shared convertPipeOptions) before the locked checks, as WebIDL
does, and check locks internally. pipeToInternal also releases the
reader if acquiring the writer throws.

Parity with C++. Behind the experimental typescript_implemented_streams
flag.
@jasnell
jasnell force-pushed the jasnell/ts-streams-pipethrough-lock-leak branch from 4ec30cb to a8db103 Compare September 24, 2026 22:23
@jasnell
jasnell merged commit 5d1832a into main Sep 24, 2026
23 checks passed
@jasnell
jasnell deleted the jasnell/ts-streams-pipethrough-lock-leak branch September 24, 2026 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants