Repository navigation
AsyncLocalStorage without Async Hooks #46265
Description
Activity
tl;dr I'm +1 on this
- How AsyncLocalStorage works under the hood is a detail, the goal was/is to have an API solving most of the real world use cases with minimal needs to know anything about how node/the event loop/promises work
- if we can make it faster, we should (optimising the bad
Mappart has been on my backlog for way too long
Bit more context about the APIs that would go in this case:
als.run(...)was designed to be the safe thing to use as it does not have a lot of corner casesals.enterWith(...)was requested for the APM use case: in term of instrumentation, it might be challenging to inject a callback or a promise at certain point. Could we still have a way to handle this? (cc @bengl @Flarna who have been doing much more instrumentation than me in the past couple years)als.disable()is mostly here to give a way out if there is a performance problem, if we are very confident about it, I think it's not a big deal to remove
Side node, as of today, only
runandAsyncResourceare stable so the scope of the proposed changes is technically acceptable.enterWith()can be usually replaced byrunif one instruments by monkeypatching/wrapping a function. It adds overhead of one more closure but that's likely acceptable.
For cases where one just gets an event/notification that a thing has started and one more that it has ended but the code inbetween is inaccessible for instrumentation requires to useenterWith(). It's not that common and can be solved on the caller side but not all libraries like to adapt their code for tracing.Do you have already an idea how to integrate this into non promise use cases where
SetContinuationPreservedEmbedderData()is of no help?- Any async resources that are created while that function is running ends up receiving a full copy of that map
Could you point to the location where a map is copied? actually only a reference to the store should be set on new resources (assuming here the store in an object not e.g. a
string).There is a built-in v8 API
v8::Context::SetContinuationPreservedEmbedderData()that can do the propagation for us much more efficiently.Is there some documentation available for this? In v8 headers I see only
Sets a value that will be stored on continuations and reset while the continuation runs.which is not really enough for me to understand what this is actually doing.Reacted by Vladimir de TurckheimI'm +1 on reducing the overhead of AsyncLocalStorage to minimum. It would be a massive progress for Node.js.
I'm worrying about potential breakages. Unless you want to put the new implementation behind a flag, the best time to land this semver-major change is for v21, so we have ~1 year to iron out all the bugs before it goes into a LTS line.
I'd be fine with putting the new implementation behind a flag. Doing so might make the implementation a bit trickier with keeping both models implemented simultaneously but the risk of breakage is non-trivial.
Reacted by Matteo CollinaDo you have already an idea how to integrate this into non promise use cases where SetContinuationPreservedEmbedderData() is of no help?
Yes, that's the part that is fairly complicated. In workerd we do this by capturing a counted reference to the current async context and setting the scope before entering the relevant callbacks, much like we do with the internal callback scope in Node.js. The advantage we have in workerd currently is that we have a much smaller set of async resource types so it's pretty simple. We'd effectively do the same with the new Node.js implementation but I still have to work out all of the implementation details.
Is there some documentation available for this? In v8 headers I see only Sets a value that will be stored on continuations and reset while the continuation runs. which is not really enough for me to understand what this is actually doing.
Unfortunately no, the documentation for this API is severely lacking. I'm hoping the v8 folks will expand the documentation on this relatively soon.
als.enterWith(...)was requested for the APM use case: in term of instrumentation, it might be challenging to inject a callback or a promise at certain point. Could we still have a way to handle this?
It's easy to implement.
Do you have already an idea how to integrate this into non promise use cases where
SetContinuationPreservedEmbedderData()is of no help?This is a side-effect of V8 not fully implementing the spec's
HostMakeJobCallbackandHostCallJobCallback. All jobs are supposed to preserve the continuation data, but V8 has only preserved it for Promise jobs. If V8 fully implemented, Node could use these two hooks for all queueing behaviors.In v8 headers I see only Sets a value that will be stored on continuations and reset while the continuation runs. which is not really enough for me to understand what this is actually doing.
In the spec, these are the equivalent to exposing the
[[HostDefined]]slot on JobCallback records. Related the previous answer, V8 is only carrying this[[HostDefined]]forward for PromiseJobs.If you're comfortable with code splunking, the relevant links are:
- PromiseFulfill/Reject (settling with a value): https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/promise-misc.tq;l=75-107;drc=39c3a97e848a7ecd1fa95e738771cc61d6d72552
- PromiseThen (adopting a thenable): https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/promise-misc.tq;l=271-287;drc=39c3a97e848a7ecd1fa95e738771cc61d6d72552
- Running promise jobs:https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/builtins-microtask-queue-gen.cc;l=191-327;drc=09769fa654c1c2d43005594163616ffbc51bc62f
- (my search: https://source.chromium.org/search?q=continuation_preserved_embedder_data%20-filepath:%5Eout%20-filepath:%5Egen)
That also points us to the unimplemented other job types:
Reacted by Ben NewmanIf I understand this correct the context seen in the
then/catchhandler would be that one active at the timethen()was called an no longer promise init.
Is this correct?For
awaitit should be the same. For common cases where a non async function creates and returns aPromiseand the caller immediatelly calls.then()it's also the same.If I understand this correct the context seen in the then/catch handler would be that one active at the time then() was called an no longer promise init.
Yes. It should be unobservably the same because the
.then()inits the new promise, so they'd have the same context.For
awaitit should be the sameCorrect.
For common cases where a non async function creates and returns a
Promiseand the caller immediatelly calls.then()it's also the same.Yup.
Testing with my implementation of this proposal, thenables are broken:
const ctx = new AsyncContext() const queue = []; const thenable = { then(onRes, _onRej) { queue.push("thenable: " + ctx.get()); onRes(); }, }; const out = ctx.run(1, () => { queue.push("new Promise"); const p = new Promise(res => res(thenable)); queue.push("p.then"); const p2 = p.then(() => thenable); queue.push("p2.then"); return p2.then(() => { queue.push("promise: " + ctx.get()); }); }); queue.push("out.then"); out.then(() => { queue.push("done"); //hook.disable(); console.log(queue); });
[ 'new Promise', 'p.then', 'p2.then', 'out.then', 'thenable: undefined', 'thenable: undefined', 'promise: 1', 'done' ]
Because it uses
EnqueueMicrotask, which doesn't preserve the context: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/promise-resolve.tq;l=189;drc=4c61bb3131b7951ed2ed896b4df6110b1e5c072fReacted by Ben NewmanI understand the goal here is to eliminate the dependence on
async_hooks. But I see some similarities between this proposal and nodejs/diagnostics#389.Reacted by Matteo Collina and James M SnellI would definitely be very supportive of such a change. :)
This overlaps a lot with the work I started in #42393 too, but I haven't had time to continue work on it. It's also worth noting that my changes for TracingChannel in #44943 includes some efforts to eliminate the need for
enterWith(...)for APMs viachannel.bindStore(...)andchannel.runStores(...).I think we have a clear path to a good state here, there's just a bunch of work that needs to be done to get there.
Reacted by Toni VillenaThis would seem to point a way forward for actually, finally, honest-to-god deprecation of
async_hooks.createHooks. In particular, this provides two big benefits to instrumentation vendors that will hopefully motivate them to migrate fromcreateHookstoAsyncLocalStorage:- It’s a lot faster, which is more than a little desirable for instrumentation libraries; the vendor that imposes the last CPU/memory cost on production apps would seem to have a competitive advantage I would think.
- It’s cross-platform, letting a vendor’s instrumentation library support not just Node but also CloudFlare and other environments that implement this becoming-standardized
AsyncLocalStorage.
@jasnell Could we perhaps have this live somewhere other than on
node:async_hooks? Maybe onnode:diagnostics? That could provide a way to keep the old and new implementations separate, and we could just deprecate the old one. We should probably stop adding anything tonode:async_hooks, since most people associate it overall withasync_hooks.createHookswhich we’re trying to sunset. Thank you for doing this!cc @nodejs/modules @nodejs/loaders ; related: https://github.com/nodejs/node/discussions/45711
Reacted by Toni Villena and Richard Simpson@GeoffreyBooth I don't agree that this provides a path to deprecating async_hooks. There are other uses outside of context management. It would get the biggest use case off the costly and at times unstable core of async_hooks which I see as a major advantage though. Also makes it easier to restructure async_hooks if we get the highly depended on use case of context management off of it, making breaking changes less ecosystem impacting.
Reacted by Geoffrey Booth, Jordan Harband, Matteo Collina, Marcel Link and Benjamin GruenbaumWould the experimental
onPropagateconstructor option introduced in #45386 work without async_hooks?Would the experimental onPropagate constructor option introduced in #45386 work without async_hooks?
Hmmm good question. I'm not sure to be honest... I don't think so? At least not in its current form.
In the new model that I'm proposing, the async context is not propagated every time a new AsyncResource is created but every time the context is mutated using
asyncLocalStorage.run()(essentially copy-on-write). Whenever an AsyncResource is created, it simply grabs a reference to the current storage context and having to invoke a callback function every time would be quite costly for performance.What I'm thinking would likely be a better solution (and more likely to be something we could implement with this new model) is to associate a tag/namespace with the store that must be echoed back in the
asyncLocalStorage.getStore()call in order to retrieve the stored value.const { AsyncLocalStorage } = require('async_hooks'); const als = new AsyncLocalStorage({ tag: 'abc' }); const { 0: fn1, 1: fn2 } = als.run(123, () => { return [ AsyncResource.bind(() => als.getStore('abc')), AsyncResource.bind(() => als.getStore()), ] }); console.log(fn1()); // 123, because the tag passed in to als.getStore matched console.log(ln2()); // undefined, because the tag did not match.
In the new model that I'm proposing, the async context is not propagated every time a new AsyncResource is created but every time the context is mutated using
asyncLocalStorage.run()(essentially copy-on-write). Whenever an AsyncResource is created, it simply grabs a reference to the current storage context and having to invoke a callback function every time would be quite costly for performance.Sounds pretty similar to @jridgewell's investigation tc39/proposal-async-context#15 on the mutation of the global state and the possible optimization based on the assumption.
Reacted by James M SnellI honestly think we should consider re-thinking
onPropagatenow with this in mind, before it sits for too long in its current state.Reacted by Chengzhong Wu
I've recently gone through and implemented a subset of the
AsyncLocalStorageAPI for Cloudflare's workerd runtime, and I've done so without implementing any part of the async_hooks API under the covers, proving that it's not only possible but results in a significant performance improvement and greatly simplified implementation. I propose that we could greatly improve our implementation by eliminating dependency on async_hooks and adopting a model similar to what we've implemented in workerd.First, an overview of how things currently work in Node.js...
AsyncLocalStorageinstance, there is an associated hidden property on the resource associated with the current execution context. Wheneverals.run(...)is called, we temporarily mutate the value associated with that key and run a function. Any async resources that are created while that function is running ends up receiving a full copy of that map.This design ends up being very inefficient as it means we are allocating and copying the context for every single object. For applications that are particularly promise heavy, this gets very expensive very quickly.
The way we implemented this in workerd is far more efficient:
At any given time, there is a current AsyncContextFrame. By default there is a logical "root" frame but we do not actually need to allocate anything to represent it.
Whenever we call
als.run(...), we create a newAsyncContextFramethat inherits the context of the current, and set the new frame as current. Any async resource that is created simply acquires a reference to the currentAsyncContextFrame.The context is only copied when a new
AsyncContextFrameis created, which happens far less frequently than creating a new async resource.Further, we do not need to rely on V8's PromiseHooks API to propagate context through promises. There is a built-in v8 API
v8::Context::SetContinuationPreservedEmbedderData()that can do the propagation for us much more efficiently. Whenever we enter anAsyncContextFrame, we set theContinuationPreservedEmbedderDatato an opaque object that wraps a reference to the frame. V8 takes care of properly associating that with all promise continuations.There's a lot more to it that I'm glossing over here but the basic idea is that
AsyncLocalStorageis absolutely possible to implement withoutasync_hooksand I think there's a ton of value in doing so.There are some caveats:
als.enterWith(...)andals.disable()APIs. With the new model, these would need to be removed as they make things way more complicated than they are worth.unhandledRejectionevent would need to change (see AsyncLocalStorage and deferred promises #46262)The point of this issue is to see if there is interest in an approach to AsyncLocalStorage that is completely decoupled from
async_hooksbefore @flakey5 and I decide whether or not to work on it./cc @mcollina @vdeturckheim @bmeck @bengl @nodejs/async_hooks