AsyncLocalStorage loses the store if used together with createHook if there is a async function in between about node HOT 22 OPEN

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.

from node.

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.

from node.

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.

from node.

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

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).

enterWith is the factor that make the pattern unintuitive. If the AsyncLocalStorage was accessed with run instead, the two examples would be identical:

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

await main()

equal(asyncLocalStorage.getStore(), undefined);

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

equal(asyncLocalStorage.getStore(), undefined);

from node.

Indeed, but that destroys the goal. asyncLocalStorage.run() leads to worse DX, which is the whole point of this exercise.

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

const asyncLocalStorage = new AsyncLocalStorage();

await asyncLocalStorage.run({ foo: 'bar' }, async () => {
  await 1
  console.log(executionAsyncResource());
  // Should not be undefined
  notEqual(asyncLocalStorage.getStore(), undefined);
})

vs

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

const asyncLocalStorage = new AsyncLocalStorage();

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

await 1

console.log(executionAsyncResource());

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

The latter is 100% more idiomatic javascript.

from node.

Indeed, but that destroys the goal. asyncLocalStorage.run() leads to worse DX, which is the whole point of this exercise. …
The latter is 100% more idiomatic javascript.

Note that we've discussed how to bring DX improvements to AsyncContext proposal in a way that would be consistent to developers:

const ctx = new AsyncContext.Variable();

async function main() {
  using _ = ctx.scope('foo');
  equal(ctx.get(), 'foo');
  await 1
  equal(ctx.get(), 'foo');
}

const m = main();
// Doesn't matter that main started executing, the suspend point restores previous context
equal(ctx.get(), undefined);

await m;
// Doesn't matter that main finished executing, the await's resume restores previous context
equal(ctx.get(), undefined);

This won't solve @Qard's goal, but it does make OP's problematic code consistent. It shouldn't matter how you structure the code, the variable stores the same undefined value outside the using scope.

from node.

That flow makes it essentially only a local variable though. Users are expecting it to flow into continuations as is shown by the assumption at the start that the context set within main is available in the top-level scope after main completes. This is the point I'm trying to make--code after await points are functionally the same as nested callbacks and those do receive the context, so the behaviour of not flowing through those points is inconsistent and confusing to users.

from node.

cc @bengl @Qard

from node.

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);

from node.

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.

from node.

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.

from node.

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.

from node.

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?

from node.

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.enterWith({ foo: 'bar' }, async () => {})

// 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.

from node.

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

from node.

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).

from node.

@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.

from node.

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

from node.

That doesn't fix the flow problem I described. It would make the behaviour consistent, but consistently not what the user is expecting.

from node.

worse DX, which is the whole point of this exercise.

Which exercise? If you were talking about AsyncContext: I don’t think the point of AsyncContext is to bring DX improvements vs AsyncLocalStorage. It is more about being a subset which can be implemented across environments, optimized well, etc.

from node.

I think this issue has multiple possible goals at the same time:

• Inconsistent behavior for seemingly equivalent code structure
• Improved DX without nesting in callbacks
• Flow-through contexts

I don't know what priority @mcollina sets for these, or even if he intended to hit on all these points in the OP. I'm only saying that we can solve the first 2 without the 3rd. AC already fixes the inconsistent behavior, and we've discussed how to improve DX. If the third is lowest priority, or even unintentional, then it doesn't necessitate a change in design for AC or ALS.

from node.

Improved DX without nesting in callbacks

How can you achieve this without flow-through contexts?

from node.