Skip to content

fs: fix close listener leak in FileHandle streams #64227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Y1D7NG wants to merge 2 commits into
base: main
Choose a base branch
from
+52 −1
Open

fs: fix close listener leak in FileHandle streams #64227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96cae83
fs: fix close listener leak in FileHandle streams
Y1D7NG Jul 1, 2026
8d811ea
fs: fix close listener leak in FileHandle streams
Y1D7NG Jul 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion lib/internal/fs/streams.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,19 @@ function importFd(stream, options) {
stream[kHandle] = options.fd;
stream[kFs] = FileHandleOperations(stream[kHandle]);
stream[kHandle][kRef]();
options.fd.on('close', FunctionPrototypeBind(stream.close, stream));

const onclose = FunctionPrototypeBind(stream.close, stream);
options.fd.on('close', onclose);
if (options.autoClose === false) {
function cleanup() {
options.fd.removeListener('close', onclose);
Comment thread
ovflowd marked this conversation as resolved.
options.fd[kUnref]();
}
stream.once('end', cleanup);
stream.once('finish', cleanup);
stream.once('error', cleanup);
Comment on lines +159 to +165

@atlowChemi atlowChemi Aug 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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);

@davidje13 davidje13 Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ovflowd ovflowd Aug 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@davidje13 davidje13 Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@MikeMcC399 MikeMcC399 Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 approvals and a green CI.

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

@ovflowd ovflowd Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Y1D7NG Y1D7NG Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the listeners can be left to normal garbage collection

}

return options.fd.fd;
}

Expand Down
39 changes: 39 additions & 0 deletions test/parallel/test-fs-promises-file-handle-stream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,46 @@ async function validateRead() {
);
}

async function validateReusedCreateReadStream() {
const filePath = path.resolve(tmpDir, 'tmp-reused-stream.txt');
fs.writeFileSync(filePath, Buffer.from('ab', 'utf8'));

const fileHandle = await open(filePath, 'r');
try {
await buffer(fileHandle.createReadStream({
start: 0,
end: 0,
autoClose: false,
}));
assert.strictEqual(fileHandle.listenerCount('close'), 0);

await buffer(fileHandle.createReadStream({
start: 1,
end: 1,
autoClose: false,
}));
assert.strictEqual(fileHandle.listenerCount('close'), 0);
} finally {
await fileHandle.close();
}
}

async function validateReusedCreateWriteStream() {
const filePath = path.resolve(tmpDir, 'tmp-reused-write-stream.txt');
const fileHandle = await open(filePath, 'w');
try {
const stream = fileHandle.createWriteStream({ autoClose: false });
stream.end('a');
await finished(stream);
assert.strictEqual(fileHandle.listenerCount('close'), 0);
} finally {
await fileHandle.close();
}
}

Promise.all([
validateWrite(),
validateRead(),
validateReusedCreateReadStream(),
validateReusedCreateWriteStream(),
]).then(common.mustCall());
Loading