fs: fix close listener leak in FileHandle streams

[Y1D7NG]

fix: #64214

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.

[atlowChemi]

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

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]

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

[davidje13]

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]

There are 2 approvals and a green CI.

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

[ovflowd]

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

[Y1D7NG]

I think the listeners can be left to normal garbage collection

[MikeMcC399]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 8488e13

[ronag]

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

[MikeMcC399]

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

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

[ovflowd]

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]

• we can also add unit tests for this specific case.

[MikeMcC399]

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]

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 🙂