-
-
Notifications
You must be signed in to change notification settings - Fork 34.4k
stream: readable read one buffer at a time #60441
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
Conversation
|
Review requested:
|
mcollina
left a comment
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.
code lgtm
I think some docs changes are needed
|
Marked as semver-major. |
|
I'm -1 on this unless there is a strong reason for it. Historically, |
The performance overhead is huge. As it stand we should at least add a note to avoid this api for anything performance sensitive.
Good point. |
What doc changes are you roughly asking for? |
a380c35 to
0798db8
Compare
Instead of wasting cycles concatenating buffers, just return each one by one. Old behavior can be achieved by using `readable.read(readable.readableLength)` instead of `readable.read()`. PR: nodejs#60441
Instead of wasting cycles concatenating buffers, just return each one by one. Old behavior can be achieved by using `readable.read(readable.readableLength)` instead of `readable.read()`. PR: nodejs#60441
Instead of wasting cycles concatenating buffers, just return each one by one. Old behavior can be achieved by using `readable.read(readable.readableLength)` instead of `readable.read()`. PR: nodejs#60441
Instead of wasting cycles concatenating buffers, just return each one by one. Old behavior can be achieved by using `readable.read(readable.readableLength)` instead of `readable.read()`. PR: nodejs#60441
Failed to start CI⚠ Commits were pushed since the last approving review: ⚠ - stream: readable read one buffer at a time ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/20654449594 |
mcollina
left a comment
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
Instead of wasting cycles concatenating buffers, just return each one by one. Similar (but not exact) old behavior can be achieved by using `readable.read(readable.readableLength)` instead of `readable.read()`. In some edge cases it might be necessary to do a `readable.read(0)` first. PR: nodejs#60441
|
@lpinca Do you need help fixing ws in relation to this PR or can you deal with it yourself? |
mcollina
left a comment
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
I've already fixed it in websockets/ws@1998485 |
mcollina
left a comment
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
gurgunday
left a comment
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
Commit Queue failed- Loading data for nodejs/node/pull/60441 ✔ Done loading data for nodejs/node/pull/60441 ----------------------------------- PR info ------------------------------------ Title stream: readable read one buffer at a time (#60441) Author Robert Nagy <ronagy@icloud.com> (@ronag) Branch ronag:read-no-copy -> nodejs:main Labels stream, semver-major, author ready, needs-ci, needs-citgm Commits 1 - stream: readable read one buffer at a time Committers 1 - Robert Nagy <ronagy@icloud.com> PR-URL: https://github.com/nodejs/node/pull/60441 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/60441 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 27 Oct 2025 18:44:50 GMT ✔ Approvals: 3 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/60441#pullrequestreview-3631894479 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/60441#pullrequestreview-3632251382 ✔ - Gürgün Dayıoğlu (@gurgunday): https://github.com/nodejs/node/pull/60441#pullrequestreview-3635847744 ✘ semver-major requires at least 2 TSC approvals ✔ Last GitHub CI successful ℹ Last Full PR CI on 2026-01-05T21:27:50Z: https://ci.nodejs.org/job/node-test-pull-request/70687/ ℹ Last CITGM CI on 2026-01-05T09:54:54Z: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3663/ - Querying data for job/node-test-pull-request/70687/ ✔ Build data downloaded ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/21187285768 |
|
@nodejs/tsc can I get another approval? |
|
Landed in fadb214 |
Uh oh!
There was an error while loading. Please reload this page.