Promise hooks don't work well in vm contexts

[targos]

After #36394, the following test is broken:

const vm = require('vm');
const { AsyncLocalStorage } = require('async_hooks')

const context = vm.createContext({
  AsyncLocalStorage,
  console,
});

vm.runInContext(`
  const storage = new AsyncLocalStorage()
  async function test() {
    return storage.run({ test: 'vm' }, async () => {
      console.log(storage.getStore());
      await 42;
      console.log(storage.getStore());
    });
  }
  test()
`, context);

const storage = new AsyncLocalStorage()
async function test() {
  return storage.run({ test: 'main context' }, async () => {
    console.log(storage.getStore());
    await 42;
    console.log(storage.getStore());
  });
}
test()

Node 16.1.0:

{ test: 'vm' }
{ test: 'main context' }
{ test: 'vm' }
{ test: 'main context' }

Node 16.2.0:

{ test: 'vm' }
{ test: 'main context' }
undefined
{ test: 'main context' }

[targos]

/cc @Qard

[Qard]

It's because it creates new v8::Context instances and the new PromiseHook API is context-scoped. It's not supposed to receive events from other contexts. It's only accidental, and somewhat of a security concern, that other parts of async_hooks are accessible from other contexts. Your example is creating an async_hooks instance in one context and then attempting to receive its events from another context.

Consider this:

const vm = require('vm')

const context = vm.createContext({
  require,
  console
})

vm.runInContext(`
  const ah = require('async_hooks')

  ah.createHook({
    init (asyncId, type, triggerAsyncId, resource) {
      if (type === 'PROMISE') {
        console.log('I stole a promise from outside my context!', resource)
      }
    }
  }).enable()
`, context)

Promise.resolve()

The intent is to make the rest of async_hooks context-scoped too so resources can't leak into other contexts. Depending on how you look at it, this change either breaks compatibility or is a partial bug fix. Either way, async_hooks is experimental, so breaking compatibility for the sake of correctness should be acceptable, as far as I understand.

I've discussed the context-scoping concerns with a few other Node.js core folks in the past and the thinking from them seemed to be in agreement with me that isolate-scoping was wrong and that it would be a bug fix to context-scope it.

I would understand if we chose to hold off on the context-based PromiseHook API until the rest of async_hooks is also adapted to be context-scoped, however that's a large change in itself which I likely won't have the bandwidth to work on in the near future.

[mcollina]

There is not intent for the user to jump between contexts.

See the following:

const vm = require('vm');
const { AsyncLocalStorage } = require('async_hooks')

const context = vm.createContext({
  AsyncLocalStorage,
  console,
});

vm.runInContext(`
  const storage = new AsyncLocalStorage()
  async function test() {
    return storage.run({ test: 'vm' }, async () => {
      console.log(storage.getStore());
      await 42;
      console.log(storage.getStore()); // this logs undefined
    });
  }
  test()
`, context);

the AsyncLocalStorage instance is fully contained within the vm module and there is no crossover, and yet it loses context. From the look of it, we are not attaching some of the mechanism to the new context.

[Qard]

The AsyncLocalStorage instance may be created within that context, but the underlying call to async_hooks.createHook(...) which backs it are not. Because the underlying hooks are created external to the class, they are setup at require-time in the outer context. See: https://github.com/nodejs/node/blob/master/lib/async_hooks.js#L255-L264.

Though even if you move the require into the vm.runInContext(...) you will still get the init for the AsyncResource created by storage.run(...) because, as I said, async_hooks is isolate-scoped, and you will get no resource after the await because PromiseHook is context-scoped and no hooks have been registered in that context.

If you try vm.runInThisContext(...) you will see that running with the vm module actually works just fine, the issue with vm.runInContext(...) is entirely that it is trying to run in a different context than where the context-scoped PromiseHook is registered, therefore the rest of async_hooks, which is isolate-scoped, will leak into that context, while the PromiseHook provided events will not.

I do agree that the intent in that code sample is not to jump between contexts, but that is how it works currently and, more importantly, is how it worked long before the context-scoping of PromiseHooks. Like I said, async_hooks is leaking information into other contexts when it shouldn't. The PromiseHook change fixes some of that leaking, but there's still the rest of async_hooks leaking into other contexts where it should not.

If we wanted to retain the old way of leaking async_hooks information into other scopes, we could probably do something to keep registered promise hook functions synchronized across all contexts. Personally I think that's a bad idea though and we should be working to context-scope everything properly. I would understand if wanted to have it leak the old way at least for the time being and then make the true context-scoping of everything a major change later.

[mcollina]

The problem is that the above breaks the usage of AsyncLocalStorage and Jest as it runs all tests in a VM - it's a significant regression that is not obvious for the end user. This should be fixed, or I fear we would have to revert the main optimization changes :/ - it's definitely needed for the new PromiseHooks to be backported to v14.

I would understand if wanted to have it leak the old way at least for the time being and then make the true context-scoping of everything a major change later.

I fully agree with this approach.

@nodejs/diagnostics wdyrt?

[targos]

We will have to work with the Jest team on this. Even if AsyncLocalStorage is changed to be context-aware, what is going to happen with promises created within core (for example using the fs/promises API in a separate context) ?

/cc @SimenB

[kibertoad]

Issue raised on Jest side: jestjs/jest#11435

[Flarna]

I'm not sure if context scoped async hooks are applicable to all use cases.
If I understand this correct this would mean that e.g. an APM tool has to hook into vm.runInContext and create and AsyncLocalStore there for tracking.
Are e.g. unhandled rejection handlers, uncaught exception handlers per context?

[mcollina]

If I understand this correct this would mean that e.g. an APM tool has to hook into vm.runInContext and create and AsyncLocalStore there for tracking.

No, this issue means that AsyncLocalStorage would not work at all inside vm because the global hooks that AyncLocalStorage relies upon are outside of the context.

[Flarna]

No, this issue means that AsyncLocalStorage would not work at all inside vm because the global hooks that AyncLocalStorage relies upon are outside of the context.

I meant after we move whole async hooks to operate per context.

[addaleax]

How does it make sense to make async hooks per context, given that async operations can cross contexts? I feel like the promise hooks here should cover all contexts created by Node.js.

[targos]

Can we get the previous behavior back without reverting #36394 entirely?

[Qard]

Like I said before, we could synchronize the registered hook functions across every created context. We'd have to store the functions and each created context to make sure that:

1. A new context will automatically set the current promise hook set on creation
2. Changing the promise hook set will update all existing contexts

Still not sure I agree that hooks should broadcast across all contexts, though I intend on eventually ensuring we have sufficient higher-level APIs to just deprecate async_hooks as a public interface anyway, which would eliminate the security concern of leaking resources into other contexts.

I'm not sure if I will have much time to work on this in the immediate term though...I'll see if I can figure something out.

[mcollina]

If we can't get a "quick" resolution, we might have to revert :(.

[Qard]

Yeah. I'll figure out soon if I can rearrange some stuff to get time for this.

[addaleax]

Looks like @bengl is already working on something, but generally, yes, this is not hard to fix, so we should really not revert the original change here.

[bengl]

Yes, I'll try to round out a PR from that commit (there's some cleanup to do, etc.) in the next day or so, time permitting. It's just manually synchronizing PromiseHooks across the main and vm.Context-created Contexts, using the AsyncHooks class to hold the list of Contexts and the JS hooks (for adding to newly created contexts). I'm happy to adjust the approach here if folks have other ideas.