fs: do not close FileHandle on stream destroy with autoClose: false

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.

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;
   }
 

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