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.
PR-URL: #55461
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Author: isker

doc/api/fs.md

@@ -477,11 +477,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
@@ -491,13 +494,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.

lib/internal/fs/promises.js

@@ -142,6 +142,10 @@ function lazyFsStreams() {
 
 constlazyRimRaf=getLazy(()=>require('internal/fs/rimraf').rimrafPromises);
 
+constlazyReadableStream=getLazy(()=>
+require('internal/webstreams/readablestream').ReadableStream,
+);
+
 // By the time the C++ land creates an error for a promise rejection (likely from a
 // libuv callback), there is already no JS frames on the stack. So we need to
 // wait until V8 resumes execution back to JS land before we have enough information
@@ -276,12 +280,9 @@ class FileHandle extends EventEmitter {
 /**
  * @typedef {import('../webstreams/readablestream').ReadableStream
  * } ReadableStream
- * @param {{
- * type?: string;
- * }} [options]
  * @returns {ReadableStream}
  */
-readableWebStream(options=kEmptyObject){
+readableWebStream(options={__proto__: null,type: 'bytes'}){
 if(this[kFd]===-1)
 thrownewERR_INVALID_STATE('The FileHandle is closed');
 if(this[kClosePromise])
@@ -293,46 +294,40 @@ class FileHandle extends EventEmitter {
 if(options.type!==undefined){
 validateString(options.type,'options.type');
 }
+if(options.type!=='bytes'){
+process.emitWarning(
+'A non-"bytes" options.type has no effect. A byte-oriented steam is '+
+'always created.',
+'ExperimentalWarning',
+);
+}
 
-letreadable;
 
-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');
+constreadFn=FunctionPrototypeBind(this.read,this);
+constondone=FunctionPrototypeBind(this[kUnref],this);
 
-constreadFn=FunctionPrototypeBind(this.read,this);
-constondone=FunctionPrototypeBind(this[kUnref],this);
+constReadableStream=lazyReadableStream();
+constreadable=newReadableStream({
+type: 'bytes',
+autoAllocateChunkSize: 16384,
 
-readable=newReadableStream({
-type: 'bytes',
-autoAllocateChunkSize: 16384,
+asyncpull(controller){
+constview=controller.byobRequest.view;
+const{ bytesRead }=awaitreadFn(view,view.byteOffset,view.byteLength);
 
-asyncpull(controller){
-constview=controller.byobRequest.view;
-const{ bytesRead }=awaitreadFn(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,

test/parallel/test-filehandle-readablestream.js

@@ -87,11 +87,11 @@ const check = readFileSync(__filename, { encoding: 'utf8' });
 awaitfile.close();
 })().then(common.mustCall());
 
-// Make sure 'bytes' stream works
+// Make sure 'byob' reader works
 (async()=>{
 constfile=awaitopen(__filename);
 constdec=newTextDecoder();
-constreadable=file.readableWebStream({type: 'bytes'});
+constreadable=file.readableWebStream();
 constreader=readable.getReader({mode: 'byob'});
 
 letdata='';
@@ -114,59 +114,16 @@ const check = readFileSync(__filename, { encoding: 'utf8' });
 awaitfile.close();
 })().then(common.mustCall());
 
-// Make sure that acquiring a ReadableStream 'bytes' stream
-// fails if the FileHandle is already closed.
-(async()=>{
-constfile=awaitopen(__filename);
-awaitfile.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()=>{
-constfile=awaitopen(__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()=>{
-constfile=awaitopen(__filename);
-constreadable=file.readableWebStream({type: 'bytes'});
-constreader=readable.getReader({mode: 'byob'});
-file.close();
-awaitreader.closed;
-})().then(common.mustCall());
-
-// Make sure the 'bytes' ReadableStream is closed when the underlying
-// FileHandle is closed.
-(async()=>{
-constfile=awaitopen(__filename);
-constreadable=file.readableWebStream({type: 'bytes'});
-file.close();
-constreader=readable.getReader({mode: 'byob'});
-awaitreader.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()=>{
 constfile=awaitopen(__filename);
-file.readableWebStream({type: 'bytes'});
-constmc=newMessageChannel();
-mc.port1.onmessage=common.mustNotCall();
-assert.throws(()=>mc.port2.postMessage(file,[file]),{
-code: 25,
-name: 'DataCloneError',
+common.expectWarning({
+ExperimentalWarning: [
+'A non-"bytes" options.type has no effect. A byte-oriented steam is '+
+'always created.',
+],
 });
-mc.port1.close();
+conststream=file.readableWebStream({type: 'foobar'});
+awaitstream.cancel();
 awaitfile.close();
 })().then(common.mustCall());