stream: fix pipeTo assert when writer is released during microtask#63733
Open
colinaaa wants to merge 2 commits into
Open
stream: fix pipeTo assert when writer is released during microtask#63733colinaaa wants to merge 2 commits into
colinaaa wants to merge 2 commits into
Conversation
The deferred write introduced by commit 65aa8f6 ("stream: fix pipeTo to defer writes per WHATWG spec") wraps the write operation in queueMicrotask(). A race condition exists where the pipe can shutdown and release the writer between when [kChunk] schedules the microtask and when it executes, causing an ERR_INTERNAL_ASSERTION because writer[[stream]] is undefined. Fix by moving the shuttingDown flag into the shared pipe state object so that PipeToReadableStreamReadRequest can check it. The microtask now skips the write if the pipe has already begun shutting down. This aligns with the WHATWG Streams spec step 15 which states: "if shuttingDown becomes true, the user agent must not initiate further reads from reader, and must only perform writes of already-read chunks". Fixes: nodejs#63732 Signed-off-by: Qingyu Wang <wangqingyu.c0l1n@bytedance.com>
The state.shuttingDown guard introduced in the previous commit is too broad: it skips writes even when the destination is still writable. The WHATWG Streams spec requires that already-read chunks are written to a still-writable destination during shutdown (step 15, shutdown substeps 3.1-3.2). Replace the guard with a check for writer[kState].stream === undefined which is only true after finalize() has released the writer. This precisely targets the crash condition without violating the spec requirement. Restore shuttingDown as a closure-local variable since it no longer needs to be shared with PipeToReadableStreamReadRequest. Add a test case verifying that an already-read chunk is written when an AbortSignal fires after enqueue while the destination is still writable. Signed-off-by: Qingyu Wang <wangqingyu.c0l1n@bytedance.com>
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
2 participants
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.
Summary
The deferred write introduced by #61800 wraps the write operation in
queueMicrotask(). A race condition exists where the pipe can shutdown and release the writer between when[kChunk]schedules the microtask and when it executes, causing anERR_INTERNAL_ASSERTIONbecausewriter[[stream]]is undefined.Fix by checking
writer[kState].stream === undefinedin the deferred microtask before attempting the write. This condition is only true afterfinalize()has released the writer, precisely targeting the crash without interfering with the spec requirement that already-read chunks are written to a still-writable destination during shutdown.Fixes: #63732