fs: fix close listener leak in FileHandle streams by Y1D7NG · Pull Request #64227 · nodejs/node

Y1D7NG · 2026-07-01T09:23:38Z

When autoClose is false, FileHandle.createReadStream registers a close listener on the handle. That listener is not removed after the stream finishes normally, so creating streams repeatedly on the same handle accumulates listeners.

In importFd, when autoClose is false, remove that listener and perform the corresponding unref cleanup when the stream ends (on end for read streams, on finish for write streams) or on error.

Fixes: nodejs#64214 Signed-off-by: y1d7ng <[email protected]>

atlowChemi · 2026-08-09T21:17:11Z

+      function cleanup() {
+        options.fd.removeListener('close', onclose);
+        options.fd[kUnref]();
+      }
+      stream.once('end', cleanup);
+      stream.once('finish', cleanup);
+      stream.once('error', cleanup);


@Y1D7NG should we removeListener for all 3 in cleanup, so we don't leave these hanging as well?

Suggested change

function cleanup() {

options.fd.removeListener('close', onclose);

options.fd[kUnref]();

}

stream.once('end', cleanup);

stream.once('finish', cleanup);

stream.once('error', cleanup);

function cleanup() {

options.fd.removeListener('close', onclose);

stream.removeListener('end', cleanup);

stream.removeListener('finish', cleanup);

stream.removeListener('error', cleanup);

options.fd[kUnref]();

}

stream.once('end', cleanup);

stream.once('finish', cleanup);

stream.once('error', cleanup);

From what I'm aware of for similar functions, it's common practice to (intentionally) leave these listeners dangling on the stream and rely on normal garbage collection to tidy it all up in the end.

There was some discussion on this stuff in #35452. In this case, the duplex stuff doesn't apply so I'd personally say it doesn't need the special handling that was eventually added there.

Thanks for bringing that, that's interesting. Unsure how to proceed then. @nodejs/streams is the current behavior as expected?

to be clear: in my comment I'm only talking about the events on the stream (end/finish/error) - removing the close listener from the file descriptor (i.e. the purpose of this PR) is definitely necessary.

There are 2 approvals and a green CI.

Could this PR be landed, or does it need clarification of the above comments first?

My comments are non-blocking, although I don't see it hurting waiting extra time to get insights from the right team tho.

I think the listeners can be left to normal garbage collection

After waiting 7 days there haven't been any further comments, or any blocking objections, so adding to commit queue.

nodejs-github-bot · 2026-08-10T11:56:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/75731/

ronag · 2026-08-18T14:55:12Z

This is broken and should be reverted. Will unref the handle multiple times.

MikeMcC399 · 2026-08-18T15:18:51Z

@ronag

This is broken and should be reverted. Will unref the handle multiple times.

That is very unfortunate 🙁 !

Would you want to initiate the revert? I'm not sure how best to handle this.

MikeMcC399 · 2026-08-18T15:55:33Z

I'll submit a revert PR, if the original approvers and @ronag would be so kind as to approve the revert as well.

ovflowd · 2026-08-18T15:58:59Z

I'll submit a revert PR, if the original approvers and @ronag would be so kind as to approve the revert as well.

I can approve. But for future reference, that's why we should wait for the subject matter experts to also approve. Even if that means the PR taking its due time to be merged.

ovflowd · 2026-08-18T15:59:15Z

we can also add unit tests for this specific case.

MikeMcC399 · 2026-08-18T16:03:21Z

But for future reference, that's why we should wait for the subject matter experts to also approve

My mistake for not waiting long enough. Very sorry!

atlowChemi · 2026-08-18T16:04:01Z

I'll submit a revert PR, if the original approvers and @ronag would be so kind as to approve the revert as well.

I can approve. But for future reference, that's why we should wait for the subject matter experts to also approve. Even if that means the PR taking its due time to be merged.

In all fairness, it waited 7 days as required 🙂

