Skip to content

async_hooks: add use() method to AsyncLocalStorage - #58104

Closed
bengl wants to merge 1 commit into
nodejs:mainfrom
bengl:bengl/als-use
Closed

async_hooks: add use() method to AsyncLocalStorage#58104
bengl wants to merge 1 commit into
nodejs:mainfrom
bengl:bengl/als-use

Conversation

@bengl

@benglbengl commented May 1, 2025

Copy link
Copy Markdown
Member

This provides a way to use the using syntax (when available) to manage AsyncLocalStorage contexts, as an alternative to run().

This provides a way to use the `using` syntax (when available) to manage
AsyncLocalStorage contexts, as an alternative to `run()`.
@nodejs-github-botnodejs-github-bot added async_local_storage AsyncLocalStorage needs-ci PRs that need a full CI run. labels May 1, 2025
@codecov

codecovBot commented May 1, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.75510% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.21%. Comparing base (102d8cf) to head (b0bc07a).
⚠️ Report is 2036 commits behind head on main.

Files with missing linesPatch %Lines
lib/internal/async_local_storage/async_hooks.js80.64%6 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #58104 +/- ##
=======================================
Coverage 90.20% 90.21% =======================================
Files 630 630 Lines 186446 186495 +49 Branches 36622 36629 +7 =======================================
+ Hits 168189 168246 +57 - Misses 11051 11058 +7 + Partials 7206 7191 -15 
Files with missing linesCoverage Δ
...nternal/async_local_storage/async_context_frame.js100.00% <100.00%> (ø)
lib/internal/async_local_storage/async_hooks.js94.94% <80.64%> (-3.02%)⬇️

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

@benglbengl added the request-ci Add this label to start a Jenkins CI on a PR. label May 1, 2025
@github-actionsgithub-actionsBot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels May 1, 2025
@github-actions

Copy link
Copy Markdown
Contributor
Failed to start CI
 ⚠ No approving reviews found
✘ Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/14782552537

@legendecaslegendecas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like #52065, i think this does not work with an async function before the first await that the value will leak out the async function scope. This will need a patch to make V8 CPED aware of async function scopes as well.

asyncfunctionfoo(){usingvalue=als.use(newValue);await0;}foo()// no awaital's.getStore()// value changed.

<!-- YAML
added: REPLACEME
-->

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intended to be added as stable ?
Otherwise add > Stability: 1 - Experimental here.

* `store` {any}
* Returns: {Disposable} A disposable object.

Transitions into the given context, and transitions back into the previous

@FlarnaFlarnaMay 2, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not the previous context. It's that one seen at the time use() was called.
If use and the matching dispose are called in right order this is the same but if they are called in different order it's not.

e.g. a cast like this (condensed, in real world this would be distributed at independent locations):

const{ AsyncLocalStorage }=require("node:async_hooks");constals1=newAsyncLocalStorage();constals2=newAsyncLocalStorage();constd1=als1.use(store1);constd2=als2.use(store2);constd3=als1.use(store3);als1.getStore();// Returns store3als2.getStore();// Returns store2d1[Symbol.dispose]();als1.getStore();// Returns undefinedals2.getStore();// Returns undefinedd2[Symbol.dispose]();als1.getStore();// Returns store1als2.getStore();// Returns undefinedd3[Symbol.dispose]();als1.getStore();// Returns store1als2.getStore();// Returns store2

Likely a user problem and once using is a thing harder to do.
But I think it should be clear documented what happens or avoid such cases.

In special that one ALS user might effect others is critical in my opinion.

Note that only the AsyncContextFrame variant shows side effects to all ALS users.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if folks are manually calling Symbol.dispose they're rather on their own at that point. It's a bit unfortunate that the ERM model gives them that ability but there's only do much we can do.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, we'll want to be sure that this works as expected when using DisposableStack when that is available.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fully agree once using keyword is a thing. But until this is true users have to do it manually.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notices that the side effect to other ALS instances is not bound to wrong usage of dispose:

constd1=als1.use("store1");als2.enterWith("store2");console.log(als1.getStore());// store1console.log(als2.getStore());// store2d1[Symbol.dispose]();console.log(als1.getStore());// undefinedconsole.log(als2.getStore());// undefined, but should be store2

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that this global side effect is not new in the AsyncContextFrame variant. See #58149.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use() is intended to roughly mimic run(), and since that's been changed in #58149 to isolate the storage instances, I'll do that here as well in the next rev.

#oldFrame = undefined;

constructor(store, storage) {
this.#oldFrame = AsyncContextFrame.current();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess by capturing/restoring only the owned store instead the complete frame the side effects to other ALS users could be avoided.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, an operation on an AsyncLocalStorage should not affect other instances. For reference: tc39/proposal-async-context-disposable#2

@jasnelljasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be marked as experimental but otherwise lgtm

function inner() {
// Once `using` syntax is supported, you can use that here, and omit the
// dispose call at the end of this function.
const disposable = asyncLocalStorage.use(store);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using works meanwhile on main (see #58154) therefore this should be updated accordingly.

@vdeturckheim

Copy link
Copy Markdown
Member

🤦 -> closing #58019 in favor of this

@vdeturckheimvdeturckheim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🫶 this

@bengl

bengl commented May 7, 2025

Copy link
Copy Markdown
MemberAuthor

@legendecas

Like #52065, i think this does not work with an async function before the first await that the value will leak out the async function scope. This will need a patch to make V8 CPED aware of async function scopes as well.

Do you see that as a blocker? Or would it just be fine to document this limitation for now?

@legendecas

legendecas commented May 8, 2025

Copy link
Copy Markdown
Member

Do you see that as a blocker? Or would it just be fine to document this limitation for now?

I would not see it as a blocker on this particular PR. It would be great to have this PR landed with this API marked as an experimental (1.1 Active development) API and with the limitation documented as a caveat. I think to promote this to stable, we'll need this semantic as a blocker.

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@FlarnaFlarna left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as there are already approvals but there are still important todos I clearly mark it so to avoid accidental merging.

@devsnek

Copy link
Copy Markdown
Member

I never got around to merging this but I have a fix for the CPED issue: https://chromium-review.googlesource.com/c/v8/v8/+/6020635

@BridgeAR

Copy link
Copy Markdown
Member

@devsnek do you think you could find some time to pick it up again and finish that? :)

@bengl

Copy link
Copy Markdown
MemberAuthor

Closing in favour of #61674

@benglbengl closed this Feb 11, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

async_local_storageAsyncLocalStorageneeds-ciPRs that need a full CI run.request-ci-failedAn error occurred while starting CI via request-ci label, and manual interventon is needed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@bengl@vdeturckheim@legendecas@devsnek@BridgeAR@mcollina@jasnell@Flarna@nodejs-github-bot