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.25%. Comparing base (c7b0dfb) to head (fd7f7a4).
⚠️ Report is 163 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59873      +/-   ##
==========================================
+ Coverage   88.23%   88.25%   +0.01%     
==========================================
  Files         703      703              
  Lines      207393   207401       +8     
  Branches    39888    39885       -3     
==========================================
+ Hits       183003   183043      +40     
+ Misses      16346    16315      -31     
+ Partials     8044     8043       -1

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

... and 27 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!

Qard · 2025-09-19T08:15:34Z

Marking as server-major just to make sure we don't accidentally land this in a minor with behaviour breakage.

See: #59873 (comment)

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.

mcollina

lgtm

mcollina · 2025-09-25T09:49:09Z

cc @Flarna

mcollina · 2025-09-25T09:49:38Z

@Qard are you still sure this is a major?

nodejs-github-bot · 2025-09-25T09:52:56Z

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

Qard · 2025-09-25T09:58:49Z

Well, I suppose async_hooks is technically still marked experimental, so we can consider it non-breaking. It definitely does break async_hooks slightly though as this will change the model to not trigger later events if the init was missed. With AsyncLocalStorage not using it anymore, I don't really care so much for that use case, but others still using async_hooks might care. Though, in my opinion, other events triggering despite earlier ones being missed has been as much a footgun for people, so... 🤷🏻

avcribl · 2025-10-02T03:10:02Z

@mcollina Is there any update for my PR? Thanks!

avcribl · 2025-10-08T19:08:45Z

@mcollina Is there any update for my PR? Thanks!

@mcollina It's a gentle reminder if I need to do anything else for this PR? Thank you!

nodejs-github-bot · 2025-10-09T00:00:00Z

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

lemire · 2025-10-09T00:00:59Z

I restated the tests. Hoping that they will be green.

avcribl · 2025-10-10T04:35:41Z

I restated the tests. Hoping that they will be green.

Thanks @lemire! Some of the tests are flaky, would you please trigger the tests again? Thanks!

nodejs-github-bot · 2025-10-10T11:42:49Z

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

nodejs-github-bot · 2025-10-11T16:19:58Z

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

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 added 6 commits September 18, 2025 15:30

stream: preserve AsyncLocalStorage on finished only when needed

62aefac

benchmark: added finished benchmark

a432265

removed async_hooks

6b25211

modified the benchmark test

5453b2f

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

a28f0fb

added unit tests

6fa016f

avcribl added 3 commits September 18, 2025 15:30

bumped the number of iterations

610be5b

fixed lint issues

3173d55

expanded on the unit test

fd7f7a4

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

mcollina approved these changes Sep 25, 2025

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2025

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

lemire approved these changes Oct 8, 2025

View reviewed changes

		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

Are you sure you want to change the base?

stream: improve performance of finished() #59873

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!

Qard commented Sep 19, 2025

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Sep 25, 2025

Uh oh!

mcollina commented Sep 25, 2025

Uh oh!

nodejs-github-bot commented Sep 25, 2025

Uh oh!

codecov bot commented Sep 13, 2025 •

edited

Loading

avcribl Sep 17, 2025 •

edited

Loading