fs: fix FileHandle close listener/ref leak in streams - #66080
Open
rasadregmi wants to merge 1 commit into
Open
rasadregmi wants to merge 1 commit into
rasadregmi wants to merge 1 commit into
Conversation
Contributor
|
Welcome to Node.js, and thank you for your first contribution! Before review, please take a moment to read:
Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal. |
createReadStream()/createWriteStream({ autoClose: false }) created
from a FileHandle attached a 'close' listener and took a reference
on the handle in importFd(), but only released either when the
stream went through _destroy(). Since autoClose: false also
disables autoDestroy, a stream that finishes on its own never
reaches _destroy(), so the listener and reference leaked. Creating
enough such streams on a long-lived handle (e.g. repeated ranged
reads) eventually tripped MaxListenersExceededWarning.
A prior fix (fixed in 64227, reverted in 65387) released the
reference again on the stream's 'end'/'finish'/'error' independent
of the destroy path, which could unref the handle twice if the
stream was later also explicitly closed/destroyed - a normal thing
to do after a stream naturally ends.
This introduces a single idempotent release function shared by both
paths (the normal destroy path and a finished()-based safety net for
autoClose: false streams that never reach _destroy()), so the
reference and listener are released exactly once regardless of how
the stream ends.
Fixes: nodejs#64214
Refs: nodejs#64227
Refs: nodejs#65387
Refs: nodejs#64229
Signed-off-by: Rasad Regmi <[email protected]>
rasadregmi
force-pushed
the
fix-filehandle-stream-close-listener-leak
branch
from
September 17, 2026 09:37
5397a5b to
456d8c4
Compare
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
fileHandle.createReadStream()/createWriteStream()withautoClose: falseleaks a'close'listener (and an internalreference) on the
FileHandleevery time the stream finishes on its own, becauseautoClose: falsealso disablesautoDestroy, so the stream never reaches_destroy(), the only place that previously released them. Enough such streams on one long-lived handle tripsMaxListenersExceededWarning.Details
importFd()inlib/internal/fs/streams.jsnow builds a single idempotentreleaseHandleRefshared betweenFileHandleOperations .close()(the normal destroy path, unchanged for the defaultautoClose: truebehavior) and afinished()-based safety net registered after the stream's readable/writable state is initialized, for streams that finish without ever callingdestroy().finished()correctly resolves on'end'/'finish'alone when the stream won't emit'close'(seewillEmitClose()ininternal/streams/utils.js), and the shared idempotency guard means it's safe even if a stream is also explicitly closed after finishing on its own the exact scenario that caused a prior fix (#64227) to be reverted in #65387 (it unreffed the handle twice).Testing
mainand confirmed it's fixed: running the reported repro (rangedautoClose: falsereads in a loop past 10 iterations) no longer triggersMaxListenersExceededWarning; the handle's'close'listenercount stays at 0.
test/parallel/test-fs-promises-file-handle-stream.js: added regression tests for repeatedautoClose: falseread/writestreams releasing their listener, an explicit
stream.close()after natural completion not double-releasing the handle'sreference count, and confirmed the default
autoClose: truebehavior (implicitly closing the handle) is unaffected.python3 tools/test.py --mode=release parallel/test-fs-*: all 281 tests pass.python3 tools/test.py --mode=release parallel/test-stream-*plus the fs-stream and events suites: all 277 tests pass.make lint-js-ci: 0 failures.Related Issue
Fixes #64214