Skip to content

stream: keep async context of the pipeTo pump - #67054

Open
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:webstreams-pipeto-async-context
Open

mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:webstreams-pipeto-async-context

Conversation

@mcollina

Copy link
Copy Markdown
Member

The pipeTo() pump re-enters from two places that are not promise reactions:

  • the destination's ready hook (parkOnReady, stream: cut promise churn in webstreams hot paths #65138), which writableStreamUpdateBackpressure invokes synchronously from whatever code cleared the backpressure;
  • the chunk steps of its read request, which run in the context of whoever enqueued the chunk.

So once the pipe resumes, the source's pull() and the writes it issues observe that context instead of the one of the pipeTo() call, which is what the promise-based pump (v24.20 and earlier, v26.9 and earlier) gave them:

const pending = [];
const rs = new ReadableStream({ pull(c) { console.log(als.getStore()); c.enqueue(1); } }, { highWaterMark: 0 });
const ws = new WritableStream({
  write() { return new Promise((resolve) => pending.push(resolve)); },
}, { highWaterMark: 1 });
als.run('piper', () => rs.pipeTo(ws));
// Resolving the writes from another context leaks it into pull():
// logs undefined (the creator's context) instead of 'piper' after the first chunk.

This captures the async context frame when the pipe starts and runs the pump in it when it is entered from another context. The frame check uses the intrinsics directly: the AsyncContextFrame wrappers put it behind one more inlining level and cost 3-4% on pipe-to.js highWaterMarkW=1.

The new test covers a sink whose writes resolve from another context, a push source under backpressure, pipeThrough() with pulls resolved from another context, and a chain into a slow sink. Expectations match v24.18.0 (with and without --no-async-context-frame) and v26.5.1; the first three cases fail on main. It checks only the pulls and writes the pump drives: the close()/flush() contexts come from the pipe's close watcher, which #67052 fixes separately, so this PR does not depend on it.

Benchmarks (pipe-to, pipe-through, lifecycle, adapters, tee; 20 runs) are within ±2% in both directions (pipe-through transform +2.2% *, pipe-to 4096/1024 -2.0% *** which measured -0.1% in the previous run of the same code).

The leak shipped with #65138 in v24.21.0 and v26.10.0, so this should be backported to both lines.

Refs: #66906
Refs: #65138
Refs: #67052

🤖 Generated with Claude Code

https://claude.ai/code/session_01Sfj4nCwaLmXB1NxA7sEJfi

The pipeTo() pump re-enters from the destination's ready hook, which
writableStreamUpdateBackpressure invokes synchronously, and from the
chunk steps of its read request. Both run in the async context of the
code that cleared the backpressure or enqueued the chunk, so the
source's pull() and the writes issued after a resumption observed that
context instead of the one of the pipeTo() call, as they did with the
promise-based pump. Capture the async context frame when the pipe starts
and run the pump in it.

Refs: nodejs#66906
Refs: nodejs#65138
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams Issues and PRs related to the Web Streams API. labels Oct 27, 2026
@codecov

codecov Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.64%. Comparing base (29bd740) to head (7047a9e).
⚠️ Report is 220 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #67054      +/-   ##
==========================================
+ Coverage   90.47%   90.64%   +0.17%     
==========================================
  Files         791      797       +6     
  Lines      277245   278296    +1051     
  Branches    53310    53515     +205     
==========================================
+ Hits       250834   252273    +1439     
+ Misses      16811    16400     -411     
- Partials     9600     9623      +23     
Files with missing lines Coverage Δ
lib/internal/webstreams/readablestream.js 98.12% <100.00%> (+0.02%) ⬆️

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

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 27, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 27, 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

needs-ci PRs that need a full CI run. web streams Issues and PRs related to the Web Streams API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants