stream: improve performance of finished() #59873

avcribl · 2025-09-12T20:31:29Z

The changes optimize the end-of-stream.js module to only preserve AsyncLocalStorage context when it's actually needed, improving performance by avoiding ALS overhead in the common case where no async context tracking is active.
The performance test results with this patch:

./out/Release/node ./benchmark/streams/end-of-stream.js
streams/end-of-stream.js streamType="readable" n=100000: 3,290,307.950450068
streams/end-of-stream.js streamType="writable" n=100000: 1,004,954.4253168119
streams/end-of-stream.js streamType="duplex" n=100000: 893,658.706842607

The same perf test results with the current nodejs:

node ./benchmark/streams/end-of-stream.js
streams/end-of-stream.js streamType="readable" n=100000: 385,512.31905964843
streams/end-of-stream.js streamType="writable" n=100000: 348,685.1671624238
streams/end-of-stream.js streamType="duplex" n=100000: 382,477.79857896036

You can compare and see the noticeable performance improvement with this patch.

nodejs-github-bot · 2025-09-12T20:31:35Z

Review requested:

@nodejs/performance
@nodejs/streams

codecov · 2025-09-13T02:03:48Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.57%. Comparing base (657428a) to head (d51ca0d).
⚠️ Report is 90 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59873      +/-   ##
==========================================
+ Coverage   88.56%   88.57%   +0.01%     
==========================================
  Files         704      704              
  Lines      208322   208330       +8     
  Branches    40027    40040      +13     
==========================================
+ Hits       184501   184533      +32     
+ Misses      15863    15818      -45     
- Partials     7958     7979      +21

Files with missing lines	Coverage Δ
lib/internal/streams/end-of-stream.js	`97.41% <100.00%> (+0.06%)`	⬆️

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

H4ad

Hey, thanks for the PR, I added a few comments about some issues I found running locally.

Also, would you mind split this change in two commits, the first one being the benchmark and the second one the optimization?

You can also improve the commit message a little bit by adding what you really changed to improve performance, eg: stream: preserve AsyncLocalStorage on finished only when needed

About the benchmark name, maybe changing to finished should be more relevant than end-of-stream since we are testing the finished function.

Preview of the results:

h4ad:node-copy-4/ (improve-stream-finished-perf✗) $ node-benchmark-compare streams.csv                                                                                                                                             [10:49:03]
                                                        confidence improvement accuracy (*)   (**)  (***)
streams/compose.js n=1000                                      ***     42.25 %       ±3.90% ±5.24% ±6.91%
streams/end-of-stream.js streamType='readable' n=100000        ***     76.67 %       ±1.41% ±1.89% ±2.48%
streams/end-of-stream.js streamType='writable' n=100000        ***    105.00 %       ±2.01% ±2.69% ±3.56%
streams/pipe-object-mode.js n=5000000                                  -0.30 %       ±0.81% ±1.08% ±1.42%
streams/pipe.js n=5000000                                               0.14 %       ±1.77% ±2.36% ±3.08%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 5 comparisons, you can thus expect the following amount of false-positive results:
  0.25 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.05 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)

benchmark/streams/end-of-stream.js

mcollina

While I love the PR, but I fear that this breaks continuation when using async_hooks.

We could argue that with ALS not using async_hooks we could remove them (or add them via a compile time flag) but that's outside of the scope of this PR.

avcribl · 2025-09-13T16:27:33Z

While I love the PR, but I fear that this breaks continuation when using async_hooks.

We could argue that with ALS not using async_hooks we could remove them (or add them via a compile time flag) but that's outside of the scope of this PR.

Thanks @mcollina! I removed the async_hooks part.

krsjenswbp

thanks

Flarna · 2025-09-15T19:56:08Z

There is command line option --no-async-context-frame to disable use of AsyncContextFrame in ALS.

I think we might need special handing for this variation.

mcollina · 2025-09-15T20:36:32Z

@avcribl you didn't address my comment.

avcribl · 2025-09-15T20:42:05Z

@avcribl you didn't address my comment.

@mcollina Are you suggesting not to use AsyncLocalStorage ??= require('async_hooks').AsyncLocalStorage at all? I see you have an open PR here #59867
Would you please provide more hints as to what your suggestion is here? (This is my first contribution)

avcribl · 2025-09-16T02:05:01Z

@avcribl you didn't address my comment.

@mcollina I've pushed a new commit that adds a check to determine whether async_context_frame is enabled. I tested both scenarios:
when async_context_frame is enabled, and when it is disabled (falling back to async_hooks).

In both cases, the unit tests ran successfully and passed:

./out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /Users/amir/Documents/projects4/node/test/async-hooks/test-async-local-storage-stream-finished.js

./out/Release/node --no-async-context-frame --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /Users/amir/Documents/projects4/node/test/async-hooks/test-async-local-storage-stream-finished.js

Also, I ran the benchmark test with and without the flag and both have similar improved performance:

./out/Release/node ./benchmark/streams/finished.js
streams/finished.js streamType="readable" n=100000: 1,033,352.7517537934
streams/finished.js streamType="writable" n=100000: 1,383,377.7808003046

./out/Release/node --no-async-context-frame ./benchmark/streams/finished.js
streams/finished.js streamType="readable" n=100000: 1,044,042.0363062695
streams/finished.js streamType="writable" n=100000: 1,362,108.6226052495

Please let me know if it addresses your comment.

mcollina · 2025-09-16T07:54:35Z

I don't see any new tests in this PR, just a benchmark.

@Qard or @Flarna could chime in as well, but checking if AsyncContextFrame is enabled is not enough in case plain async_hooks are used. There are still a lot of modules out there that are not using AsyncLocalStorage for context tracking. My understanding is that using async_hooks.createHook() does not enable this block, breaking continuation.

Flarna · 2025-09-16T08:31:51Z

It's quite hard to tell. I would recommend to write tests for all these scenarios to be improved here and also verify that non improvable scenarios still work.

FWIW I'm not a big fan of tinkering with async hooks/async local store internals at several places in code.
Maybe using enabledHooksExist instead getHookArrays()[0].length would make it easier to understand.

Qard · 2025-09-16T09:56:39Z

lib/internal/streams/end-of-stream.js

