Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit ccaf7fe

Browse filesBrowse files
iskertargos
isker
authored and
targos
committed
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>
1 parent 0c3ae25 commit ccaf7fe
Copy full SHA for ccaf7fe

File tree

Expand file treeCollapse file tree

3 files changed

+47
-95
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

3 files changed

+47
-95
lines changed
Open diff view settings
Collapse file

‎doc/api/fs.md‎

Copy file name to clipboardExpand all lines: doc/api/fs.md
+6-6Lines changed: 6 additions & 6 deletions
  • Display the source diff
  • Display the rich diff
Original file line numberDiff line numberDiff line change
@@ -477,11 +477,14 @@ Reads data from the file and stores that in the given buffer.
477477
If the file is not modified concurrently, the end-of-file is reached when the
478478
number of bytes read is zero.
479479
480-
#### `filehandle.readableWebStream([options])`
480+
#### `filehandle.readableWebStream()`
481481
482482
<!-- YAML
483483
added: v17.0.0
484484
changes:
485+
- version: REPLACEME
486+
pr-url: https://github.com/nodejs/node/pull/55461
487+
description: Removed option to create a 'bytes' stream. Streams are now always 'bytes' streams.
485488
- version:
486489
- v20.0.0
487490
- v18.17.0
@@ -491,13 +494,10 @@ changes:
491494
492495
> Stability: 1 - Experimental
493496
494-
* `options` {Object}
495-
* `type` {string|undefined} Whether to open a normal or a `'bytes'` stream.
496-
**Default:** `undefined`
497-
498497
* Returns: {ReadableStream}
499498
500-
Returns a `ReadableStream` that may be used to read the files data.
499+
Returns a byte-oriented `ReadableStream` that may be used to read the file's
500+
contents.
501501
502502
An error will be thrown if this method is called more than once or is called
503503
after the `FileHandle` is closed or closing.
Collapse file

‎lib/internal/fs/promises.js‎

