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

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

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

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

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