From fe93fc3571783617f10a7f3e99041b85dd55b53f Mon Sep 17 00:00:00 2001 From: Sahinur Date: Mon, 12 Oct 2026 02:50:06 +0600 Subject: [PATCH] fs: do not close FileHandle on stream destroy with autoClose: false When a ReadStream or WriteStream is created from a FileHandle, the stream takes a reference on the handle. Destroying the stream (for example by breaking out of a `for await` loop) dropped that reference and then unconditionally called `filehandle.close()`, which also dropped the caller's reference and closed the handle, even when the stream was created with `autoClose: false`. Only release the stream's own reference when `autoClose` is false, so that the FileHandle stays usable and remains owned by the caller, as documented. Also remove the stream's 'close' listener from the FileHandle when the stream closes, so that creating many streams from a long-lived FileHandle does not leak listeners and streams. Fixes: https://github.com/nodejs/node/issues/45721 Refs: https://github.com/nodejs/node/pull/45909 Signed-off-by: Sahinur --- doc/api/fs.md | 14 +-- lib/internal/fs/streams.js | 17 +++- ...t-fs-stream-file-handle-autoclose-false.js | 92 +++++++++++++++++++ 3 files changed, 113 insertions(+), 10 deletions(-) create mode 100644 test/parallel/test-fs-stream-file-handle-autoclose-false.js diff --git a/doc/api/fs.md b/doc/api/fs.md index e327a330a142..3327c1c6cbcf 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -304,10 +304,10 @@ setTimeout(() => { ``` If `autoClose` is false, then the file descriptor won't be closed, even if -there's an error. It is the application's responsibility to close it and make -sure there's no file descriptor leak. If `autoClose` is set to true (default -behavior), on `'error'` or `'end'` the file descriptor will be closed -automatically. +there's an error or the stream is destroyed. It is the application's +responsibility to close it and make sure there's no file descriptor leak. If +`autoClose` is set to true (default behavior), on `'error'` or `'end'` the file +descriptor will be closed automatically. An example to read the last 10 bytes of a file which is 100 bytes long: @@ -352,9 +352,9 @@ the default `r`. The `encoding` can be any one of those accepted by {Buffer}. If `autoClose` is set to true (default behavior) on `'error'` or `'finish'` the file descriptor will be closed automatically. If `autoClose` is false, -then the file descriptor won't be closed, even if there's an error. -It is the application's responsibility to close it and make sure there's no -file descriptor leak. +then the file descriptor won't be closed, even if there's an error or the +stream is destroyed. It is the application's responsibility to close it and +make sure there's no file descriptor leak. By default, the stream will emit a `'close'` event after it has been destroyed. Set the `emitClose` option to `false` to change this behavior. diff --git a/lib/internal/fs/streams.js b/lib/internal/fs/streams.js index c8c86b59640a..7fe00dc74498 100644 --- a/lib/internal/fs/streams.js +++ b/lib/internal/fs/streams.js @@ -90,13 +90,23 @@ function _construct(callback) { } // This generates an fs operations structure for a FileHandle -const FileHandleOperations = (handle) => { +const FileHandleOperations = (stream, handle, onHandleClose) => { return { open: (path, flags, mode, cb) => { throw new ERR_METHOD_NOT_IMPLEMENTED('open()'); }, close: (fd, cb) => { + // The stream no longer needs to react to the FileHandle closing, and + // must not be kept alive by a long-lived FileHandle. + handle.off('close', onHandleClose); + // Release the reference taken in importFd(). handle[kUnref](); + if (!stream.autoClose) { + // With autoClose: false the FileHandle is owned by the caller, so + // destroying the stream must not close it. + process.nextTick(cb); + return; + } PromisePrototypeThen(handle.close(), () => cb(), cb); }, @@ -155,10 +165,11 @@ function importFd(stream, options) { // FileHandle is not supported with custom fs operations throw new ERR_METHOD_NOT_IMPLEMENTED('FileHandle with fs'); } + const onHandleClose = FunctionPrototypeBind(stream.close, stream); stream[kHandle] = options.fd; - stream[kFs] = FileHandleOperations(stream[kHandle]); + stream[kFs] = FileHandleOperations(stream, stream[kHandle], onHandleClose); stream[kHandle][kRef](); - options.fd.on('close', FunctionPrototypeBind(stream.close, stream)); + options.fd.on('close', onHandleClose); return options.fd.fd; } diff --git a/test/parallel/test-fs-stream-file-handle-autoclose-false.js b/test/parallel/test-fs-stream-file-handle-autoclose-false.js new file mode 100644 index 000000000000..75f701343758 --- /dev/null +++ b/test/parallel/test-fs-stream-file-handle-autoclose-false.js @@ -0,0 +1,92 @@ +'use strict'; + +// Regression test for https://github.com/nodejs/node/issues/45721 +// Destroying a stream created from a FileHandle with `autoClose: false` must +// not close the FileHandle, and must not leave a 'close' listener behind on +// the FileHandle. + +const common = require('../common'); +const assert = require('assert'); +const fs = require('fs'); +const tmpdir = require('../common/tmpdir'); + +tmpdir.refresh(); +const file = tmpdir.resolve('stream-file-handle-autoclose-false.txt'); +const input = 'x'.repeat(1024); +fs.writeFileSync(file, input); + +(async () => { + // Partially consuming a ReadStream with `break` destroys it. + const handle = await fs.promises.open(file, 'r'); + handle.on('close', common.mustCall()); + const listeners = handle.listenerCount('close'); + + // Run more iterations than the default maxListeners (10) to make sure + // no listener leaks onto the long-lived FileHandle. + process.on('warning', common.mustNotCall()); + for (let i = 0; i < 15; i++) { + const stream = handle.createReadStream({ + autoClose: false, + start: 0, + highWaterMark: 16, + }); + for await (const chunk of stream) { + assert.strictEqual(chunk.toString(), input.slice(0, 16)); + break; + } + assert.strictEqual(stream.destroyed, true); + // The stream is only closed once any pending read has completed. + if (!stream.closed) { + await new Promise((resolve) => stream.once('close', resolve)); + } + assert.notStrictEqual(handle.fd, -1); + assert.strictEqual(handle.listenerCount('close'), listeners); + } + + // Explicitly destroying the stream. + const stream = handle.createReadStream({ autoClose: false, start: 0 }); + stream.destroy(); + await new Promise((resolve) => stream.on('close', resolve)); + assert.notStrictEqual(handle.fd, -1); + + // The FileHandle is still usable. + const { bytesRead } = await handle.read(Buffer.alloc(4), 0, 4, 0); + assert.strictEqual(bytesRead, 4); + await handle.close(); + assert.strictEqual(handle.fd, -1); +})().then(common.mustCall()); + +(async () => { + // Same for WriteStream. + const handle = await fs.promises.open(tmpdir.resolve('ws.txt'), 'w'); + handle.on('close', common.mustCall()); + const stream = handle.createWriteStream({ autoClose: false }); + stream.write('hello'); + stream.destroy(); + await new Promise((resolve) => stream.on('close', resolve)); + assert.notStrictEqual(handle.fd, -1); + assert.strictEqual(handle.listenerCount('close'), 1); + await handle.write('world'); + await handle.close(); +})().then(common.mustCall()); + +(async () => { + // Closing the FileHandle while a stream with `autoClose: false` holds a + // reference to it still closes the stream and the FileHandle. + const handle = await fs.promises.open(file, 'r'); + const stream = handle.createReadStream({ autoClose: false }); + stream.on('close', common.mustCall()); + await handle.close(); + assert.strictEqual(handle.fd, -1); +})().then(common.mustCall()); + +(async () => { + // `autoClose: true` (the default) still closes the FileHandle on destroy. + const handle = await fs.promises.open(file, 'r'); + handle.on('close', common.mustCall()); + const stream = handle.createReadStream(); + stream.destroy(); + await new Promise((resolve) => stream.on('close', resolve)); + assert.strictEqual(handle.fd, -1); + assert.strictEqual(handle.listenerCount('close'), 1); +})().then(common.mustCall());