Copy file name to clipboardExpand all lines: lib/internal/fs/promises.js
+31-36Lines changed: 31 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,10 @@ function lazyFsStreams() {
142142

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

145+
const lazyReadableStream = getLazy(() =>
146+
require('internal/webstreams/readablestream').ReadableStream,
147+
);
148+
145149
// By the time the C++ land creates an error for a promise rejection (likely from a
146150
// libuv callback), there is already no JS frames on the stack. So we need to
147151
// wait until V8 resumes execution back to JS land before we have enough information
@@ -276,12 +280,9 @@ class FileHandle extends EventEmitter {
276280
/**
277281
* @typedef {import('../webstreams/readablestream').ReadableStream
278282
* } ReadableStream
279-
* @param {{
280-
* type?: string;
281-
* }} [options]
282283
* @returns {ReadableStream}
283284
*/
284-
readableWebStream(options = kEmptyObject) {
285+
readableWebStream(options = { __proto__: null, type: 'bytes' }) {
285286
if (this[kFd] === -1)
286287
throw new ERR_INVALID_STATE('The FileHandle is closed');
287288
if (this[kClosePromise])
@@ -293,46 +294,40 @@ class FileHandle extends EventEmitter {
293294
if (options.type !== undefined) {
294295
validateString(options.type, 'options.type');
295296
}
297+
if (options.type !== 'bytes') {
298+
process.emitWarning(
299+
'A non-"bytes" options.type has no effect. A byte-oriented steam is ' +
300+
'always created.',
301+
'ExperimentalWarning',
302+
);
303+
}
296304

297-
let readable;
298305

299-
if (options.type !== 'bytes') {
300-
const {
301-
newReadableStreamFromStreamBase,
302-
} = require('internal/webstreams/adapters');
303-
readable = newReadableStreamFromStreamBase(
304-
this[kHandle],
305-
undefined,
306-
{ ondone: () => this[kUnref]() });
307-
} else {
308-
const {
309-
ReadableStream,
310-
} = require('internal/webstreams/readablestream');
306+
const readFn = FunctionPrototypeBind(this.read, this);
307+
const ondone = FunctionPrototypeBind(this[kUnref], this);
311308

312-
const readFn = FunctionPrototypeBind(this.read, this);
313-
const ondone = FunctionPrototypeBind(this[kUnref], this);
309+
const ReadableStream = lazyReadableStream();
310+
const readable = new ReadableStream({
311+
type: 'bytes',
312+
autoAllocateChunkSize: 16384,
314313

315-
readable = new ReadableStream({
316-
type: 'bytes',
317-
autoAllocateChunkSize: 16384,
314+
async pull(controller) {
315+
const view = controller.byobRequest.view;
316+
const { bytesRead } = await readFn(view, view.byteOffset, view.byteLength);
318317

319-
async pull(controller) {
320-
const view = controller.byobRequest.view;
321-
const { bytesRead } = await readFn(view, view.byteOffset, view.byteLength);
318+
if (bytesRead === 0) {
319+
ondone();
320+
controller.close();
321+
}
322322

323-
if (bytesRead === 0) {
324-
ondone();
325-
controller.close();
326-
}
323+
controller.byobRequest.respond(bytesRead);
324+
},
327325

328-
controller.byobRequest.respond(bytesRead);
329-
},
326+
cancel() {
327+
ondone();
328+
},
329+
});
330330

331-
cancel() {
332-
ondone();
333-
},
334-
});
335-
}
336331

337332
const {
338333
readableStreamCancel,
Collapse file

‎test/parallel/test-filehandle-readablestream.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-filehandle-readablestream.js
+10-53Lines changed: 10 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -87,11 +87,11 @@ const check = readFileSync(__filename, { encoding: 'utf8' });
8787
await file.close();
8888
})().then(common.mustCall());
8989

90-
// Make sure 'bytes' stream works
90+
// Make sure 'byob' reader works
9191
(async () => {
9292
const file = await open(__filename);
9393
const dec = new TextDecoder();
94-
const readable = file.readableWebStream({ type: 'bytes' });
94+
const readable = file.readableWebStream();
9595
const reader = readable.getReader({ mode: 'byob' });
9696

9797
let data = '';
@@ -114,59 +114,16 @@ const check = readFileSync(__filename, { encoding: 'utf8' });
114114
await file.close();
115115
})().then(common.mustCall());
116116

117-
// Make sure that acquiring a ReadableStream 'bytes' stream
118-
// fails if the FileHandle is already closed.
119-
(async () => {
120-
const file = await open(__filename);
121-
await file.close();
122-
123-
assert.throws(() => file.readableWebStream({ type: 'bytes' }), {
124-
code: 'ERR_INVALID_STATE',
125-
});
126-
})().then(common.mustCall());
127-
128-
// Make sure that acquiring a ReadableStream 'bytes' stream
129-
// fails if the FileHandle is already closing.
130-
(async () => {
131-
const file = await open(__filename);
132-
file.close();
133-
134-
assert.throws(() => file.readableWebStream({ type: 'bytes' }), {
135-
code: 'ERR_INVALID_STATE',
136-
});
137-
})().then(common.mustCall());
138-
139-
// Make sure the 'bytes' ReadableStream is closed when the underlying
140-
// FileHandle is closed.
141-
(async () => {
142-
const file = await open(__filename);
143-
const readable = file.readableWebStream({ type: 'bytes' });
144-
const reader = readable.getReader({ mode: 'byob' });
145-
file.close();
146-
await reader.closed;
147-
})().then(common.mustCall());
148-
149-
// Make sure the 'bytes' ReadableStream is closed when the underlying
150-
// FileHandle is closed.
151-
(async () => {
152-
const file = await open(__filename);
153-
const readable = file.readableWebStream({ type: 'bytes' });
154-
file.close();
155-
const reader = readable.getReader({ mode: 'byob' });
156-
await reader.closed;
157-
})().then(common.mustCall());
158-
159-
// Make sure that the FileHandle is properly marked "in use"
160-
// when a 'bytes' ReadableStream has been acquired for it.
117+
// Make sure a warning is logged if a non-'bytes' type is passed.
161118
(async () => {
162119
const file = await open(__filename);
163-
file.readableWebStream({ type: 'bytes' });
164-
const mc = new MessageChannel();
165-
mc.port1.onmessage = common.mustNotCall();
166-
assert.throws(() => mc.port2.postMessage(file, [file]), {
167-
code: 25,
168-
name: 'DataCloneError',
120+
common.expectWarning({
121+
ExperimentalWarning: [
122+
'A non-"bytes" options.type has no effect. A byte-oriented steam is ' +
123+
'always created.',
124+
],
169125
});
170-
mc.port1.close();
126+
const stream = file.readableWebStream({ type: 'foobar' });
127+
await stream.cancel();
171128
await file.close();
172129
})().then(common.mustCall());

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.