From b9586bf898ceffa7fd1ead0885c4909f2faf5b76 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 1 Jun 2025 08:31:04 -0700 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/58548 Reviewed-By: LiviaMedeiros Reviewed-By: Benjamin Gruenbaum --- doc/api/fs.md | 8 +++++- lib/internal/fs/promises.js | 29 +++++++++++++-------- test/es-module/test-wasm-web-api.js | 4 ++- test/parallel/test-filehandle-autoclose.mjs | 17 ++++++++++++ 4 files changed, 45 insertions(+), 13 deletions(-) create mode 100644 test/parallel/test-filehandle-autoclose.mjs diff --git a/doc/api/fs.md b/doc/api/fs.md index 8681d5c6606..091db4c5879 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -476,7 +476,7 @@ 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])` +* `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 diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 80e1aa42a87..e43fb2a3daa 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -84,7 +84,6 @@ const { validateEncoding, validateInteger, validateObject, - validateString, kValidateObjectAllowNullable, } = require('internal/validators'); const pathModule = require('path'); @@ -278,9 +277,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]) @@ -289,10 +289,15 @@ 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.', @@ -300,9 +305,11 @@ class FileHandle extends EventEmitter { ); } - 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({ @@ -314,15 +321,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(); }, }); diff --git a/test/es-module/test-wasm-web-api.js b/test/es-module/test-wasm-web-api.js index b199393a18c..879748e4403 100644 --- a/test/es-module/test-wasm-web-api.js +++ b/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' } diff --git a/test/parallel/test-filehandle-autoclose.mjs b/test/parallel/test-filehandle-autoclose.mjs new file mode 100644 index 00000000000..cc8733f3ef4 --- /dev/null +++ b/test/parallel/test-filehandle-autoclose.mjs @@ -0,0 +1,17 @@ +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' }); +}