MikeMcC399 · 2026-08-19T06:07:42Z

Revert PR is #65387, landed in 882a781

This reverts commit 8488e13. It was advised that the fix in question is broken since it will unref the handle multiple times. Signed-off-by: Mike McCready <[email protected]> PR-URL: #65387 Refs: #64227 Refs: #64214 Reviewed-By: Claudio Wunder <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

ovflowd · 2026-08-19T07:36:29Z

I'll submit a revert PR, if the original approvers and @ronag would be so kind as to approve the revert as well.

I can approve. But for future reference, that's why we should wait for the subject matter experts to also approve. Even if that means the PR taking its due time to be merged.

In all fairness, it waited 7 days as required 🙂

Yeah, not assigning blame. After all I also approved the PR. But I feel that for such reasons waiting for more time till we get full confirmation could be helpful.

Fixes: #64214 Signed-off-by: y1d7ng <[email protected]> PR-URL: #64227 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Claudio Wunder <[email protected]>

This reverts commit 8488e13. It was advised that the fix in question is broken since it will unref the handle multiple times. Signed-off-by: Mike McCready <[email protected]> PR-URL: #65387 Refs: #64227 Refs: #64214 Reviewed-By: Claudio Wunder <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Fixes: #64214 Signed-off-by: y1d7ng <[email protected]> PR-URL: #64227 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Claudio Wunder <[email protected]>

This reverts commit 8488e13. It was advised that the fix in question is broken since it will unref the handle multiple times. Signed-off-by: Mike McCready <[email protected]> PR-URL: #65387 Refs: #64227 Refs: #64214 Reviewed-By: Claudio Wunder <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Fixes: #64214 Signed-off-by: y1d7ng <[email protected]> PR-URL: #64227 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Claudio Wunder <[email protected]>

This reverts commit 8488e13. It was advised that the fix in question is broken since it will unref the handle multiple times. Signed-off-by: Mike McCready <[email protected]> PR-URL: #65387 Refs: #64227 Refs: #64214 Reviewed-By: Claudio Wunder <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

MikeMcC399 · 2026-09-07T09:37:33Z

Should be removed from #65873

Fixes: #64214 Signed-off-by: y1d7ng <[email protected]> PR-URL: #64227 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Claudio Wunder <[email protected]>

This reverts commit 8488e13. It was advised that the fix in question is broken since it will unref the handle multiple times. Signed-off-by: Mike McCready <[email protected]> PR-URL: #65387 Refs: #64227 Refs: #64214 Reviewed-By: Claudio Wunder <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

