fs: add autoClose option to FileHandle readableWebStream

By default, the `readableWebStream` method of `FileHandle` returns
a ReadableStream that, when finished, does not close the underlying
FileHandle. This can lead to issues if the stream is consumed
without having a reference to the FileHandle to close after use.
This commit adds an `autoClose` option to the `readableWebStream`
method, which, when set to `true`, will automatically close the
FileHandle when the stream is finished or canceled.

The test modified in this commit demonstrates one of the cases where
this is necessary in that the stream is consumed by separate code than
the FileHandle which was being left to close the underlying fd when
it is garbage collected, which is a deprecated behavior.

PR-URL: #58548
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

Author: jasnell

doc/api/fs.md

@@ -476,11 +476,14 @@ Reads data from the file and stores that in the given buffer.
 If the file is not modified concurrently, the end-of-file is reached when the
 number of bytes read is zero.
 
-#### `filehandle.readableWebStream()`
+#### `filehandle.readableWebStream([options])`
 
 <!-- YAML
 added: v17.0.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/58548
+    description: Added the `autoClose` option.
   - version: v24.0.0
     pr-url: https://github.com/nodejs/node/pull/57513
     description: Marking the API stable.
@@ -496,6 +499,9 @@ changes:
     description: Added option to create a 'bytes' stream.
 -->
 
+* `options` {Object}
+  * `autoClose` {boolean} When true, causes the {FileHandle} to be closed when the
+    stream is closed. **Default:** `false`
 * Returns: {ReadableStream}
 
 Returns a byte-oriented `ReadableStream` that may be used to read the file's

lib/internal/fs/promises.js

@@ -86,7 +86,6 @@ const {
   validateInteger,
   validateObject,
   validateOneOf,
-  validateString,
   kValidateObjectAllowNullable,
 } = require('internal/validators');
 const pathModule = require('path');
@@ -279,9 +278,10 @@ class FileHandle extends EventEmitter {
   /**
    * @typedef {import('../webstreams/readablestream').ReadableStream
    * } ReadableStream
+   * @param {{ type?: 'bytes', autoClose?: boolean }} [options]
    * @returns {ReadableStream}
    */
-  readableWebStream(options = { __proto__: null, type: 'bytes' }) {
+  readableWebStream(options = kEmptyObject) {
     if (this[kFd] === -1)
       throw new ERR_INVALID_STATE('The FileHandle is closed');
     if (this[kClosePromise])
@@ -290,20 +290,27 @@ class FileHandle extends EventEmitter {
       throw new ERR_INVALID_STATE('The FileHandle is locked');
     this[kLocked] = true;
 
-    if (options.type !== undefined) {
-      validateString(options.type, 'options.type');
-    }
-    if (options.type !== 'bytes') {
+    validateObject(options, 'options');
+    const {
+      type = 'bytes',
+      autoClose = false,
+    } = options;
+
+    validateBoolean(autoClose, 'options.autoClose');
+
+    if (type !== 'bytes') {
       process.emitWarning(
         'A non-"bytes" options.type has no effect. A byte-oriented steam is ' +
         'always created.',
         'ExperimentalWarning',
       );
     }
 
-
     const readFn = FunctionPrototypeBind(this.read, this);
-    const ondone = FunctionPrototypeBind(this[kUnref], this);
+    const ondone = async () => {
+      this[kUnref]();
+      if (autoClose) await this.close();
+    };
 
     const ReadableStream = lazyReadableStream();
     const readable = new ReadableStream({
@@ -315,15 +322,15 @@ class FileHandle extends EventEmitter {
         const { bytesRead } = await readFn(view, view.byteOffset, view.byteLength);
 
         if (bytesRead === 0) {
-          ondone();
           controller.close();
+          await ondone();
         }
 
         controller.byobRequest.respond(bytesRead);
       },
 
-      cancel() {
-        ondone();
+      async cancel() {
+        await ondone();
       },
     });
 

test/es-module/test-wasm-web-api.js

@@ -106,7 +106,9 @@ function testCompileStreamingRejectionUsingFetch(responseCallback, rejection) {
   // Response whose body is a ReadableStream instead of calling fetch().
   await testCompileStreamingSuccess(async () => {
     const handle = await fs.open(fixtures.path('simple.wasm'));
-    const stream = handle.readableWebStream();
+    // We set the autoClose option to true so that the file handle is closed
+    // automatically when the stream is completed or canceled.
+    const stream = handle.readableWebStream({ autoClose: true });
     return Promise.resolve(new Response(stream, {
       status: 200,
       headers: { 'Content-Type': 'application/wasm' }

test/parallel/test-filehandle-autoclose.mjs

@@ -0,0 +1,30 @@
+import '../common/index.mjs';
+import { open } from 'node:fs/promises';
+import { rejects } from 'node:assert';
+
+{
+  const fh = await open(new URL(import.meta.url));
+
+  // TODO: remove autoClose option when it becomes default
+  const readableStream = fh.readableWebStream({ autoClose: true });
+
+  // Consume the stream
+  await new Response(readableStream).text();
+
+  // If reading the FileHandle after the stream is consumed fails,
+  // then we assume the autoClose option worked as expected.
+  await rejects(fh.read(), { code: 'EBADF' });
+}
+
+{
+  await using fh = await open(new URL(import.meta.url));
+
+  const readableStream = fh.readableWebStream({ autoClose: false });
+
+  // Consume the stream
+  await new Response(readableStream).text();
+
+  // Filehandle must be still open
+  await fh.read();
+  await fh.close();
+}