Skip to content

fs: make FileHandle.readableWebStream always create byte streams #55461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nodejs-github-bot merged 1 commit into from
Feb 10, 2025
Merged

fs: make FileHandle.readableWebStream always create byte streams #55461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.
  • Loading branch information
@isker
isker committed Nov 17, 2024
commit 41c448c778734c4d6c04521b5eab7609d3f2125d
12 changes: 6 additions & 6 deletions doc/api/fs.md
View file
Open in desktop
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()`
Comment thread
isker marked this conversation as resolved.

<!-- 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:

Comment thread
isker marked this conversation as resolved.
> 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
67 changes: 31 additions & 36 deletions lib/internal/fs/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,10 @@ function lazyFsStreams() {

const lazyRimRaf = getLazy(() => require('internal/fs/rimraf').rimrafPromises);

const lazyReadableStream = 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
Expand Down Expand Up @@ -276,12 +280,9 @@ class FileHandle extends EventEmitter {
/**
* @typedef {import('../webstreams/readablestream').ReadableStream
* } ReadableStream
* @param {{
* type?: string;
* }} [options]
* @returns {ReadableStream}
*/
readableWebStream(options = kEmptyObject) {
Comment thread
isker marked this conversation as resolved.
readableWebStream(options = { __proto__: null, type: 'bytes' }) {
if (this[kFd] === -1)
throw new ERR_INVALID_STATE('The FileHandle is closed');
if (this[kClosePromise])
Expand All @@ -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',
);
}

let readable;

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 ReadableStream = lazyReadableStream();
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
View file
Open in desktop
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());
Comment thread
isker marked this conversation as resolved.