AsyncLocalStorage loses the store if used together with createHook if there is a async function in between

[mcollina]

Consider the following code:

import { AsyncLocalStorage, createHook } from 'node:async_hooks';
import { notEqual } from 'assert/strict';

const hook = createHook({
  init () {},
})

// Commenting out this line will make the bug go away
hook.enable();

const asyncLocalStorage = new AsyncLocalStorage();

async function main () {
  // Commenting out this line will make the bug go away
  await 1
  asyncLocalStorage.enterWith({ foo: 'bar' });
}

await main()

console.log(executionAsyncResource());

// Should not be undefined
notEqual(asyncLocalStorage.getStore(), undefined);

Note that:

1. disabling the hook solve the problem
2. removing the await 1 promise solves the problem

---

Debugging the executionAsyncResource() in this case:

Promise {
  <pending>,
  [Symbol(async_id_symbol)]: 12,
  [Symbol(trigger_async_id_symbol)]: 9
}

while in without the createHook().enable() call:

{ [Symbol(kResourceStore)]: { foo: 'bar' } }

[mcollina]

cc @bengl @Qard

[bengl]

Additional notes:

I attempted with the following changes, and it still happens:

• Replace the async_hook with a promise hook. This narrows it down to promise hooks, I think.
• Moved the hook after the storage construction. The order here doesn't seem to matter, regardless of what type of hook is used, except that it must be before the first await in the example below.
• Replace the main() function, and awaiting it, with a function that reduces the number of awaits used, in case that's an issue. I could not reproduce without awaiting here. Simply putting a then() containing the checks at the end did not suffice to reproduce.

import { AsyncLocalStorage, executionAsyncResource } from 'node:async_hooks';
import { promiseHooks } from 'node:v8';
import { notEqual } from 'assert/strict';

const asyncLocalStorage = new AsyncLocalStorage();

// Commenting out this line will make the bug go away
promiseHooks.onInit(() => {});

await Promise.resolve().then(() => {
  asyncLocalStorage.enterWith({ foo: 'bar' });
})

console.log(executionAsyncResource());

// Should not be undefined
notEqual(asyncLocalStorage.getStore(), undefined);

[Qard]

This is known and (unfortunately) expected. It's also presently encoded into the AsyncContext spec proposal, which I'm trying to explain to people why it should not work that way. Specifically the issue is that PromiseHook, ContinuationPreservedEmbedderData, and the design of the future AsyncContext API all follow the path of continuation registration rather than promise resolution, so effectively context routes around awaits instead of through them.

The reason it works before the first await is that the code up until that first await runs synchronously and so it changes the current context before the await happens. That await then stores that context and restores it when it resumes. However if you put it after the first await then it will have already captured the context to restore coming out of the await before that happens. It only flows within that async function, which effectively makes it the same as a local variable. 😐

I'll continue to work on explaining to TC39 and V8 folks why this doesn't work.

[Qard]

Oh, and the reason why the hooks.enable() in the first example changes the behaviour is because PromiseHook would otherwise not be enabled until that enterWith() is reached and therefore there would be no captured context from the await to restore when it resumes. If you put the hooks.enable() right before that enterWith() it would work because it would have already passed the point when it could have captured a context value for the await.

[mcollina]

Can you expand on why this works:

import { AsyncLocalStorage, createHook, executionAsyncResource } from 'node:async_hooks';
import { notEqual } from 'assert/strict';

const hook = createHook({
  init () {},
})

hook.enable();

const asyncLocalStorage = new AsyncLocalStorage();

await 1

asyncLocalStorage.enterWith({ foo: 'bar' });

console.log(executionAsyncResource());

// Should not be undefined
notEqual(asyncLocalStorage.getStore(), undefined);

While this doesn't?

import { AsyncLocalStorage, createHook, executionAsyncResource } from 'node:async_hooks';
import { notEqual } from 'assert/strict';

const hook = createHook({
  init () {},
})

hook.enable();

const asyncLocalStorage = new AsyncLocalStorage();

await Promise.resolve(1).then(() => {
  asyncLocalStorage.enterWith({ foo: 'bar' })
});

console.log(executionAsyncResource());

// Should not be undefined
notEqual(asyncLocalStorage.getStore(), undefined);

This is really unintuitive.

[Qard]

The enterWith(...) call will set the context until something would change it. In your first example the gap between where you set it and where you try to read it is sync so nothing would have changed it. Whereas in your second example it is set within an awaited promise continuation so the context value captured when the await started would replace that value when the await resumes.

Both async functions and promise continuations occur within this sort of dead space where context will only flow internally. In the case of async/await, the context at the point the await yields is restored when the await resumes. In the case of promises the context is captured when a continuation is added and restored when it runs, but it will do this for each point in the chain, so if you chain multiple continuations in place they will all link back to the same context rather than nesting. You would have to nest the continuation attachment for it to nest the contexts.

The more user-intuitive flow one would expect it to have is that promise continuations and await resumes are treated in the same way as callbacks. The point of confusion is that callbacks have a single point that functions both as callback registration which naturally leads into resolution, but the registration is the final point in the flow before it goes to the native side. Whereas promises split the two concerns and people are conflating the current design of AsyncLocalStorage binding callbacks as meaning that the appropriate place to bind such context is at registration point. Which is actually not the case, just the resolve happens in unobservable code and, due to implementation details, async_hooks and therefore AsyncLocalStorage needs that context binding to occur in observable code.

We don't flow context into internals, so there are a bunch of points where the value stored in context would be technically incorrect or empty, but it doesn't matter because it's impossible to acquire the context value at those points as it's in native code at the time. My rewrite to use AsyncContextFrame was an attempt to begin making that more correct, but that got blocked on V8 internal politics and I haven't got back to it yet...hopefully I will soon.

[legendecas]

Isn't this an issue that constructing an asyncLocalStorage doesn't enable promise hooks for performance optimization, until setting a value in an asyncLocalStorage? Enabling promise hooks before and after a promise creation/resolution would change how AsyncLocalStorage propagates a value is a problem on either semantics.

[mcollina]

Isn't this an issue that constructing an asyncLocalStorage doesn't enable promise hooks for performance optimization, until setting a value in an asyncLocalStorage?

Partially, but I think it's a good tradeoff.

We might want to document this "odd" behavior.

[littledan]

I'm confused about @Qard 's above comment that this issue exists in AsyncContext. The performance optimization that @legendecas mentions would be disabled for any native AsyncContext implementation. (Also enterWith doesn't exist in AsyncContext.) Could you explain the relationship?

[Qard]

The issue is that the user is expecting the context to flow out of an await.

async function main () {
  await 1
  asyncLocalStorage.enterWith({ foo: 'bar' });
}

await main()

// Should not be undefined
notEqual(asyncLocalStorage.getStore(), undefined);

While this does use the current scope modifying enterWith(...) rather than run(...), it could equally be expressed like this and have the same problem:

await asyncLocalStorage.run({ foo: 'bar' }, async () => {
  await 1
})

// Should not be undefined
notEqual(asyncLocalStorage.getStore(), undefined);

The expected flow is that the context flows into asynchronously following code, which it does not do whereas callback code does. A user could reasonably expect rewriting of callback-based code into promise-based code to continue to flow in roughly the same way, but this is not the case.

[littledan]

@Qard Do you agree that the bug as originally filed can be fixed without that other change?

[mcollina]

The issue is that the user is expecting the context to flow out of an await.

I think the issue is that the user expects these two following code to be identical:

async function main () {
  await 1
  asyncLocalStorage.enterWith({ foo: 'bar' });
}

await main()

// Should not be undefined

notEqual(asyncLocalStorage.getStore(), undefined);

await 1
asyncLocalStorage.enterWith({ foo: 'bar' });
await 1

// Should not be undefined
notEqual(asyncLocalStorage.getStore(), undefined);

Most developers would think those are identical. Only experts would notice the difference (I was fooled by it on first sight, even after implmeneting a chunk of it).

[Qard]

@Qard Do you agree that the bug as originally filed can be fixed without that other change?

How? What other change? I'm not clear what you are referring to.

[littledan]

The other change being turning off the optimization that legendecas identified in #53037 (comment) . Such an optimization will not apply to AsyncContext.