s/node/pull/64229 Signed-off-by: Rasad Regmi <[email protected]> Title: fs: fix FileHandle close listener/ref leak in streams Description: `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`. `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 (nodejs#64227) to fs: fix FileHandle close listener/ref leak in streams 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]> Manual Git Steps (for you to run) cd "/home/rasadregmi/Desktop/Open Source Contributions Repos/node" git checkout -b fix-filehandle-stream-close-listener-leak main git add lib/internal/fs/streams.js test/parallel/test-fs-promises-file-hand git commit -s git push fork fix-filehandle-stream-close-listener-leak Commit Message fs: fix FileHandle close listener/ref leak in streams 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]>

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]>

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 Jul 1, 2026

Y1D7NG force-pushed the fix-stream-leak branch 5 times, most recently from b433fb3 to c4a165a Compare July 1, 2026 11:11

fs: fix close listener leak in FileHandle streams

96cae83

Fixes: nodejs#64214 Signed-off-by: y1d7ng <[email protected]>

Y1D7NG force-pushed the fix-stream-leak branch from c4a165a to 96cae83 Compare July 1, 2026 11:18

davidje13 reviewed Jul 1, 2026

View reviewed changes

Comment thread test/parallel/test-fs-promises-file-handle-stream.js Outdated

fs: fix close listener leak in FileHandle streams

8d811ea

Fixes: nodejs#64214 Signed-off-by: y1d7ng <[email protected]>

Y1D7NG requested a review from davidje13 July 1, 2026 16:33

MikeMcC399 added the review wanted PRs that need review. label Jul 29, 2026

atlowChemi approved these changes Aug 9, 2026

View reviewed changes

MikeMcC399 added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 9, 2026

ovflowd reviewed Aug 9, 2026

View reviewed changes

Comment thread lib/internal/fs/streams.js

atlowChemi reviewed Aug 9, 2026

View reviewed changes

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 9, 2026

This comment was marked as outdated.

Sign in to view

ovflowd approved these changes Aug 9, 2026

View reviewed changes

This comment was marked as outdated.

Sign in to view

MikeMcC399 removed the review wanted PRs that need review. label Aug 10, 2026

This comment was marked as outdated.

Sign in to view

This was referenced Aug 12, 2026

Flag cleanup callbacks that leave sibling EventEmitter listeners attached adversarylabs/nodejs-adversary#5

Closed

Detect incomplete EventEmitter lifecycle cleanup adversarylabs/nodejs-adversary#6

Merged

MikeMcC399 mentioned this pull request Aug 17, 2026

fs: remove FileHandle stream close listener #64229

Closed

MikeMcC399 mentioned this pull request Aug 18, 2026

Revert "fs: fix close listener leak in FileHandle streams" #65387

Merged

MikeMcC399 mentioned this pull request Aug 18, 2026

Close listener leak in fs/promises createReadStream #64214

Open

Paraspandey-debugs mentioned this pull request Aug 20, 2026

test: add validation for listener removal in read streams #65421

Open

aduh95 pushed a commit that referenced this pull request Aug 25, 2026

fs: fix close listener leak in FileHandle streams

a6271eb

Fixes: #64214 Signed-off-by: y1d7ng <[email protected]> PR-URL: #64227 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Claudio Wunder <[email protected]>

aduh95 pushed a commit that referenced this pull request Aug 25, 2026

fs: fix close listener leak in FileHandle streams

8873b91

Fixes: #64214 Signed-off-by: y1d7ng <[email protected]> PR-URL: #64227 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Claudio Wunder <[email protected]>

github-actions Bot mentioned this pull request Aug 25, 2026

2026-08-26, Version 26.8.0 (Current) #65551

Merged

aduh95 pushed a commit that referenced this pull request Aug 27, 2026

fs: fix close listener leak in FileHandle streams

071d46d

Fixes: #64214 Signed-off-by: y1d7ng <[email protected]> PR-URL: #64227 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Claudio Wunder <[email protected]>

aduh95 added dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. labels Sep 5, 2026

github-actions Bot mentioned this pull request Sep 7, 2026

2026-09-08, Version 24.21.0 'Krypton' (LTS) #65873

Merged

aduh95 pushed a commit that referenced this pull request Sep 8, 2026

fs: fix close listener leak in FileHandle streams

996f636

Fixes: #64214 Signed-off-by: y1d7ng <[email protected]> PR-URL: #64227 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Claudio Wunder <[email protected]>

rasadregmi mentioned this pull request Sep 17, 2026

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

Open

Uh oh!

Conversation

Y1D7NG commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Aug 10, 2026

Uh oh!

ronag commented Aug 18, 2026

Uh oh!

MikeMcC399 commented Aug 18, 2026

Uh oh!

MikeMcC399 commented Aug 18, 2026

Uh oh!

ovflowd commented Aug 18, 2026

Uh oh!

ovflowd commented Aug 18, 2026

Uh oh!

MikeMcC399 commented Aug 18, 2026

Uh oh!

atlowChemi commented Aug 18, 2026

Uh oh!

MikeMcC399 commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ovflowd commented Aug 19, 2026

Uh oh!

MikeMcC399 commented Sep 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Y1D7NG commented Jul 1, 2026 •

edited

Loading

MikeMcC399 commented Aug 19, 2026 •

edited

Loading