Skip to content

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

Merged
nodejs-github-bot merged 3 commits intonodejs:mainfrom
debadree25:ft/blob-byob
Sep 22, 2023
Merged

lib: allow byob reader for 'blob.stream()'#49713
nodejs-github-bot merged 3 commits intonodejs:mainfrom
debadree25:ft/blob-byob

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
// hit the flow rate of the stream (c.desiredSize).
queueMicrotask(() => {
if (c.desiredSize <= 0) {
if (c.desiredSize < 0) {
Copy link
Member Author

@debadree25 debadree25 Sep 19, 2023

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

debadree25 commented Sep 19, 2023

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

nodejs-github-bot commented Sep 19, 2023

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

nodejs-github-bot commented Sep 21, 2023

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

nodejs-github-bot commented Sep 21, 2023

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
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 22, 2023

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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

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