stream: clean up when pipeline throws synchronously - #65064
Open
shani-singh1 wants to merge 1 commit into
Open
Conversation
`pipelineImpl()` wires the streams together in a loop that can throw synchronously, for example `ERR_STREAM_UNABLE_TO_PIPE` when the destination is already closed or destroyed. The streams it has already adopted are registered in `destroys`, but `finishImpl()` is the only code that drains that list, disposes the `AbortSignal` listener and calls `ac.abort()`, and it never runs on this path. Those streams are therefore left undestroyed and their resources leak; for an `fs.ReadStream` source that is a leaked file descriptor. Wrap the loop so the same teardown runs before the error propagates. The error is still thrown, so the observable failure mode is unchanged. Fixes: nodejs#65063
Collaborator
|
Review requested:
|
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
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.
pipelineImpl()wires the streams together in a loop that can throw synchronously. The most common case isERR_STREAM_UNABLE_TO_PIPE, raised when the next stream is already closed or destroyed, which happens routinely when a destination goes away first (for examplepipeline(fs.createReadStream(file), res)after the HTTP client disconnected).Each stream the loop adopts registers a destroy function in
destroys.finishImpl()is the only code that drainsdestroys, disposes the listener added to the caller'sAbortSignaland callsac.abort(), and it never runs when the loop throws. Every stream already wired up is therefore left undestroyed and its resources leak. For anfs.ReadStreamsource that is a leaked file descriptor.The loop has six synchronous throw sites: one
ERR_STREAM_UNABLE_TO_PIPE, threeERR_INVALID_RETURN_VALUEand twoERR_INVALID_ARG_TYPE. This wraps the loop so the same teardown runs before the error propagates. The error is still thrown, so the observable failure mode is unchanged.Most of the diff is the re-indentation of the existing loop. Reviewing with
git diff -wshows the actual change is 13 lines.Before
After
The full reproduction is in the linked issue. The added test fails on
main(5 failing assertions) and passes with this change.I also checked the change against a set of ordinary
pipeline()usages (happy path,fsread to writable, asynchronous mid-stream error,ENOENTsource, async generator transform, abort via an outer signal) and the behaviour is identical before and after. The only observable differences are on the two synchronous-throw paths, where the streams are now destroyed.Fixes: #65063