Skip to content

streams: more iter streams fixes - #66028

Open
jasnell wants to merge 4 commits into
nodejs:mainfrom
jasnell:jasnell/moar-iter-streams
Open

jasnell wants to merge 4 commits into
nodejs:mainfrom
jasnell:jasnell/moar-iter-streams

Conversation

@jasnell

@jasnell jasnell commented Sep 15, 2026

Copy link
Copy Markdown
Member

Another batch of iterable streams fixes. See each commit.

While the Broadcast is still open, maintain buffered data when
consumer count reaches zero

Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Opencode
Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Opencode
Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Opencode
Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Opencode
@jasnell
jasnell requested review from panva and trivikr September 15, 2026 03:39
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to Node.js streams. labels Sep 15, 2026
@codecov

codecov Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.71930% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.24%. Comparing base (c081d10) to head (7befc15).

Files with missing lines Patch % Lines
lib/internal/streams/iter/broadcast.js 87.50% 3 Missing and 1 partial ⚠️
lib/internal/streams/iter/share.js 88.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66028      +/-   ##
==========================================
+ Coverage   90.22%   90.24%   +0.01%     
==========================================
  Files         787      787              
  Lines      270307   270349      +42     
  Branches    51726    51749      +23     
==========================================
+ Hits       243890   243979      +89     
+ Misses      16899    16846      -53     
- Partials     9518     9524       +6     
Files with missing lines Coverage Δ
lib/internal/streams/iter/share.js 89.35% <88.00%> (+1.55%) ⬆️
lib/internal/streams/iter/broadcast.js 90.17% <87.50%> (-0.03%) ⬇️

... and 25 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.

@panva panva 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.

Review Assisted-by: Codex

Comment on lines +608 to +612
while (self.#bufferedBytes >= self.#options.budget &&
!self.#sourceExhausted &&
!self.#cancelled &&
self.#sourceError === kNoShareError) {
self.#pullFromSource(true);

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.

This loop cannot make buffer space: discarded entries do not reduce #bufferedBytes, and a slow consumer cannot advance while this synchronous next() is running. With an infinite generator yielding 16 KiB batches, a 16 KiB budget, and two consumers, the fast consumer's second next() loops forever. It also drains the entire remainder of a large finite source in one call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. stream Issues and PRs related to Node.js streams.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants