Repository navigation
AsyncResource behaves differently w/ AsyncLocalStorage in v24.0.0 #58204
Description
Activity
- changed the title
[-]AsyncResource behaves differently w/ AsyncLocalStorace in v24.0.0[/-][+]AsyncResource behaves differently w/ AsyncLocalStorage in v24.0.0[/+]on May 7, 2025 That seems expected, actually. AsyncResource is meant to capture its value at construction and restore that value around each run. It's a bit weird that the value could be mutated by an enterWith within itself. It should persist into new descending tasks, but AsyncResource should not mutate like this.
The difference seems to be that with old async_hooks the captured value is accidentally getting mutated by the enterWith somehow when it is active, probably because of how it stored the context values directly on the resource objects. I'd have to take a closer look at why it was behaving that way.
I would say from the semantics though that it seems to me like a bug that it ever behaved this way. 🤔
I'd argue that
enterWith(...)is the realy bug here ;-)Snark aside, while I generally agree that the new behavior is likely preferable we should consider this a regression.
The prior implementation also suffers from a time travel bug of sorts. It can mutate already inflight async branches which results in strange behaviours like this:
const { AsyncLocalStorage, AsyncResource } = require('async_hooks') const storage = new AsyncLocalStorage() storage.enterWith("foo") const ar = new AsyncResource('test') ar.runInAsyncScope(() => { // The "foo" value flows into this asynchronous branch. setImmediate(() => { // However, this restores the AsyncResource temporally _after_ the enterWith below, // and so it uses that mutated value of the AsyncResource. ar.runInAsyncScope(() => { console.log(storage.getStore()) // "bar" }) }) // Generally one would assume this would change only the _following_ execution in this scope // and not modify the value of already created branches which lead to a restore of the AsyncResource. storage.enterWith("bar") })
Basically AsyncResource is supposed to be a snapshot of state at the point at which it is constructed, which it then sets as the store state when it is run. But because of the previous implementation of ALS stamping the store value directly on the current resource, these behaviours form a circular dependency of sorts which results in a modification loop which could actually leak out into everything which uses AsyncResource.
That is to say, yep...
enterWith(...)is kind of the bug here, though equally as much so isAsyncResource. Neither is particular great as neither are protected well or specified clearly enough.As for calling this a regression, I think what makes the most sense here is to document the difference. Trying to adapt the new code to behave the same as the original would be not only quite complicated, as it would need to start keeping track of constructed AsyncResource instances to modify whenever an AsyncContextFrame change happens (which would be expensive), but it would also be objectively worse than the new behaviour.
AsyncResourceis essentially equivalent toAsyncContext.Snapshotin that it's supposed to capture state at construction time to restore later for some scope. By making it accidentally mutable, it violates a whole lot of the flow guarantees which context management is supposed to provide.Hmm.. yeah, I agree with that also... hmm... given that
enterWith(...)is still technically experimental and given the issues, I think I agree that the best approach here is to document. I want to be sensitive, however, to anything this may have broken ... which I assume is why @bengl opened the issue. I wonder if there's an approach to tellingAsyncResourceto update its internal snapshot, something like...const storage = new AsyncLocalStorage() storage.enterWith(1) const ar = new AsyncResource('test') ar.runInAsyncScope(() => { storage.enterWith(2); ar.refreshSnapshot(); // <-- tells the AsyncResource to take a new snapshot of the async context ar.runInAsyncScope(() => strictEqual(storage.getStore(), 2)) })
I want to be sensitive, however, to anything this may have broken ... which I assume is why @bengl opened the issue.
For my purposes, I have a workaround, which is to re-implement the relevant code using far better primitives like
tracingChannel. That said, I think anyone wanting to be that deliberate can just make anotherAsyncResourceinstance locking in the new state. 🤔The problem with using
AsyncResourceto capture/activate a context is the global scope:const { AsyncLocalStorage, AsyncResource } = require("node:async_hooks"); const als1 = new AsyncLocalStorage(); const als2 = new AsyncLocalStorage(); als1.run("one", () => { const ar = new AsyncResource("ar1"); als2.run("two", () => { console.log(als1.getStore()); // one console.log(als2.getStore()); // two ar.runInAsyncScope(() => { console.log(als1.getStore()); // one console.log(als2.getStore()); // undefined }); }); });
As you can see both als instances are effected from the
AsyncResource.While this is fine and expected if used by library programmers being friendly to ALS users (e.g. some DB client) it's usually an unintended side effect if done by monitoring tools like OpenTelemetry.
I'd argue that
enterWith(...)is the realy bug here ;-)Yes and no. It's the combination of
enterWith()andrun(). Using onlyenterWith()or onlyrun()is usually easier to understand then combining it.
UsingtracingChannelis effective using onlyrunso you are back on the safe track.This issue has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.This issue has been automatically closed after 30 days of inactivity following its stale status (no activity for a total of 240 days).
If this is still relevant, feel free to reopen it or leave a comment with additional details so we can continue the discussion.
Version
v24.0.0
Platform
Subsystem
async_hooks
What steps will reproduce the bug?
Running this in Node.js 24.0.0 breaks, and it passes on all earlier versions.
How often does it reproduce? Is there a required condition?
This is reproducible as long as
--no-async-context-frameis not set.What is the expected behavior? Why is that the expected behavior?
This should pass, as it does on previous versions. If a storage is entered with a given
storein arunAsyncContext()block of anAsyncResource, then future invocations ofgetStore()should returnstoreas long as no otherenterWith()has been called on that storage.What do you see instead?
It looks like it's reverting to the
storethat was active when theAsyncResourcewas created.Additional information
The test case provided is a whittled-down version of something we do in legacy instrumentations in
dd-trace-jsthat pre-datetracingChannel. Re-implementing those instrumentations usingtracingChannelsolves this, since we're then not doing anything too strange withAsyncResourceslike we are here.