Skip to content

Commit

Permalink
fs: make FileHandle.readableWebStream always create byte streams
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
isker committed Nov 17, 2024
1 parent 5e5af29 commit 86a1021
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 95 deletions.
12 changes: 6 additions & 6 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
65 changes: 29 additions & 36 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Expand All @@ -293,46 +290,42 @@ 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',
);
}

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,
Expand Down
63 changes: 10 additions & 53 deletions test/parallel/test-filehandle-readablestream.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '';
Expand All @@ -114,59 +114,16 @@ 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" options.type has no effect. A byte-oriented steam is ' +
'always created.',
],
});
mc.port1.close();
const stream = file.readableWebStream({ type: 'foobar' });
await stream.cancel();
await file.close();
})().then(common.mustCall());

0 comments on commit 86a1021

Please sign in to comment.