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