async_hooks: add AsyncLocal API mk2

[puzpuzpuz]

• Depends on async_hooks: add executionAsyncResource #30959. See the last commit to understand what this PR changes
  • As async_hooks: add executionAsyncResource #30959 was merged, this PR is ready for review
• Combines ideas for async_hooks: add AsyncLocal class #27172 and async-hooks: introduce async-storage API #26540. I believe that this PR combines strong points of those implementation and builds a solid foundation for further evolution
• Includes tests and the benchmark (see changes in benchmark/async_hooks/async-resource-vs-destroy.js)

Introduces new AsyncLocal API to provide capabilities for building continuation local storage on top of it.

The implementation is based on async hooks (hook callbacks + executionAsyncResource()), but async hooks are not exposed in public API in any way. Public API is inspired by ThreadLocal class in Java.

I've marked @Flarna as a co-author, as the idea of this PR came out of #27172.

FYI @Quard, @Flarna, @vdeturckheim

Design overview

Main properties of the new API:

• Both API and implementation are very simple (.get()/.set() + optional .destroy() methods)
  • This API achieves the same feature set as any other CLS API. Moreover, as it's quite "low-level", it's possible to build more opinionated APIs on top of AsyncLocal, say, continuation-local-storage-like API or AsyncContext (async-hooks: introduce async-storage API #26540). See sample snippets from the following comment: Executive summary: Introducing a CLS-like API to Node.js core TSC#807 (comment)
• It's relatively safe (memory-wise):
  • Does not depend on destroy hook. Thus, misbehaving AsyncResource won't be able to lead to mem leaks
  • .destroy() method disables the hook callback and frees all values from memory. Thus, it's possible to remove all remains of the AsyncLocal
• Has copy-on-write semantics, i.e. setting a new value branches subsequent parts of the tree
• Performance should be at least as good as for other implementations

Benchmarks against alternatives

I have modified async-resource-vs-destroy.js benchmark and compared proposed implementation with #26540. Here is the result that I got on my machine:

$ ./node benchmark/async_hooks/async-resource-vs-destroy.js benchmarker=autocannon
async_hooks/async-resource-vs-destroy.js n=1000000 method="callbacks" type="async-local" benchmarker="autocannon": 20,277.6
async_hooks/async-resource-vs-destroy.js n=1000000 method="async" type="async-local" benchmarker="autocannon": 15,877.6
async_hooks/async-resource-vs-destroy.js n=1000000 method="callbacks" type="async-context" benchmarker="autocannon": 16,922.41
async_hooks/async-resource-vs-destroy.js n=1000000 method="async" type="async-context" benchmarker="autocannon": 11,841.2
async_hooks/async-resource-vs-destroy.js n=1000000 method="callbacks" type="async-resource" benchmarker="autocannon": 23,582.4
async_hooks/async-resource-vs-destroy.js n=1000000 method="async" type="async-resource" benchmarker="autocannon": 18,407.2

As expected, AsyncLocal is significantly faster than AsyncContext, but slower than the sample implementation that stores values as resource object properties (async-resource type). As for the comparison with latter, I think that .destroy() method that frees all values from the memory is more valuable than certain performance gain.

Benchmarks against existing libraries

A performance comparison with cls-hooked v4.2.2 was also made. The benchmark is available here: https://gist.github.com/puzpuzpuz/f0b23458a821d7edab3738550e58f0e2 (again, it's a fragment of async-resource-vs-destroy.js).

Here is the result:

$ ./node benchmark/async_hooks/async-resource-vs-destroy.js benchmarker=autocannon type=cls-hooked
async_hooks/async-resource-vs-destroy.js n=1000000 method="callbacks" type="cls-hooked" benchmarker="autocannon": 13,299.6
async_hooks/async-resource-vs-destroy.js n=1000000 method="async" type="cls-hooked" benchmarker="autocannon": 10,580.4

With these results cls-hooked shows worst results among all candidates. But if both ns.bindEmitter calls are commented (which contradicts with library guidelines for middlewares), the result improves significantly:

$ ./node benchmark/async_hooks/async-resource-vs-destroy.js benchmarker=autocannon type=cls-hooked
async_hooks/async-resource-vs-destroy.js n=1000000 method="callbacks" type="cls-hooked" benchmarker="autocannon": 19,704.8
async_hooks/async-resource-vs-destroy.js n=1000000 method="async" type="cls-hooked" benchmarker="autocannon": 14,580

With this result, cls-hooked is on par with AsyncLocal.

Possible enhancements

1. Increase performance by storing values in resources directly instead of an internal WeakMap

That should bring AsyncLocal's performance to async-resource's level (see benchmark results above). The downside is that it won't be possible to free values in all reachable resources in .destroy() method anymore, so this method will provide weaker guarantees after the change.

2. Introduce a global registry instead of public constructor

With current implementation, if a strong reference to AsyncLocal is destroyed in application code, a strong reference in async_hooks will still remain and the value propagation will continue to be executed. Thus, the instance will be effectively leaked.

Note: the same consideration also applies to async_hooks.createHook. Once the user-land reference to an AsyncHook is lost, it's not possible to disable it.

To make the API less error-prone AsyncLocal's local constructor could be kept private and createAsyncLocal(string)/destroyAsyncLocal(string) functions could be introduced to async_hooks. Each AsyncLocal would be identified by a string (the exact type of id is not that important) and multiple calls of createAsyncLocal with the same identifier would return the same AsyncLocal instance.

The main problem with this enhancement is user-land library isolation, i.e. library A shouldn't be able to retrieve library B's instance.

Alternatively, option 1 from comment #26540 (comment) can be considered. See this PR for draft implementation and benchmark results: https://github.com/vdeturckheim/node/pull/2

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[puzpuzpuz]

An interesting update.

I've implemented benchmark based on benchmark/async_hooks/async-resource-vs-destroy.js and compared this implementation with alternatives. See PR description for the result.

[puzpuzpuz]

@mcollina @benjamingr
You were interested in benchmarking #26540, so you might be interested in the recent update. For obvious reasons, I didn't push the benchmark for that implementation into this PR (all other benchmarks are present in async-resource-vs-destroy.js in here). But I've shared it as a gist: https://gist.github.com/puzpuzpuz/f0b23458a821d7edab3738550e58f0e2

[mcollina]

I really like this implementation.

[puzpuzpuz]

As an optimization suggested by @Qard (see nodejs/diagnostics#345 (comment)), async hook per AsyncLocal was merged into a single global hook.

Public API and behavior of AsyncLocal didn't change.

[puzpuzpuz]

Another interesting update.

I've compared performance with cls-hooked, one of the most popular CLS libraries for Node.js. See PR description for the result.

[Qard]

This would set a barrier between the current execution layer and its parent, discarding the upper value and only propagating the new one downward through the tree. That's not necessarily a bad thing, but a user could very easily make the mistake of calling set again somewhere further into the request thinking it would allow a different path through the request to access the value, which is not the case. In my experience, it's better for the barrier to be applied in the constructor that way the object can't unexpectedly fork part-way through a request. Rather than constructing one AsyncLocal instance at the top-level, you'd construct a new one at the start of each request.

I know there's also the use-case of enabling isolated components of a system to share data without explicit communication, and I think the best way to achieve that is just using a factory that can be called to produce or retrieve AsyncLocal instances. In a way, you kind of already have this with the set and get, but I feel the naming is misleading and should be made clearer that set is forking the following async tree to use the new context object.

[puzpuzpuz]

Rather than constructing one AsyncLocal instance at the top-level, you'd construct a new one at the start of each request.

With this API such usage should be considered a bad practice, because of performance reasons and tricky API manipulations. By the latter I mean that it's hard to share individual AsyncLocal instance with each request. It's much simpler to use a single instance to handle all requests. See this example: https://github.com/nodejs/node/pull/31016/files#diff-9a4649f1c3f167b0da2c95fc38953a1fR485

As for merging values or doing any other actions on value modification, that could (and, in my opinion, should) be done in user-land, i.e. in application code or libraries, because exact logic depends on particular use case.

In a way, you kind of already have this with the set and get, but I feel the naming is misleading and should be made clearer that set is forking the following async tree to use the new context object.

I think that .get()/.set(value) method names are quite intuitive for anyone who is familiar with the concept of asynchronous call chain. To help users, PR adds a couple of examples showing how values are propagated into the documentation.

For me, alternative method names, like .read()/.fork(value) or something like that are much less obvious.

[Qard]

I feel the opposite. The .get() and .set(value) names are not intuitive at all to someone trying to use the API to solve the need of having a unique context object per-request. It's capable of achieving that by using .set(value) at the start of the request to store an object and then only using .get() to retrieve and modify that object for the rest of the request, but that's very non-obvious from the naming or the docs. The only reason it makes sense to us is because we are already intimately familiar with the concept of async call trees and how this feature is implemented. An average user is not, and I'm almost 100% certain users will try to use this like ThreadLocal in Java and then be totally confused when different parts of their app get different data because they keep calling .set(value) to "update" the stored value; it looks a lot like setState(...) in React, and I suspect many people will treat it as such. I'm sure many users will also try to construct a new AsyncLocal within each request as that's a common practice with ThreadLocal in Java, again doing it the wrong way here.

In my opinion, a proper async context API should look something more like this:

// This corresponds roughly to the existing `AsyncLocal`
const contexts = new ContextManager()

function loadUser(auth) {
  const { onError, onUser } = contexts.current
  User.login(auth).then(onUser, onError)
}

function httpResponder(res) {
  contexts.current.onError = err => {
    res.status(500).end(err.message)
  }
  contexts.current.onUser = user => {
    res.status(200).end(user.token)
  }
}

app.post('/login', (req, res) => {
  // This corresponds to `.set(value)` and is closest
  // to what most people actually think of as a local.
  contexts.create()
  httpResponder(res)
  loadUser(req.headers.authorization)
})

It's functionally identical to what you have built, but more cleanly communicates the intent of the objects and methods.

[puzpuzpuz]

The snippet looks like something that can be built on top of AsyncLocal API, but personally I find it less straightforward. AsyncLocal API, in its current state, allows building APIs on top of it, say, the one from the snippet or something similar to cls-hooked/continuation-local-storage API. So, I expect it to be mainly used by library authors, not all node users.

As for user confusion, I hope that it can be mitigated by good enough documentation.

[Qard]

I don't really like the manual cleanup required here. If someone decides to make a unique AsyncLocal object per-request it may be ambiguous where to close the object and they might even not close it at all, leading to a memory leak. It'd be better to use a WeakSet so you don't have to worry about explicit cleanup.

[puzpuzpuz]

WeakSet won't do here, as it doesn't allow iteration over its items. A Set with WeakRefs + FinalizationGroup API could potentially help to achieve automatic AsyncLocal clean up, but FG API is experimental and has to be enabled via a flag.

On the other hand, other CLS API implementations do not provide a way to free underlying resources and disable hooks. That's because most applications use a single instance of CLS without getting rid of it. So, AsyncLocal.remove() method is an advanced API, useful in specific scenarios.

WDYT?

[Qard]

There's ways to eliminate the global Set entirely by making the WeakMap global and propagating on that directly.

[puzpuzpuz]

A global WeakMap used to store values won't allow to isolate different AsyncLocals, which kills the whole idea.

If you mean a WeakMap that stores <AsyncLocal, WeakMap> pairs, then it's not possible to iterate over WeakMap entries.

[Qard]

I mean something like WeakMap<AsyncResource, Set<AsyncLocal>>. The hooks can loop over the set of AsyncLocal instances at the current level and propagate them downwards. It means an array at each level, rather than just the top-level, but it also cleans up automatically.

[puzpuzpuz]

That's an interesting idea. Another level of indirection with a WeakMap<AsyncResource, Set<AsyncLocal>> will help to clean up both values and AsyncLocals list in the hook. As downsides I can see the following:

• The hook will have to be kept always enabled, once the first AsyncLocal instance was created. Currently .remove() disables the hook if there are no active AsyncLocals.
• This change will probably have a certain performance impact. But that has to be measured.

In general, it sounds like a nice idea for an experiment. I'm going to do that experiment, if this PR gets some chances to be reviewed and merged (I understand that chances are close to zero, especially considering #30959 (comment)).

[puzpuzpuz]

Closing this PR in favor of #26540, as the decision is still pending and I believe that having one or another CLS API in core in the nearest future is more important than having this particular one.

Going to port AsyncLocal into user-land once #30959 lands.

[puzpuzpuz]

As #31746 was created recently, it makes no sense to have this PR closed. So, I'm reopening it.

[puzpuzpuz]

@Flarna
I've marked you as a co-author (Co-authored-by clause in the commit message), as the idea of this PR came out of #27172.

I hope you don't mind. Sorry for not doing this initially.

[Flarna]

@puzpuzpuz No problem. I don't care much about my authorship and this PR had already a link to the previous PR.
The important point for me is that people starting to review now have a chance to get the full history - which is quite long meanwhile.

[puzpuzpuz]

@Flarna, @mcollina
You had shown some interest in this PR in the past, when it was blocked by #30959. Now, as it's unblocked, would it be too much to ask you to review it?

[puzpuzpuz]

@Flarna looks like I have addressed all of your points. Could you do another review round?

[puzpuzpuz]

Closing as #26540 has landed