From 57bfbb31217278d72520ba01a8b89226cbd2830c Mon Sep 17 00:00:00 2001
From: Ian Kerins <git@isk.haus>
Date: Sat, 19 Oct 2024 19:21:11 -0400
Subject: [PATCH] fs: make `FileHandle.readableWebStream` always create byte
 streams

The original implementation of the experimental
`FileHandle.readableWebStream` API created non-`type: 'bytes'` streams,
which prevented callers from creating `mode: 'byob'` readers from the
returned stream, which means they could not achieve the associated
"zero-copy" performance characteristics.

Then, #46933 added a parameter allowing callers to pass the `type`
parameter down to the ReadableStream constructor, exposing the same
semantics to callers of `FileHandle.readableWebStream`.

But there is no point to giving callers this choice: FileHandle-derived
streams are by their very nature byte streams. We should not require
callers to explicitly opt in to having byte stream semantics. Moreover,
I do not see a situation in which callers would ever want to have a
non-bytes stream: bytes-streams only do anything differently than normal
ones if `mode: 'byob'` is passed to `getReader`.

So, remove the `options` parameter and always create a ReadableStream
with `type: 'bytes'`.

Fixes #54041.
---
 doc/api/fs.md                                 |  6 +-
 lib/internal/fs/promises.js                   | 63 +++++++------------
 .../test-filehandle-readablestream.js         | 61 +-----------------
 3 files changed, 25 insertions(+), 105 deletions(-)

diff --git a/doc/api/fs.md b/doc/api/fs.md
index ab9977dfca70d4..eb117ec3b58ea0 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -480,7 +480,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([options])`
+#### `filehandle.readableWebStream()`
 
 <!-- YAML
 added: v17.0.0
@@ -494,10 +494,6 @@ changes:
 
 > Stability: 1 - Experimental
 
-* `options` {Object}
-  * `type` {string|undefined} Whether to open a normal or a `'bytes'` stream.
-    **Default:** `undefined`
-
 * Returns: {ReadableStream}
 
 Returns a `ReadableStream` that may be used to read the files data.
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index f5a9a3a854263b..1f8ce78eae8753 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -86,7 +86,6 @@ const {
   validateInteger,
   validateObject,
   validateOneOf,
-  validateString,
   kValidateObjectAllowNullable,
 } = require('internal/validators');
 const pathModule = require('path');
@@ -276,12 +275,9 @@ class FileHandle extends EventEmitter {
   /**
    * @typedef {import('../webstreams/readablestream').ReadableStream
    * } ReadableStream
-   * @param {{
-   *   type?: string;
-   *   }} [options]
    * @returns {ReadableStream}
    */
-  readableWebStream(options = kEmptyObject) {
+  readableWebStream() {
     if (this[kFd] === -1)
       throw new ERR_INVALID_STATE('The FileHandle is closed');
     if (this[kClosePromise])
@@ -290,49 +286,34 @@ 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');
-    }
-
-    let readable;
+    const {
+      ReadableStream,
+    } = require('internal/webstreams/readablestream');
 
-    if (options.type !== 'bytes') {
-      const {
-        newReadableStreamFromStreamBase,
-      } = require('internal/webstreams/adapters');
-      readable = newReadableStreamFromStreamBase(
-        this[kHandle],
-        undefined,
-        { ondone: () => this[kUnref]() });
-    } else {
-      const {
-        ReadableStream,
-      } = require('internal/webstreams/readablestream');
+    const readFn = FunctionPrototypeBind(this.read, this);
+    const ondone = FunctionPrototypeBind(this[kUnref], this);
 
-      const readFn = FunctionPrototypeBind(this.read, this);
-      const ondone = FunctionPrototypeBind(this[kUnref], this);
+    const readable = new ReadableStream({
+      type: 'bytes',
+      autoAllocateChunkSize: 16384,
 
-      readable = new ReadableStream({
-        type: 'bytes',
-        autoAllocateChunkSize: 16384,
+      async pull(controller) {
+        const view = controller.byobRequest.view;
+        const { bytesRead } = await readFn(view, view.byteOffset, view.byteLength);
 
-        async pull(controller) {
-          const view = controller.byobRequest.view;
-          const { bytesRead } = await readFn(view, view.byteOffset, view.byteLength);
+        if (bytesRead === 0) {
+          ondone();
+          controller.close();
+        }
 
-          if (bytesRead === 0) {
-            ondone();
-            controller.close();
-          }
+        controller.byobRequest.respond(bytesRead);
+      },
 
-          controller.byobRequest.respond(bytesRead);
-        },
+      cancel() {
+        ondone();
+      },
+    });
 
-        cancel() {
-          ondone();
-        },
-      });
-    }
 
     const {
       readableStreamCancel,
diff --git a/test/parallel/test-filehandle-readablestream.js b/test/parallel/test-filehandle-readablestream.js
index 70cd1814b2fe1b..a314654497d675 100644
--- a/test/parallel/test-filehandle-readablestream.js
+++ b/test/parallel/test-filehandle-readablestream.js
@@ -87,11 +87,11 @@ const check = readFileSync(__filename, { encoding: 'utf8' });
   await file.close();
 })().then(common.mustCall());
 
-// Make sure 'bytes' stream works
+// Make sure 'byob' reader works
 (async () => {
   const file = await open(__filename);
   const dec = new TextDecoder();
-  const readable = file.readableWebStream({ type: 'bytes' });
+  const readable = file.readableWebStream();
   const reader = readable.getReader({ mode: 'byob' });
 
   let data = '';
@@ -113,60 +113,3 @@ const check = readFileSync(__filename, { encoding: 'utf8' });
 
   await file.close();
 })().then(common.mustCall());
-
-// Make sure that acquiring a ReadableStream 'bytes' stream
-// fails if the FileHandle is already closed.
-(async () => {
-  const file = await open(__filename);
-  await file.close();
-
-  assert.throws(() => file.readableWebStream({ type: 'bytes' }), {
-    code: 'ERR_INVALID_STATE',
-  });
-})().then(common.mustCall());
-
-// Make sure that acquiring a ReadableStream 'bytes' stream
-// fails if the FileHandle is already closing.
-(async () => {
-  const file = await open(__filename);
-  file.close();
-
-  assert.throws(() => file.readableWebStream({ type: 'bytes' }), {
-    code: 'ERR_INVALID_STATE',
-  });
-})().then(common.mustCall());
-
-// Make sure the 'bytes' ReadableStream is closed when the underlying
-// FileHandle is closed.
-(async () => {
-  const file = await open(__filename);
-  const readable = file.readableWebStream({ type: 'bytes' });
-  const reader = readable.getReader({ mode: 'byob' });
-  file.close();
-  await reader.closed;
-})().then(common.mustCall());
-
-// Make sure the 'bytes' ReadableStream is closed when the underlying
-// FileHandle is closed.
-(async () => {
-  const file = await open(__filename);
-  const readable = file.readableWebStream({ type: 'bytes' });
-  file.close();
-  const reader = readable.getReader({ mode: 'byob' });
-  await reader.closed;
-})().then(common.mustCall());
-
-// Make sure that the FileHandle is properly marked "in use"
-// when a 'bytes' ReadableStream has been acquired for it.
-(async () => {
-  const file = await open(__filename);
-  file.readableWebStream({ type: 'bytes' });
-  const mc = new MessageChannel();
-  mc.port1.onmessage = common.mustNotCall();
-  assert.throws(() => mc.port2.postMessage(file, [file]), {
-    code: 25,
-    name: 'DataCloneError',
-  });
-  mc.port1.close();
-  await file.close();
-})().then(common.mustCall());