Skip to content
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.

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

lib: allow byob reader for 'blob.stream()' #49713

Merged
merged 3 commits into from
Sep 22, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
10 changes: 8 additions & 2 deletions lib/internal/blob.js
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@

const reader = this[kHandle].getReader();
return new lazyReadableStream({
type: 'bytes',
start(c) {
// There really should only be one read at a time so using an
// array here is purely defensive.
Expand All @@ -339,6 +340,9 @@
if (status === 0) {
// EOS
c.close();
// this is to signal the end for byob readers

Check failure on line 343 in lib/internal/blob.js

View workflow job for this annotation

GitHub Actions / lint-js-and-md

Comments should not begin with a lowercase character

Check failure on line 343 in lib/internal/blob.js

View workflow job for this annotation

GitHub Actions / lint-js-and-md

Trailing spaces not allowed
// see https://streams.spec.whatwg.org/#example-rbs-pull
c.byobRequest?.respond(0);
const pending = this.pendingPulls.shift();
pending.resolve();
return;
Expand All @@ -352,13 +356,15 @@
pending.reject(error);
return;
}
if (buffer !== undefined) {
// ReadableByteStreamController.enqueue errors if we submit a 0-length
// buffer. We need to check for that here.
if (buffer !== undefined && buffer.byteLength !== 0) {
c.enqueue(new Uint8Array(buffer));
}
// We keep reading until we either reach EOS, some error, or we
// hit the flow rate of the stream (c.desiredSize).
queueMicrotask(() => {
if (c.desiredSize <= 0) {
if (c.desiredSize < 0) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would draw attention of reviewers to this specific update it seems when we turn this into a ReadableByteStream the high water mark for bytestreams is 0 and for normal default streams is 1 hence we have to remove the equality here
see:

if (`${type}` === 'bytes') {
if (size !== undefined)
throw new ERR_INVALID_ARG_VALUE.RangeError('strategy.size', size);
setupReadableByteStreamControllerFromSource(
this,
source,
extractHighWaterMark(highWaterMark, 0));
} else {
if (type !== undefined)
throw new ERR_INVALID_ARG_VALUE('source.type', type);
setupReadableStreamDefaultControllerFromSource(
this,
source,
extractHighWaterMark(highWaterMark, 1),
extractSizeAlgorithm(size));
}
}

also each byte is considered to be an element in the case of byte streams and hence the desiredSize can go below 0, I nonetheless asserted that the memory isnt overflowing in the existing tests but do take a look at them too if they look correct!

Thank You!

// A manual backpressure check.
if (this.pendingPulls.length !== 0) {
// A case of waiting pull finished (= not yet canceled)
Expand Down
46 changes: 44 additions & 2 deletions test/parallel/test-blob.js
Original file line number Diff line number Diff line change
Expand Up @@ -331,12 +331,54 @@
const b = new Blob(Array(10).fill('hello'));
const stream = b.stream();
const reader = stream.getReader();
assert.strictEqual(stream[kState].controller.desiredSize, 1);
assert.strictEqual(stream[kState].controller.desiredSize, 0);
const { value, done } = await reader.read();
assert.strictEqual(value.byteLength, 5);
assert(!done);
setTimeout(() => {
assert.strictEqual(stream[kState].controller.desiredSize, 0);
// the blob stream is now a byte stream hence after the first read,

Check failure on line 339 in test/parallel/test-blob.js

View workflow job for this annotation

GitHub Actions / lint-js-and-md

Comments should not begin with a lowercase character
// it should pull in the next 'hello' which is 5 bytes hence -5.
assert.strictEqual(stream[kState].controller.desiredSize, -5);
}, 0);
})().then(common.mustCall());

(async () => {
const blob = new Blob(['hello', 'world']);
const stream = blob.stream();
const reader = stream.getReader({ mode: 'byob' });
const decoder = new TextDecoder();
const chunks = [];
while (true) {
const { value, done } = await reader.read(new Uint8Array(100));
if (done) break;
chunks.push(decoder.decode(value, { stream: true }));
}
assert.strictEqual(chunks.join(''), 'helloworld');
})().then(common.mustCall());

(async () => {
const b = new Blob(Array(10).fill('hello'));
const stream = b.stream();
const reader = stream.getReader({ mode: 'byob' });
assert.strictEqual(stream[kState].controller.desiredSize, 0);
const { value, done } = await reader.read(new Uint8Array(100));
assert.strictEqual(value.byteLength, 5);
assert(!done);
setTimeout(() => {
assert.strictEqual(stream[kState].controller.desiredSize, -5);
}, 0);
})().then(common.mustCall());

(async () => {
const b = new Blob(Array(10).fill('hello'));
const stream = b.stream();
const reader = stream.getReader({ mode: 'byob' });
assert.strictEqual(stream[kState].controller.desiredSize, 0);
const { value, done } = await reader.read(new Uint8Array(2));
assert.strictEqual(value.byteLength, 2);
assert(!done);
setTimeout(() => {
assert.strictEqual(stream[kState].controller.desiredSize, -3);
}, 0);
})().then(common.mustCall());

Expand Down
7 changes: 0 additions & 7 deletions test/wpt/status/FileAPI/blob.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,5 @@
},
"Blob-slice.any.js": {
"skip": "Depends on File API"
},
"Blob-stream.any.js": {
"fail": {
"expected": [
"Reading Blob.stream() with BYOB reader"
]
}
}
}
Loading