Skip to content

doc: stabilize AsyncLocalStorage methods #58019

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vdeturckheim
Copy link
Member

Mark AsyncLocalStorage#enterWith and AsyncLocalStorage#disable as stable

Following discussions with @mcollina , I don't remember fully why these two methods are now stable.

They might be confusing for the end user to know which context they are in but now that async context tracking is more well known, I don't think this is so much of a big problem?

Mark AsyncLocalStorage#enterWith and AsyncLocalStorage#disable as stable
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 25, 2025
@Flarna
Copy link
Member

Flarna commented Apr 25, 2025

As far as I remember it was not marked as stable for following reasons:

  • enterWith was seen as "dangerous", prone to result in leaks
  • The TC39 proposal for AsyncContext doesn't include them for the same reason
  • CloudFlare Worker didn't add them for the same reason (@jasnell please correct me if I'm wrong)

Having these APIs stable and getting native JS support for AsyncContext without it would likely result in adoption problems.

@Flarna Flarna added the async_local_storage AsyncLocalStorage label Apr 25, 2025
@@ -215,7 +215,7 @@ added:
- v12.17.0
-->

> Stability: 1 - Experimental
> Stability: 2 - Stable
Copy link
Member

Choose a reason for hiding this comment

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

I think we should mention the version when it was moved from experimental to stable in the YAML section above.
see e.g. AsyncLocalStorage.snapshot() as reference.

@mcollina
Copy link
Member

We can't remove them either because they are needed for the instrumentation/apm use case as well as many frameworks. You can see examples in: nodejs/standards-positions#2 (comment).

We should mark those as stable, or remove them entirely if an alternative can be identified.

@vdeturckheim
Copy link
Member Author

Thanks for the context @Flarna !
Do you think that documentation warnings would be enough in 2025 to let the user know they need to worry about leaks?

It is indeed dangerous, but I agree with @mcollina 's point here, it's needed or we should look for an alternative.

@Flarna
Copy link
Member

Flarna commented Apr 25, 2025

Sorry, my intention was not to block this. I was in favor of a simple set API from beginning but that time there was a lot push back.

I guess the best extension to run would be via using (Refs: tc39/proposal-async-context#60)

What I still don't like is the name - enterWith(newCtx) sounds like a matching exit() fits but actually needed is a enterWith(prevCtx). But well, renaming this was already too late a few years ago (see #45373).

Another question: Any reason why AsyncLocalStorage.exit() is kept experimental?
The current AsyncContextFrame based implemenation is just a wrapper around run(undefined, ...).
The previous, resource based implemenation is a bit different as it first disables and afterward enabled ALS. Maybe calling disable() followed by enterWith() would do the job.
Or is that an API we should actually removed?

While writing this I remembered that disable() acts different between current and previous implemenation (refs: #48528 (comment)) which indicates stability is not that good.

@vdeturckheim
Copy link
Member Author

Sorry, my intention was not to block this.

No worried, I did not read it this way, also, I opened this PR to re-open the discussion!

I like the idea of using using here tbh, seems to be an elegant solution.

cc @legendecas

@legendecas
Copy link
Member

legendecas commented Apr 25, 2025

using support on AsyncContext just reached to stage 1 at https://github.com/tc39/proposal-async-context-disposable.

Comparing to run, enterWith has to be used with care, either due to how it is implemented with lazy-initialization (#53037), or due to its semantics with async function before first await like:

async function foo() {
  als.enterWith(newValue);
  await 0;
}

If enterWith is promoted to stable, I think it'd be better to initialize AsyncLocalStorage eagerly to fix issues like #53037. To kick off the work, I submitted #58029.

It's fair to say that an experimental API has been used a lot and it should be promote to stable, but it should also be documented in which case it will break as caveats.


That's been said, I think AsyncLocalStorage.prototype.disable() should be separated from this PR. It's use cases are not the same with enterWith and I think it is more error proven to cause more issues like #53037.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_local_storage AsyncLocalStorage doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants