OSSForks/node
1
0
Fork 0
mirror of https://github.com/zebrajr/node.git synced 2026-01-15 12:15:26 +00:00
Code Issues Actions 8 Packages Projects Releases Wiki Activity

fs: make FileHandle.readableWebStream always create byte streams

Browse Source 
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: https://github.com/nodejs/node/pull/55461
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This commit is contained in:
Ian Kerins
2025-02-09 22:40:31 -05:00
committed by GitHub
parent a2aa6ca9d7
commit 1781f63633
3 changed files with 53 additions and 101 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
doc/api/fs.md
View File

@@ -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.

79
lib/internal/fs/promises.js
View File

@@ -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
@@ -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)
throw new ERR_INVALID_STATE('The FileHandle is closed');
if (this[kClosePromise])
@@ -293,47 +294,41 @@ class FileHandle extends EventEmitter {
if (options.type !== undefined) {
validateString(options.type, 'options.type');
}
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);
readable = new ReadableStream({
type: 'bytes',
autoAllocateChunkSize: 16384,
async pull(controller) {
const view = controller.byobRequest.view;
const { bytesRead } = await readFn(view, view.byteOffset, view.byteLength);
if (bytesRead === 0) {
ondone();
controller.close();
}
controller.byobRequest.respond(bytesRead);
},
cancel() {
ondone();
},
});
process.emitWarning(
'A non-"bytes" options.type has no effect. A byte-oriented steam is ' +
'always created.',
'ExperimentalWarning',
);
}
const readFn = FunctionPrototypeBind(this.read, this);
const ondone = FunctionPrototypeBind(this[kUnref], this);
const ReadableStream = lazyReadableStream();
const readable = new ReadableStream({
type: 'bytes',
autoAllocateChunkSize: 16384,
async pull(controller) {
const view = controller.byobRequest.view;
const { bytesRead } = await readFn(view, view.byteOffset, view.byteLength);
if (bytesRead === 0) {
ondone();
controller.close();
}
controller.byobRequest.respond(bytesRead);
},
cancel() {
ondone();
},
});
const {
readableStreamCancel,
} = require('internal/webstreams/readablestream');

63
test/parallel/test-filehandle-readablestream.js
View File

@@ -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,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.
// Make sure a warning is logged if a non-'bytes' type is passed.
(async () => {
const file = await open(__filename);
await file.close();
assert.throws(() => file.readableWebStream({ type: 'bytes' }), {
code: 'ERR_INVALID_STATE',
common.expectWarning({
ExperimentalWarning: [
'A non-"bytes" options.type has no effect. A byte-oriented steam is ' +
'always created.',
],
});
})().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();
const stream = file.readableWebStream({ type: 'foobar' });
await stream.cancel();
await file.close();
})().then(common.mustCall());