fix(node): mirror webstreams Readable adapter cancel semantics #30526
Conversation
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
![gitstart-app[bot]](https://avatars.githubusercontent.com/in/46283?s=60&v=4){kind=small}
added 2 commits
August 26, 2025 05:57
…ms adapter Prevent uncaught TypeErrors when ReadableStream reader is canceled while the underlying Node.js stream is closing. Convert cancel() to async with proper Promise-based cleanup and add defensive error handling. - Make cancel() async and return Promise that resolves after destruction - Add isCanceled flag to prevent duplicate controller termination - Check isDestroyed() before processing to avoid invalid operations - Wrap controller operations in try-catch with debug logging - Remove data listener before destroy to prevent further callbacks - Add NODE_DEBUG=stream logging for terminated controller errors Fixes test-stream-readable-to-web-termination.js
- Add error handling for controller operations when stream is already closed - Prevent uncaught promise rejections during stream cancellation - Make cancel() async and properly wait for stream destruction - Add debug logging for swallowed errors in stream operations - Enable test-stream-readable-to-web-termination.js test Fixes parallel/test-stream-readable-to-web-termination.js by preventing race conditions between stream cancellation and controller operations.
bartlomieju
reviewed
Aug 26, 2025
| @@ -543,8 +593,43 @@ export function newReadableStreamFromStreamReadable( | |||
| streamReadable.resume(); | |||
| }, | |||
|
|
|||
| cancel(reason) { | |||
| destroy(streamReadable, reason); | |||
There was a problem hiding this comment.
It appears that Node.js, keeps track of wasCanceled variable here and then later uses it to decide if the controller should be closed. Have you tried this approach? https://github.com/nodejs/node/blob/886e4b3b534a9f3ad2facbc99097419e06615900/lib/internal/webstreams/adapters.js#L499
gitstart-app
bot
changed the title
node:stream tracking issue - parallel/test-stream-readable-to-web-termination.js
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR was created by GitStart to address the requirements from this ticket: DENB-29802.
Fix WebStreams adapter race condition when canceling during stream close
This PR fixes a race condition in the Node.js WebStreams adapter that causes uncaught TypeErrors when a ReadableStream's reader is canceled while the underlying
Node.js stream is closing.
Problem:
When reader.cancel() is called during the 'close' event of a Node.js stream (as in test-stream-readable-to-web-termination.js), the adapter would throw "Cannot
read properties of undefined" errors because the stream was being destroyed multiple times or the controller was in an invalid state.
Solution:
- Convert cancel() method to async with proper Promise-based cleanup
- Add isCanceled flag to prevent the finished callback from trying to close an already-canceled controller
- Check isDestroyed() before attempting stream operations
- Add defensive try-catch blocks around all controller operations
- Include proper cleanup with event listener removal before destruction
- Add NODE_DEBUG=stream logging for debugging
Test Fixed:
- test-stream-readable-to-web-termination.js - Now passes without errors
The changes ensure graceful handling of the race condition between stream cancellation and closure events.