stream: readable read one buffer at a time

[ronag]

Instead of wasting cycles concatenating buffers, just return each one by one.

[nodejs-github-bot]

Review requested:

• @nodejs/streams

[mcollina]

code lgtm

I think some docs changes are needed

[mcollina]

Marked as semver-major.

[lpinca]

I'm -1 on this unless there is a strong reason for it. Historically, readable.read() has always returned all queued data. If this lands, I think that the easiest workaround for modules affected by the breaking change would be replacing readable.read() with readable.read(readable.readableLength).

[ronag]

I'm -1 on this unless there is a strong reason for it.

The performance overhead is huge. As it stand we should at least add a note to avoid this api for anything performance sensitive.

If this lands, I think that the easiest workaround for modules affected by the breaking change would be replacing readable.read() with readable.read(readable.readableLength).

Good point.

[ronag]

I think some docs changes are needed

What doc changes are you roughly asking for?

[github-actions]

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 PR

https://github.com/nodejs/node/actions/runs/20654449594

[github-actions]

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 PR

https://github.com/nodejs/node/actions/runs/20654684497

[mcollina]

actually, no doc changes are needed.

Each call to readable.read() returns a chunk of data or null, signifying that there's no more data to read at that moment. These chunks aren't automatically concatenated. Because a single read() call does not return all the data, using a while loop may be necessary to continuously read chunks until all data is retrieved. When reading a large file, .read() might return null temporarily, indicating that it has consumed all buffered content but there may be more data yet to be buffered. In such cases, a new 'readable' event is emitted once there's more data in the buffer, and the 'end' event signifies the end of data transmission.

@lpinca this PR matches what we are already documenting. We could even make a case for this to be a bugfix.

Anyway, we should run citgm and see if there are breakages.

Given the significant impact on performance, I think we should be landing this.

[lpinca]

Anyway, we should run citgm and see if there are breakages.

There is an occurrence in ws and I don't think CITGM would catch it because, if I remember correctly, the test uses a single buffer, but I can probably fix it as written above.

[github-actions]

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 PR

https://github.com/nodejs/node/actions/runs/20655680849

[mcollina]

lgtm

[nodejs-github-bot]

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.55%. Comparing base (0457bfe) to head (93274c5).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #60441      +/-   ##
==========================================
+ Coverage   88.53%   88.55%   +0.01%     
==========================================
  Files         704      704              
  Lines      208736   208741       +5     
  Branches    40274    40276       +2     
==========================================
+ Hits       184810   184856      +46     
+ Misses      15944    15905      -39     
+ Partials     7982     7980       -2     

Files with missing lines	Coverage Δ
lib/internal/streams/readable.js	96.27% <100.00%> (+0.01%)	⬆️

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.