From 946019e5723581b80427f4296baaee4de0548dea 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                                 | 12 ++--
 lib/internal/fs/promises.js                   | 66 +++++++++----------
 .../test-filehandle-readablestream.js         | 64 ++++--------------
 3 files changed, 47 insertions(+), 95 deletions(-)

diff --git a/doc/api/fs.md b/doc/api/fs.md
index ab9977dfca70d4..04abb9654f015d 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -480,11 +480,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([options])`
+#### `filehandle.readableWebStream()`
 
 <!-- YAML
 added: v17.0.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/55461
+    description: Removed option to create a 'bytes' stream. Streams are now always 'bytes' streams.
   - version:
     - v20.0.0
     - v18.17.0
@@ -494,13 +497,10 @@ 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.
+Returns a byte-oriented `ReadableStream` that may be used to read the file's
+contents.
 
 An error will be thrown if this method is called more than once or is called
 after the `FileHandle` is closed or closing.
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index f5a9a3a854263b..88bd5615c4928d 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -276,12 +276,9 @@ class FileHandle extends EventEmitter {
   /**
    * @typedef {import('../webstreams/readablestream').ReadableStream
    * } ReadableStream
-   * @param {{
-   *   type?: string;
-   *   }} [options]
    * @returns {ReadableStream}
    */
-  readableWebStream(options = kEmptyObject) {
+  readableWebStream(options = { type: 'bytes' }) {
     if (this[kFd] === -1)
       throw new ERR_INVALID_STATE('The FileHandle is closed');
     if (this[kClosePromise])
@@ -293,46 +290,43 @@ class FileHandle extends EventEmitter {
     if (options.type !== undefined) {
       validateString(options.type, 'options.type');
     }
+    if (options.type !== 'bytes') {
+      process.emitWarning(
+        'A non-"bytes" type option to readableWebStream() no longer does ' +
+        'anything. readableWebStream() always creates a byte-oriented stream ' +
+        'now.',
+        'ExperimentalWarning',
+      );
+    }
 
-    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..cae4a9f12687a0 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 = '';
@@ -114,59 +114,17 @@ 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.
+// Make sure a warning is logged if a non-'bytes' type is passed.
 (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',
+  common.expectWarning({
+    ExperimentalWarning: [
+      'A non-"bytes" type option to readableWebStream() no longer does ' +
+      'anything. readableWebStream() always creates a byte-oriented stream ' +
+      'now.',
+    ],
   });
-  mc.port1.close();
+  const stream = file.readableWebStream({ type: 'foobar' });
+  await stream.cancel();
   await file.close();
 })().then(common.mustCall());