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.

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

Jump to bottom

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

Merged
merged 3 commits into from
Sep 22, 2023
Merged

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

merged 3 commits into from
Sep 22, 2023

Conversation

debadree25
Copy link
Member

@debadree25 debadree25 commented Sep 19, 2023 •
edited
Loading

Fixes: #47993

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 19, 2023
@debadree25 debadree25 mentioned this pull request Sep 19, 2023
Closed
@debadree25 debadree25 marked this pull request as ready for review September 19, 2023 10:34
debadree25
debadree25 commented Sep 19, 2023
View reviewed changes
lib/internal/blob.js
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:

node/lib/internal/webstreams/readablestream.js

Lines 282 to 298 in 2ccfb23

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!

@debadree25
Copy link
Member Author

cc @nodejs/whatwg-stream

@debadree25 debadree25 added request-ci Add this label to start a Jenkins CI on a PR. web streams labels Sep 19, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 19, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54059/

This was referenced Sep 20, 2023
Open
Open
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54110/

@debadree25 debadree25 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 21, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54112/

@debadree25 debadree25 added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 21, 2023
mcollina
mcollina approved these changes Sep 21, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@github-actions github-actions bot mentioned this pull request Sep 22, 2023
Open
17 tasks
@debadree25 debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 22, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 22, 2023
@nodejs-github-bot nodejs-github-bot merged commit 23a3410 into nodejs:main Sep 22, 2023
36 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 23a3410

@github-actions github-actions bot mentioned this pull request Sep 24, 2023
Open
20 tasks
@debadree25 debadree25 deleted the ft/blob-byob branch September 24, 2023 17:31
@debadree25 debadree25 mentioned this pull request Sep 26, 2023
Closed
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
@debadree25 @ruyadorno
lib: allow byob reader for 'blob.stream()'
1287d5b 
Fixes: #47993
PR-URL: #49713
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Sep 28, 2023
Closed
@ruyadorno ruyadorno mentioned this pull request Sep 28, 2023
Merged
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@debadree25 @alexfernandez
lib: allow byob reader for 'blob.stream()'
cefe957 
Fixes: nodejs#47993
PR-URL: nodejs#49713
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjamingr benjamingr benjamingr approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. web streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

byob reader support for Blob.stream()
4 participants
@debadree25 @nodejs-github-bot @mcollina @benjamingr