GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — stream: use ByteLengthQueuingStrategy when not in object mode by CGQAQ · Pull Request #48847 · nodejs/node

Conversation

@CGQAQ

@CGQAQ CGQAQ commented Jul 20, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss, hax, GCM6, and mtsmfm reacted with thumbs up emoji
All reactions
  • 👍 4 reactions
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams Issues and PRs related to the Web Streams API. 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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This would probably not be spec compliant, https://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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
CGQAQ
CGQAQ commented Jul 20, 2023
View reviewed changes
Comment thread lib/internal/webstreams/adapters.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Sep 14, 2023
View reviewed changes
@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.
debadree25 reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Sep 15, 2023
CI Reliability 2023-09-15 nodejs/reliability#666
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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
debadree25
debadree25 reviewed Sep 15, 2023
View reviewed changes
Comment thread test/parallel/test-stream-readable-to-web.js Outdated

@debadree25 debadree25 Sep 15, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Ethan-Arrowood Ethan-Arrowood Jul 3, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mcollina
mcollina approved these changes Sep 15, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@debadree25 debadree25 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 15, 2023
This was referenced Sep 16, 2023
CI Reliability 2023-09-16 nodejs/reliability#667
Open
CI Reliability 2023-09-17 nodejs/reliability#668
Open
CI Reliability 2023-09-18 nodejs/reliability#669
Open
CI Reliability 2023-09-19 nodejs/reliability#670
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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Sep 24, 2023
CI Reliability 2023-09-24 nodejs/reliability#675
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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
CGQAQ added 12 commits May 11, 2024 17:09
@CGQAQ @aduh95
stream: fix format again
d68f95f
@CGQAQ @aduh95
stream: add test case and format
aec3501
@CGQAQ @aduh95
other: remove unwanted file
ea2db5f
@CGQAQ @aduh95
stream: change comment
7da26c5
@CGQAQ @aduh95
stream: fix wrong stream type in adapters.js
a93d0c1
@CGQAQ @aduh95
stream: format
ad1cb8f
@CGQAQ @aduh95
stream: format
d662d53
@CGQAQ @aduh95
stream: single quotes
ace2966
@CGQAQ @aduh95
stream: use ByteLengthQueuingStrategy when not in object mode (fixes n…
752229a 
…odejs#46347)
@CGQAQ @aduh95
fix test
e614953
@CGQAQ @aduh95
add hasCrypto check
26ff010
@CGQAQ @aduh95
run make lint-js-fix
6cb8b2f
@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 with CI started, the required approvals, and no outstanding review comments. 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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@CGQAQ
CGQAQ deleted the stream-nodestream-toweb branch May 13, 2024 02:33
@targos targos mentioned this pull request May 13, 2024
v22.2.0 release proposal #52971
Merged
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
v20.15.0 proposal #53486
Merged
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@arsnyder16

arsnyder16 commented Aug 30, 2024

Copy link
Copy Markdown

@CGQAQ Same issues with newWritableStreamFromStreamWritable ?https://github.com/CGQAQ/node/blob/885692a34f582c99d74430ba351879431738e43a/lib/internal/webstreams/adapters.js#L167
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Ethan-Arrowood Ethan-Arrowood mentioned this pull request Jul 3, 2025
CI Reliability 2025-07-03 nodejs/reliability#1250
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. needs-ci PRs that need a full CI run. web streams Issues and PRs related to the Web Streams API.

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

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@CGQAQ @debadree25 @jasnell @nodejs-github-bot @aduh95 @idranme @mcollina @StefanStojanovic @arsnyder16 @Ethan-Arrowood
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.