Skip to content

stream: drop noop .then in pipe orchestration#63578

Closed
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:stream/markhandled-pipe-orchestration
Closed

stream: drop noop .then in pipe orchestration#63578
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:stream/markhandled-pipe-orchestration

Conversation

@mcollina
Copy link
Copy Markdown
Member

Follow-up to the per-chunk markPromiseAsHandled substitution. Apply the same pattern to the four remaining pipe-orchestration setPromiseHandled call sites that fire once per pipe setup (rather than per chunk):

  • readablestream.js: pipeThrough's inner readableStreamPipeTo promise
  • readablestream.js: pipeTo's run() loop promise
  • transfer.js: cross-realm readable + writable pipe promises

In each case the chain Promise + noop closure that setPromiseHandled allocates has no observer beyond V8's unhandled-rejection tracker — errors flow back via the relevant stream's closed/errored state path, which surrounding code already monitors. markPromiseAsHandled flips the same V8 flag with no allocation.

These sites are not on a per-chunk hot path, so no benchmark impact is expected. The change is purely consistency cleanup matching the per-chunk fix.

WPT streams parity preserved: 1403 subtests passing, 0 unexpected failures.

Other setPromiseHandled call sites in writablestream.js (Ensure{Ready,Closed}PromiseRejected, setupWritableStreamDefaultWriter, RejectCloseAndClosedPromiseIfNeeded) and readablestream.js (readableStreamError, readableStreamReaderGenericRelease) were considered but not converted: probing one of them (Ensure{Ready,Closed}PromiseRejected on the writer-release path) reproduces an unhandled-rejection regression on writable-streams/close.any.js and the piping error-propagation tests — MarkAsHandled and .then(undefined, () => {}) are not equivalent in that specific V8 path. Those sites stay on setPromiseHandled.

Apply the same markPromiseAsHandled-instead-of-setPromiseHandled
substitution as the per-chunk path to the four pipe orchestration call
sites that fire once per pipe setup rather than per chunk:

- readablestream.js: pipeThrough's inner readableStreamPipeTo promise
- readablestream.js: pipeTo's run() loop promise
- transfer.js: cross-realm readable pipe promise
- transfer.js: cross-realm writable pipe promise

In each case the chain Promise + noop closure that setPromiseHandled
allocates has no observer beyond V8's unhandled-rejection tracker:
errors flow back via the relevant stream's closed/errored state path,
which the surrounding code already monitors. markPromiseAsHandled flips
the same V8 flag without the allocation.

No measurable benchmark impact (these sites fire once per pipe, not per
chunk), but the change is consistent with the per-chunk fix and removes
four unnecessary Promise + closure allocations from each pipe setup.

WPT streams parity preserved: 1403 subtests passing, 0 unexpected
failures.
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels May 26, 2026
@mcollina
Copy link
Copy Markdown
Member Author

Closing — no measurable perf benefit. These four sites fire once per pipe setup, not per chunk, so the consistency cleanup doesn't justify reviewer burden. The per-chunk equivalent change ships in #63572.

@mcollina mcollina closed this May 26, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.34%. Comparing base (74ccf38) to head (3bb877e).
⚠️ Report is 31 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63578      +/-   ##
==========================================
+ Coverage   90.32%   90.34%   +0.01%     
==========================================
  Files         730      730              
  Lines      234209   234483     +274     
  Branches    43934    43908      -26     
==========================================
+ Hits       211558   211837     +279     
+ Misses      14372    14371       -1     
+ Partials     8279     8275       -4     
Files with missing lines Coverage Δ
lib/internal/webstreams/readablestream.js 98.55% <100.00%> (+<0.01%) ⬆️
lib/internal/webstreams/transfer.js 98.11% <100.00%> (+0.03%) ⬆️
lib/internal/webstreams/util.js 99.57% <100.00%> (+<0.01%) ⬆️

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

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. web streams

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants