stream: use ByteLengthQueuingStrategy when not in object mode by CGQAQ · Pull Request #48847 · nodejs/node · GitHub
Skip to content

stream: use ByteLengthQueuingStrategy when not in object mode#48847

Merged
aduh95 merged 14 commits into
nodejs:mainfrom
CGQAQ:stream-nodestream-toweb
May 12, 2024
Merged

stream: use ByteLengthQueuingStrategy when not in object mode#48847
aduh95 merged 14 commits into
nodejs:mainfrom
CGQAQ:stream-nodestream-toweb

Conversation

@CGQAQ

@CGQAQ CGQAQ commented Jul 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Use ByteLengthQueuingStrategy when not in object mode

After this PR

Array buffers memory usage is 0 MiB
Array buffers memory usage is 0 MiB
converting node stream to web stream
Array buffers memory usage is 0 MiB
Array buffers memory usage is 0 MiB
Array buffers memory usage is 0 MiB
Array buffers memory usage is 0 MiB
Array buffers memory usage is 0 MiB
Array buffers memory usage is 0 MiB
Array buffers memory usage is 0 MiB
reading the chunks
Array buffers memory usage is 2 MiB
Array buffers memory usage is 31 MiB
Array buffers memory usage is 9 MiB
Array buffers memory usage is 18 MiB
Array buffers memory usage is 9 MiB
Array buffers memory usage is 5 MiB
Array buffers memory usage is 8 MiB
Array buffers memory usage is 17 MiB
Array buffers memory usage is 13 MiB
Array buffers memory usage is 12 MiB
Array buffers memory usage is 19 MiB
Array buffers memory usage is 20 MiB
Array buffers memory usage is 31 MiB
Array buffers memory usage is 11 MiB
Array buffers memory usage is 24 MiB
Array buffers memory usage is 17 MiB

fixes: #46347

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Jul 20, 2023
@CGQAQ CGQAQ changed the title stream: don't pull for first time (fixes #46347) stream: don't pull for the first time (fixes #46347) Jul 20, 2023
@CGQAQ CGQAQ force-pushed the stream-nodestream-toweb branch from 2d2b9d0 to a87605f Compare July 20, 2023 08:19
@debadree25

debadree25 commented Jul 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

CGQAQ
CGQAQ commented Jul 20, 2023
View reviewed changes
Comment thread lib/internal/webstreams/adapters.js
@CGQAQ CGQAQ requested a review from debadree25 July 21, 2023 02:06
@CGQAQ CGQAQ force-pushed the stream-nodestream-toweb branch from ee2ce42 to 2d2672a Compare July 21, 2023 02:10
@CGQAQ CGQAQ changed the title stream: don't pull for the first time (fixes #46347) stream: fix wrong stream type in adapters.js (fixes nodejs#46347) Jul 21, 2023
@CGQAQ

This comment was marked as resolved.

Sign in to view
@CGQAQ CGQAQ force-pushed the stream-nodestream-toweb branch from d6fcf75 to 74ada82 Compare July 21, 2023 04:40
@CGQAQ CGQAQ changed the title stream: fix wrong stream type in adapters.js (fixes nodejs#46347) stream: use ByteLengthQueuingStrategy when not in object mode Jul 21, 2023
@CGQAQ CGQAQ force-pushed the stream-nodestream-toweb branch 2 times, most recently from c5c66b4 to 81c3376 Compare July 21, 2023 09:52
@debadree25

debadree25 commented Jul 22, 2023

Copy link
Copy Markdown
Contributor

A previous PR had attempted the same, nonethless would cc @nodejs/whatwg-stream to take a look!

@jasnell

jasnell commented Sep 14, 2023

Copy link
Copy Markdown
Member

This would probably not be spec compliant, streams.spec.whatwg.org/#readable-stream-default-controller-should-call-pull I do not think we are allowed to add additional steps not mentioned in the spec

Given that this is for our Node.js stream adapters, this change should be fine.

@nodejs-github-bot

nodejs-github-bot commented Sep 14, 2023

Copy link
Copy Markdown
Collaborator

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

@github-actions github-actions Bot mentioned this pull request Sep 15, 2023
Open
13 tasks
@nodejs-github-bot

nodejs-github-bot commented Sep 15, 2023

Copy link
Copy Markdown
Collaborator

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

debadree25
debadree25 reviewed Sep 15, 2023
View reviewed changes
Comment thread test/parallel/test-stream-readable-to-web.js

assert(currentMemoryUsage <= 256 * 1024 * 1024);
};
setInterval(reportMemoryUsage, 1000);

@debadree25 debadree25 Sep 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if this test could be somewhat flakey? too many timeouts but nonetheless shouldn't be a blocker

@Ethan-Arrowood Ethan-Arrowood Jul 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ironically yes, this test has become flaky: nodejs/reliability#1250

mcollina
mcollina approved these changes Sep 15, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@debadree25 debadree25 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 15, 2023
This was referenced Sep 16, 2023
Open
Open
Open
Open
@nodejs-github-bot

nodejs-github-bot commented Sep 23, 2023

Copy link
Copy Markdown
Collaborator

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

@github-actions github-actions Bot mentioned this pull request Sep 24, 2023
Open
20 tasks
@nodejs-github-bot

nodejs-github-bot commented Sep 26, 2023

Copy link
Copy Markdown
Collaborator

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

@aduh95 aduh95 force-pushed the stream-nodestream-toweb branch from 5b65e37 to 6cb8b2f Compare May 11, 2024 14:09
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 11, 2024
@nodejs-github-bot

nodejs-github-bot commented May 11, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented May 12, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented May 12, 2024

Copy link
Copy Markdown
Collaborator

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

@aduh95 aduh95 merged commit 261e88e into nodejs:main May 12, 2024
@aduh95

aduh95 commented May 12, 2024

Copy link
Copy Markdown
Contributor

Landed in 261e88e

targos pushed a commit that referenced this pull request May 12, 2024
@CGQAQ @targos
stream: use ByteLengthQueuingStrategy when not in objectMode
8d3b0b7 
Fixes: #46347
PR-URL: #48847
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@CGQAQ CGQAQ deleted the stream-nodestream-toweb branch May 13, 2024 02:33
@targos targos mentioned this pull request May 13, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@CGQAQ @marco-ippolito
stream: use ByteLengthQueuingStrategy when not in objectMode
6257273 
Fixes: #46347
PR-URL: #48847
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@CGQAQ @marco-ippolito
stream: use ByteLengthQueuingStrategy when not in objectMode
4010424 
Fixes: #46347
PR-URL: #48847
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@CGQAQ @marco-ippolito
stream: use ByteLengthQueuingStrategy when not in objectMode
af29120 
Fixes: #46347
PR-URL: #48847
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@CGQAQ @soophoo
stream: use ByteLengthQueuingStrategy when not in objectMode
4aee175 
Fixes: nodejs#46347
PR-URL: nodejs#48847
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@CGQAQ @bmeck
stream: use ByteLengthQueuingStrategy when not in objectMode
a7e3f47 
Fixes: nodejs#46347
PR-URL: nodejs#48847
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@arsnyder16

arsnyder16 commented Aug 30, 2024

Copy link
Copy Markdown

@Ethan-Arrowood Ethan-Arrowood mentioned this pull request Jul 3, 2025
Open
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ethan-Arrowood Ethan-Arrowood Ethan-Arrowood left review comments
@mcollina mcollina mcollina approved these changes
@jasnell jasnell jasnell approved these changes
+1 more reviewer
@debadree25 debadree25 debadree25 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

Readable.toWeb seems to load file contents to memory

10 participants

@CGQAQ @debadree25 @jasnell @nodejs-github-bot @aduh95 @idranme @mcollina @StefanStojanovic @arsnyder16 @Ethan-Arrowood