Skip to content

stream: avoid duplicated endReadableNT scheduling - #65310

Open
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:stream-dedupe-endreadable-tick
Open

stream: avoid duplicated endReadableNT scheduling#65310
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:stream-dedupe-endreadable-tick

Conversation

@mcollina

Copy link
Copy Markdown
Member

Calling read() on an ended stream multiple times before the microtask queue drains scheduled one endReadableNT tick per call, since the only guard (endEmitted) is set inside the tick itself. A hello-world HTTP server was scheduling it four times per request while dumping the unread request body.

Add a kEndScheduled flag, armed on scheduling and cleared when the tick runs. It must be cleared unconditionally: undestroy() resets endEmitted through the state descriptors but cannot reach this flag, and a stale value would block 'end' after a net.Socket reconnect.

Benchmarks vs main: streams/finished.js readable +10.45% (99.9% confidence), no significant regressions across the streams suite; ~+1-3% on HTTP server benchmarks (10 → 7 nextTicks per request).

Calling read() on an ended stream multiple times before the microtask
queue drains scheduled one endReadableNT tick per call, as the only
guard was endEmitted, which is set inside the tick itself. A hello-world
HTTP server was scheduling it four times per request while dumping the
unread request body.

Introduce a kEndScheduled flag armed when the tick is scheduled and
cleared when it runs. Clearing it unconditionally matters for reused
sockets: undestroy() resets endEmitted through the state descriptors
but cannot reach this flag, and a stale value would block the 'end'
event after a net.Socket reconnect.
@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 the stream subsystem. labels Aug 15, 2026
@codecov

codecov Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.13%. Comparing base (347e266) to head (4c33258).
⚠️ Report is 139 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65310      +/-   ##
==========================================
- Coverage   90.30%   90.13%   -0.17%     
==========================================
  Files         759      752       -7     
  Lines      248294   251575    +3281     
  Branches    46860    47267     +407     
==========================================
+ Hits       224220   226757    +2537     
- Misses      15516    16177     +661     
- Partials     8558     8641      +83     
Files with missing lines Coverage Δ
lib/internal/streams/readable.js 96.56% <100.00%> (-0.72%) ⬇️

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

@ronag ronag added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 16, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 16, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

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

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants