Skip to content

fs: fix FileHandle close listener/ref leak in streams - #66080

Open
rasadregmi wants to merge 1 commit into
nodejs:mainfrom
rasadregmi:fix-filehandle-stream-close-listener-leak
Open

rasadregmi wants to merge 1 commit into
nodejs:mainfrom
rasadregmi:fix-filehandle-stream-close-listener-leak

Conversation

@rasadregmi

Copy link
Copy Markdown

Summary

fileHandle.createReadStream()/createWriteStream() with autoClose: false leaks a 'close' listener (and an internal
reference) on the FileHandle every time the stream finishes on its own, because autoClose: false also disables autoDestroy, so the stream never reaches _destroy(), the only place that previously released them. Enough such streams on one long-lived handle trips MaxListenersExceededWarning.

Details

importFd() in lib/internal/fs/streams.js now builds a single idempotent releaseHandleRef shared between FileHandleOperations .close() (the normal destroy path, unchanged for the default autoClose: true behavior) and a finished()-based safety net registered after the stream's readable/writable state is initialized, for streams that finish without ever calling destroy(). finished() correctly resolves on 'end'/'finish' alone when the stream won't emit 'close' (see willEmitClose() in internal/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

  • Reproduced the original issue on main and confirmed it's fixed: running the reported repro (ranged autoClose: false reads in a loop past 10 iterations) no longer triggers MaxListenersExceededWarning; the handle's 'close' listener
    count stays at 0.
  • test/parallel/test-fs-promises-file-handle-stream.js: added regression tests for repeated autoClose: false read/write
    streams releasing their listener, an explicit stream.close() after natural completion not double-releasing the handle's
    reference count, and confirmed the default autoClose: true behavior (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

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run. labels Sep 17, 2026
@github-actions

Copy link
Copy Markdown
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 <regmirasad53@gmail.com>
@rasadregmi
rasadregmi force-pushed the fix-filehandle-stream-close-listener-leak branch from 5397a5b to 456d8c4 Compare September 17, 2026 09:37
@codecov

codecov Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.26%. Comparing base (67e66b8) to head (456d8c4).
⚠️ Report is 365 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66080      +/-   ##
==========================================
- Coverage   90.27%   90.26%   -0.01%     
==========================================
  Files         789      789              
  Lines      271473   271653     +180     
  Branches    51808    51850      +42     
==========================================
+ Hits       245066   245205     +139     
- Misses      16880    16919      +39     
- Partials     9527     9529       +2     
Files with missing lines Coverage Δ
lib/internal/fs/streams.js 93.44% <100.00%> (+0.33%) ⬆️

... and 37 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Close listener leak in fs/promises createReadStream

2 participants