-
-
Notifications
You must be signed in to change notification settings - Fork 34.3k
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
base: main
Are you sure you want to change the base?
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 |
Failed to start CI⚠ Commits were pushed since the last approving review: ⚠ - stream: readable read one buffer at a time ⚠ - Update test/parallel/test-stream-readable-readable-one.js ⚠ - fixup ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/20683777042 |
Failed to start CI⚠ Commits were pushed since the last approving review: ⚠ - stream: readable read one buffer at a time ⚠ - Update test/parallel/test-stream-readable-readable-one.js ⚠ - fixup ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/20683908897 |
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
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 |
Uh oh!
There was an error while loading. Please reload this page.