+  if ((AsyncContextFrame.enabled && AsyncContextFrame.current()) ||
+      (!AsyncContextFrame.enabled && getHookArrays()[0].length > 0)) {


This could be simplified to:

Suggested change

if ((AsyncContextFrame.enabled && AsyncContextFrame.current()) ||

(!AsyncContextFrame.enabled && getHookArrays()[0].length > 0)) {

if (AsyncContextFrame.current() || enabledHooksExist()) {

The AsyncContextFrame.current() call is a no-op when inactive, you don't really need the enabled check as that will just add more instructions. Also, as @Flarna pointed out, enabledHooksExist() is the clearer way to check if there are active async_hooks listeners.

Thanks! I removed AsyncContextFrame.enabled

Qard · 2025-09-16T10:01:47Z

One thing worth noting is that this would be a breaking change as currently the AsyncLocalStorage.bind(...) will always bind, even if there are no hooks or stores in use, but async_hooks could start listening at any time. In the current model you could start listening after the bind call there but before the callback is called and you would see the events. After this change you would not.

avcribl · 2025-09-17T23:43:34Z

Thanks all for the feedback!
Following the suggestion to use enabledHooksExist(), I spent quite some time on it and gave that a try—but found that it consistently returned true, even when no async hooks were active. So, I reverted to using getHookArrays()[0].length > 0, as it provides a more accurate reflection of the current hook state.
I've also added additional unit tests to cover both code paths and validate the conditions across different scenarios.

mcollina

lgtm

mcollina · 2025-09-18T12:06:28Z

test/parallel/test-stream-finished-bindAsyncResource-path.js

+finished(readable, common.mustCall(() => {
+  strictEqual(internalAsyncHooks.getHookArrays()[0].length > 0,
+              true, 'Should have active user async hook');
+}));


Note that this does not verify continuation is preserved

mcollina · 2025-09-18T12:06:35Z

test/parallel/test-stream-finished-bindAsyncResource-path.js

+finished(readable, common.mustCall(() => {
+  strictEqual(internalAsyncHooks.getHookArrays()[0].length > 0,
+              true, 'Should have active user async hook');
+}));


Note that this does not verify continuation is preserved

This doesn't verify the code path either. It is a copy of the condition and someone might change one of the locations.

Thanks! I expanded on the test!

@mcollina I addressed your comment, thanks!

avcribl · 2025-10-15T18:46:21Z

I rebased my PR. Can I please get approval for kicking off the workflows? Thanks!

nodejs-github-bot · 2025-10-16T02:15:09Z

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

avcribl · 2025-10-20T16:48:17Z

Can the pipeline be triggered again? Still, some flaky tests fail, thank you!

nodejs-github-bot · 2025-10-20T17:10:58Z

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

avcribl · 2025-10-22T00:05:00Z

Can I get another run please? Thanks!

nodejs-github-bot · 2025-10-22T00:09:34Z

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

avcribl · 2025-10-22T18:01:01Z

CI is green now!

avcribl · 2025-10-27T15:55:53Z

The CI build is green, do I need to do anything more here? Thanks!

avcribl · 2025-10-27T18:58:32Z

Thanks @H4ad ! I think the label commit-queue needs to be added as well.

nodejs-github-bot · 2025-10-27T19:23:36Z

Landed in 4fe325d

nodejs-github-bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. labels Sep 12, 2025

jasnell approved these changes Sep 13, 2025

View reviewed changes

H4ad reviewed Sep 13, 2025

View reviewed changes

benchmark/streams/end-of-stream.js Show resolved Hide resolved

benchmark/streams/end-of-stream.js Outdated Show resolved Hide resolved

benchmark/streams/end-of-stream.js Outdated Show resolved Hide resolved

benchmark/streams/end-of-stream.js Outdated Show resolved Hide resolved

H4ad mentioned this pull request Sep 13, 2025

stream: use new AsyncResource instead of bind #59867

Merged

mcollina requested changes Sep 13, 2025

View reviewed changes

avcribl force-pushed the improve-stream-finished-perf branch from 56bc9ed to 2d4a3b5 Compare September 13, 2025 16:24

krsjenswbp reviewed Sep 14, 2025

View reviewed changes

avcribl force-pushed the improve-stream-finished-perf branch from 2d4a3b5 to e7d45d6 Compare September 16, 2025 01:43

Qard reviewed Sep 16, 2025

View reviewed changes

avcribl force-pushed the improve-stream-finished-perf branch from e7d45d6 to 51e0905 Compare September 17, 2025 22:56

mcollina approved these changes Sep 18, 2025

View reviewed changes

mcollina requested changes Sep 18, 2025

View reviewed changes

avcribl force-pushed the improve-stream-finished-perf branch from ff9809b to fd7f7a4 Compare September 18, 2025 22:50

avcribl added 8 commits October 15, 2025 05:08

benchmark: added finished benchmark

f5595c6

removed async_hooks

66dd3f6

modified the benchmark test

48db583

added check if no-async-context-frame is enabled or not

8e5b4a2

added unit tests

2d55f08

bumped the number of iterations

a7f49d3

fixed lint issues

c49b350

expanded on the unit test

d51ca0d

avcribl force-pushed the improve-stream-finished-perf branch from fd7f7a4 to d51ca0d Compare October 15, 2025 12:09

anonrig approved these changes Oct 16, 2025

View reviewed changes

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2025

gurgunday added the performance Issues and PRs related to the performance of Node.js. label Oct 27, 2025

H4ad added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Oct 27, 2025

H4ad added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 27, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 27, 2025

nodejs-github-bot merged commit 4fe325d into nodejs:main Oct 27, 2025
72 checks passed

gurgunday mentioned this pull request Nov 30, 2025

process: preserve AsyncLocalStorage on queueMicrotask only when needed #60913

Open

		if ((AsyncContextFrame.enabled && AsyncContextFrame.current()) \|\|
		(!AsyncContextFrame.enabled && getHookArrays()[0].length > 0)) {

	if ((AsyncContextFrame.enabled && AsyncContextFrame.current()) \|\|
	(!AsyncContextFrame.enabled && getHookArrays()[0].length > 0)) {
	if (AsyncContextFrame.current() \|\| enabledHooksExist()) {

Uh oh!

stream: improve performance of finished() #59873

stream: improve performance of finished() #59873

Uh oh!

Conversation

avcribl commented Sep 12, 2025

Uh oh!

nodejs-github-bot commented Sep 12, 2025

Uh oh!

codecov bot commented Sep 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

H4ad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

avcribl commented Sep 13, 2025

Uh oh!

krsjenswbp left a comment

Choose a reason for hiding this comment

Uh oh!

Flarna commented Sep 15, 2025

Uh oh!

mcollina commented Sep 15, 2025

Uh oh!

avcribl commented Sep 15, 2025

Uh oh!

avcribl commented Sep 16, 2025

Uh oh!

mcollina commented Sep 16, 2025

Uh oh!

Flarna commented Sep 16, 2025

Uh oh!

Qard Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

avcribl Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Qard commented Sep 16, 2025

Uh oh!

avcribl commented Sep 17, 2025

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

mcollina Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Flarna Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

avcribl Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

avcribl Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

avcribl commented Oct 15, 2025

Uh oh!

nodejs-github-bot commented Oct 16, 2025

Uh oh!

avcribl commented Oct 20, 2025

Uh oh!

nodejs-github-bot commented Oct 20, 2025

Uh oh!

avcribl commented Oct 22, 2025

Uh oh!

nodejs-github-bot commented Oct 22, 2025

codecov bot commented Sep 13, 2025 •

edited

Loading

avcribl Sep 17, 2025 •

edited

Loading