Node: Async Hooks do not recognize execution contexts created when processing thenables

[medikoo]

Promise.all to observe input promises, invokes promise.then in next event loop, at least it's what stack traces show in both Node.js and Google Chrome.

Still Async Hooks see it as same execution context, it can be seen by running following code:

const async_hooks = require('async_hooks');

async_hooks.createHook({ init() {} }).enable();

Promise.all([
  {
    then() {
      console.log(async_hooks.executionAsyncId(), async_hooks.triggerAsyncId(), 'thenable.then invoked by Promise.all');
      console.trace('thenable.then invoked by Promise.all');
    },
  },
]);

console.log(async_hooks.executionAsyncId(), async_hooks.triggerAsyncId(), 'Main');
console.trace('Main');

The outcome is:

1 0 'Main'
Trace: Main
    at Object.<anonymous> (/Users/medikoo/Projects/maas/maas-backend/test.js:15:9)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at startup (internal/bootstrap/node.js:266:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
1 0 'promise.then invoked by Promise.all'
Trace: promise.then invoked by Promise.all
    at Object.then (/Users/medikoo/Projects/maas/maas-backend/test.js:9:15)
    at process._tickCallback (internal/process/next_tick.js:68:7)
    at Function.Module.runMain (internal/modules/cjs/loader.js:745:11)
    at startup (internal/bootstrap/node.js:266:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)

I've stumbled on that when trying to configure long stack trace solution for Node.js, and was dealing with custom promise implementations.

I believe that's not expected.

• Version: v10.9.0 (observable also on v8)
• Platform: macOs 10.13.6 (17G65)

[medikoo]

As I see, it goes down to Promise.resolve:

const async_hooks = require('async_hooks');

async_hooks.createHook({ init() {} }).enable();

Promise.resolve({
  then() {
    console.log(
      async_hooks.executionAsyncId(),
      async_hooks.triggerAsyncId(),
      'thenable.then invoked by Promise.resolve'
    );
    console.trace('thenable.then invoked by Promise.resolve');
  },
});

console.log(async_hooks.executionAsyncId(), async_hooks.triggerAsyncId(), 'Main');
console.trace('Main');

Produces:

1 0 'Main'
Trace: Main
    at Object.<anonymous> (/Users/medikoo/Projects/maas/maas-backend/test.js:17:9)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at startup (internal/bootstrap/node.js:266:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
1 0 'promise.then invoked by Promise.resolve'
Trace: promise.then invoked by Promise.resolve
    at Object.then (/Users/medikoo/Projects/maas/maas-backend/test.js:12:13)
    at process._tickCallback (internal/process/next_tick.js:68:7)
    at Function.Module.runMain (internal/modules/cjs/loader.js:745:11)
    at startup (internal/bootstrap/node.js:266:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)

[firedfox]

1. Installing async hooks via async_hooks.createHook enables promise execution tracking.
   As described here.

2. Passing { then: Function } to Promise.all or Promise.resolve triggers then in current event loop because there is no Promise before then.

I think this is how to produce what you expected:

const async_hooks = require('async_hooks');

async_hooks.createHook({ init() {} }).enable();

console.log(async_hooks.executionAsyncId(), async_hooks.triggerAsyncId(), 'Main');
Promise.all([
  Promise.resolve().then(() => {
    console.log(async_hooks.executionAsyncId(), async_hooks.triggerAsyncId(), 'promise.then invoked by Promise.all');
    console.trace('promise.then invoked by Promise.all');
  }),
]);

Produces:

1 0 'Main'
7 6 'promise.then invoked by Promise.all'
Trace: promise.then invoked by Promise.all
    at Promise.all.Promise.resolve.then (/Users/firedfox/tmp/test.js:9:13)
    at process._tickCallback (internal/process/next_tick.js:68:7)
    at Function.Module.runMain (internal/modules/cjs/loader.js:745:11)
    at startup (internal/bootstrap/node.js:236:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:560:3)

[medikoo]

@firedfox indeed I forgot about async_hooks.createHook({..}).enable() to make posted test case fully reliable. Still it was just mistake in code example (I've reedited it to avoid further confusion), adding that initialization doesn't help, outcome remains same.

Note: it's about an extra event loop in which _then method is executed_ and not its callback. Your example tests the latter, that works without issues and it's not a concern here.

[medikoo]

I've updated examples a bit more to better expose that we deal with different event loop, which is not reflected by async_hooks.executionAsyncId()

[firedfox]

IMO, it's because { then: Function } you used is not a promise.then but just plain object.then.
The Promise.resolve() method returns a Promise object. So Promise.resolve().then gives us a promise.then. All other ways that return a Promise object should produce the same result.

Sorry I finally understand what you mean. You are right. It seems to be a problem.

const async_hooks = require('async_hooks');

async_hooks.createHook({ init() {} }).enable();

Promise.all([
  {
    then() {
      console.log(async_hooks.executionAsyncId(), async_hooks.triggerAsyncId(), 'promise.then invoked by Promise.all');
      console.trace('promise.then invoked by Promise.all');
    },
  },
]);

process.nextTick(() => {
  console.log(async_hooks.executionAsyncId(), async_hooks.triggerAsyncId(), 'nextTick');
  console.trace('nextTick');
});

console.log(async_hooks.executionAsyncId(), async_hooks.triggerAsyncId(), 'Main');

Produces:

1 0 'Main'
Trace: Main
    at Object.<anonymous> (/Users/firedfox/tmp/test.js:20:9)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at startup (internal/bootstrap/node.js:236:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:560:3)
8 1 'nextTick'
Trace: nextTick
    at process.nextTick (/Users/firedfox/tmp/test.js:16:11)
    at process._tickCallback (internal/process/next_tick.js:61:11)
    at Function.Module.runMain (internal/modules/cjs/loader.js:745:11)
    at startup (internal/bootstrap/node.js:236:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:560:3)
1 0 'promise.then invoked by Promise.all'
Trace: promise.then invoked by Promise.all
    at Object.then (/Users/firedfox/tmp/test.js:9:15)
    at process._tickCallback (internal/process/next_tick.js:68:7)
    at Function.Module.runMain (internal/modules/cjs/loader.js:745:11)
    at startup (internal/bootstrap/node.js:236:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:560:3)

[medikoo]

@firedfox I don't understand how your comment is relevant to this report?

Again, bug is that async contexts in which thenable.then is called by any native Promise method is not exposed by async_hooks as expected.

This bug makes async_hooks unreliable for cases when we work with custom thenables, and that's not that uncommon, e.g. you can find them in quite popular knex library.
With current state of things, it's not possible to get information about _trigger_ async context, within scope of that then method invocation (when invoked by native Promise method)

As long as it's the case, reliable long stack trace solution cannot be configured using async_hooks.

[jasnell]

I believe the issue here is more about the fact that async_hooks does not currently track thenables in the same way that it tracks actual Promise instances. Because the thenable here is not actually visible to async_hooks, it is not able to do the appropriate context association.

[medikoo]

Because the thenable here is not actually visible to async_hooks, it is not able to do the appropriate context association.

I'm not sure if it's a valid explanation, as here it's native Promise.all that's responsible for introduction of new async context, and not thenable that's passed to it.

I suppose that if async_hooks can monitor moments in which callbacks passed to promise.then are invoked (by promise.then internals), same way it should be able to monitor invocations of thenable.then (which are made within Promise.resolve internals)

I take that code responsible for that is in C++ layer (?)

[jasnell]

The state of Promise resolution is tracked using an API at the C++ layer. Thenables do not tap into that mechanism and therefore cannot be tracked the same way as normal promises.

You can visualize the difference using the clinic bubbleprof tool

thenable:

Promise.all([
  {
    then() {
      console.log('a')
    }
  }
])

https://upload.clinicjs.org/public/7c8bcbc1db0fdaf015a34941a1fbfa0b32db378f8d6b5b06db9031df7cd48f76/171215.clinic-bubbleprof.html

promise:

Promise.all([
  Promise.resolve('a').then(console.log)
])

https://upload.clinicjs.org/public/8a263182a3c9a50c2d4066aa22eae417cb5c6366e9977e2fef6560d7ff6f87e7/171277.clinic-bubbleprof.html

If you explore the differences between the two call graphs, you'll see that the thenable is not seen by V8 or Node.js as a promise and therefore cannot be tracked in the same way.

[medikoo]

I don't see those two examples equivalent. In first one console.log is within a body of then, and in second one it makes a body of a __callback__ passed to then, and that's a quite big difference.
Technically first example involves two event loops, and second three event loops.

Anyway to make sure I understand you right - you acknowledge there's an event loop introduced by native Promise internals (so V8) to resolve thenables, but want to point that it's not exposed properly outside, so cannot picked by async_hooks, and therefore it's not possible to fix this issue?

[devsnek]

async_hooks tracks promises, not the event loop's promise job queue.

[medikoo]

async_hooks tracks promises, not the event loop's promise job queue.

Ok, clear, so event loops tracking is made purely by following promise instances, which in turn makes it not perfectly complete (as not all enqueued jobs within V8 are mapped to new async contexts).

Question is whether this should be considered a feature or a bug? :)

[stephenh]

@medikoo I'm just wandering by, but does this comment in the google stackdriver tracer address/work around your observation?

https://github.com/GoogleCloudPlatform/cloud-trace-nodejs/blob/master/src/cls/async-hooks.ts#L62

[medikoo]

@stephenh it doesn't look related to this issue. PROMISE type reflects promise initialization, and here issue is that we miss tracking of execution contexts created by V8 for processing thenables (those do not involve promise creation).

[Trott]

Question is whether this should be considered a feature or a bug? :)

@nodejs/async_hooks Thoughts? This is a bug that should be fixed and/or a feature that needs to be implemented? Or this is working as intended and can be closed?

[AndreasMadsen]

As I recall, the executeAsuncId should be 0 to indicate a missing context. That is the only bug here.

If a user implements thenables or anything else that can't be tracked, they need to use a AsyncResource instance to maintain the context. The same is the case for bluebird promises, as an example.

[medikoo]

Thanks @AndreasMadsen I just checked and indeed that can be workaround with AsyncResource, so that's a good news!

While thenable.then is _invoked_ in next tick, it is actually _retrieved_ immediatelly from thenable, on basis of that we can configure well working async resource by defining a then as a getter.

Working proof of a concept:

const { createHook, executionAsyncId, triggerAsyncId, AsyncResource } = require("async_hooks");

const events = [];
process.on("exit", () => {
  console.log(events);
});

createHook({
  init(asyncId, type, triggerAsyncId) {
    events.push(["init", asyncId, type, triggerAsyncId]);
  },
  before(asyncId) {
    events.push(["before", asyncId]);
  },
  after(asyncId) {
    events.push(["after", asyncId]);
  }
}).enable();

class Thenable {
  get then() {
    const asyncResource = new AsyncResource("Thenable");
    console.trace(executionAsyncId(), triggerAsyncId(), "get thenable.then");
    return function(onSuccess, onFailure) {
      return asyncResource.runInAsyncScope(
        (onSuccess, onFailure) => {
          console.trace(executionAsyncId(), triggerAsyncId(), "thenable.then");
        },
        this,
        onSuccess,
        onFailure
      );
    };
  }
}

const thenable = new Thenable();

const outerNotAccessibleContext = thenable =>
  setTimeout(() => {
    console.trace(executionAsyncId(), triggerAsyncId(), "setTimeout");
    Promise.resolve(thenable);
  });

outerNotAccessibleContext(new Thenable());

console.trace(executionAsyncId(), triggerAsyncId(), "Main");

As I take it's how it's supposed to work, I'm closing the issue

[medikoo]

The only subtle issue is that, such new async resource should be created __only__ in case then is retrieved by V8 async queue logic, and there are no straightforward means to detect that.
Best we can do probably is to examine current stack trace.

[AndreasMadsen]

Alternativly, just make Thenable inherit from AsyncResource. What you prefer, depends on how you want the async context to flow.

[medikoo]

Alternativly, just make Thenable inherit from AsyncResource

As far as I understand it that won't work, as we need AsyncResource instance not per thenable but per one thenable.then invocation (and per one thenable there can be many invocations in different async contexts)

Generally current means do not leave us with any solid solution for that problem. As even if we hack in as a then getter, we don't know whether it's a V8 async queue that retrieves then (and it's only in that case when new AsyncResource should be initialized, otherwise we expose weird side effects that may expose misinformation about async contexts)

[AndreasMadsen]

As far as I understand it that won't work, as we need AsyncResource instance not per thenable but per one thenable.then invocation (and per one thenable there can be many invocations in different async contexts)

Not really correct. In the AsyncHooks model, the promise is the AsyncResource.

Consider this logger:

const { createHook } = require('async_hooks')

const asyncHook = createHook({
  init (asyncId, type, triggerAsyncId, resource) {
    process._rawDebug('init', asyncId, type);
  },

  before (asyncId) {
    process._rawDebug('before', asyncId);
  },

  after (asyncId) {
    process._rawDebug('after', asyncId);
  },

  destroy (asyncId) {
    process._rawDebug('destroy', asyncId);
  },

  promiseResolve (asyncId) {
    process._rawDebug('resolve', asyncId);
  }
})
asyncHook.enable()

Then consider this code:

const thenable = new Promise(function (resolve) {
  resolve(1);
});

That emits:

init 5 PROMISE
resolve 5

so I would try:

class ImmediateThenable extends AsyncResource {
  constructor(value) {
    super('ImmediateThenable');
    this._value = value;
  }

  then(onSuccess, onFailure) {
    return this.runInAsyncScope(function (onSuccess, onFailure) {
      if (onSuccess) onSuccess(this._value);
    }, this, onSuccess, onFailure);
  }
}

[medikoo]

Use case I'm addressing is connecting async stack contexts to produce reliable long stack traces.

As far as I understand with async_hooks I can achieve it by gathering stack trace at init event, and appending it to any stack generated betwen before and after events for given async resource.

So e.g. in following example result should be that console.trace at A3 should expose stack that joins traces from A1, A2 and A3 points, same way B3 should join stacks fro B1, B2 and B3 points.
(note that while A1 and B1 point same async contexts, it's not the same with A2 and B2, and A3 and B3).

const { createHook, executionAsyncId, triggerAsyncId, AsyncResource } = require("async_hooks");

const getStack = () =>
  new Error().stack
    .split("\n")
    .slice(2)
    .join("\n");

const asyncHook = createHook({
  init(asyncId, type, triggerAsyncId, resource) {
    process._rawDebug("init", asyncId, type, getStack());
  },
  before(asyncId) {
    process._rawDebug("before", asyncId);
  },
  after(asyncId) {
    process._rawDebug("after", asyncId);
  },
  destroy(asyncId) {
    process._rawDebug("destroy", asyncId);
  },
  promiseResolve(asyncId) {
    process._rawDebug("resolve", asyncId);
  }
});
asyncHook.enable();

class Thenable extends AsyncResource {
  constructor(value) {
    super("Thenable");
  }

  then(onSuccess, onFailure) {
    return this.runInAsyncScope(
      function(onSuccess, onFailure) {
        // Invoked twice in different async contexts
        process._rawDebug("A3 or B3", getStack());
      },
      this,
      onSuccess,
      onFailure
    );
  }
}

const thenable = new Thenable();

process._rawDebug("A1", getStack());
setTimeout(() => {
  process._rawDebug("A2", getStack());
  Promise.resolve(thenable);
}, 100);

process._rawDebug("B1", getStack());
setTimeout(() => {
  process._rawDebug("B2", getStack());
  Promise.resolve(thenable);
}, 50);

Now if you examine log output. There are two issues:

• Two different async contexts created by two distinctthenable.then resolutions are presented with same id (5). It doesn't appear as right to me.
• We lack information on where each thenable.then async context really initializes. Technically it should point to context in which Promise.resolve call was made. By looking into logs we see that PROMISE type resources point there (as new promises are created by Promise.resolve), but that information is disconnected from thenables. I don't have means to correctly map it to thenable.then resolutions.

[AndreasMadsen]

As far as I understand with async_hooks I can achieve it by gathering stack trace at init event, and appending it to any stack generated betwen before and after events for given async resource.

Not necessarily. This is the general rule I would use but for promises, it can get very complicated. Because there multiple places in your code that you may consider the "start".

• new Promise(): this would be the init event like you suggest.
• resolve(value): instead of init you should use the promiseResolve event.
• .then(): calling .then() creates a new promise. Instead of the init event of the parent promise, use the init of this "chainedPromise".

When you are arguing your point, please be aware that all these 3 different places of "origin" are valid. There is no true origin for a promise, which is why we need to allow all three types.

[medikoo]

In case I'm after init event emitted at new Promise() is irrelevant, as this doesn't technically produce any new async context my code would be run in (I don't loose any stack continuity here)

It's at promise.then() or resolve(value) when this happens. So I want to bridge in stack a point of where promise.then() was invoked with point where callback passed to then was invoked, and similarly, point of where resolve(thenable) was invoked with point where thenable.then was invoked.

So while promiseResolve event is invoked on resolve(thenable), there's still no straightforward way to connect this with given thenable.then invocation. The solution I see, is to:

• gather stack trace at promiseResolve
• if thenable.then is retrieved after promiseResolve event, both happened in same async context, and share same stack trace. Then we can I assume it's resolve(thenable) operation, and initialize new async resource.

[AndreasMadsen]

Sorry, I don't get what the issue is. I think my "point 3" is what you need. Apparently, you say it isn't. As far as I can see every single point of execution regarding promises is somehow exposed. So I really don't get it.

/cc @jasnell @nodejs/async_hooks – I don't get this, maybe one of you can help.

[medikoo]

Sorry, I don't get what the issue is.

Putting it is as simple as possible:

With help of async hooks, I want to construct a long stack trace solution that will produce an expected stack trace in below example:

class Thenable {
  then() {
    // Stack trace that expose that this originated at `Promise.resolve(thenable)`
    console.trace();
  }
}
Promise.resolve(new Thenable());

I don't see a straightforward way to achieve it (best I came up with is configuring thenable.then as a getter and put some monitoring logic there)

[AndreasMadsen]

I don't think this has anything to do with it being a Thenable and not a promise. If you are implementing your own Thenable, then you need to create an AsyncRessource instance to maintain context!

In this case, use the init event.

If what you really mean is that you want

class Thenable {
  then() {
    // Stack trace that expose that this originated at `Promise.resolve(thenable)`
    console.trace();
  }
}
const thenable = new Thenable();
Promise.resolve(thenable);

to show Promise.resolve(thenable); in the stack trace. Then then() should create another Thenable (just like an ordinary Promise would do), then use the init event for that promise.

[medikoo]

I'll try to clarify it further:

• To rely on async_hook to connect stack traces, the init event for given new context should be emitted in parent (trigger) async context and not in same one in which before and after are emitted.
• I'm addressing use case of lazy thenables, so one where in context of then invocation there's some significant work going on (_it's not internals we want to hide as it's the case with native promises_) and eventual crash (or console.trace) in there should produce expected stack trace.

So situation is as follows:

class Thenable {
  then() {
    // ('before' event should be emitted at this point)
    // As we're already in new executed async context
    // (invoked in other microtask by V8 async queue) 
    // it's too late here for 'init' event and if one was not emitted at trigger context
    // we lost means to connect stack trace with `Promise.resolve(thenable)`
    const returnValue = new Thenable(); // 'init' event here brings no valuable info
    // .. lazy thenable.then operations
    console.trace(); // Let me see Promise.resolve(thenable) here
    // ... lazy thenable.then operation
    return returnValue;
    // ('after' event should be emitted at this point)
  }
}
const thenable = new Thenable(); // 'init' event here brings no valuable info, see below:
// Added setTimeout to show that resolution of thenable.then may happen
// at very different context than thenable construction
setTimeout(() => {
  // Below line triggers new async context
  Promise.resolve(thenable); // ('init' event should be emitted at this point)
});

So from what I see, following suggestion:

then() should create another Thenable (just like an ordinary Promise would do), then use the init event for that promise.

Won't work for this use case.

[netproteus]

I've just spent the last 3 days trying to follow thenable chains 3 dependencies deep from the code in my actual application. (Objection => Knex => Tarn) Was absolutely miserable to try and find the problems, thus far have found 3 and still there are more somewhere. And I suspect yet more in other code paths. It's all very well to say this can be worked around with AsyncResource, but thenables are part of the Promises/A+ spec, and there is nothing to stop anyone implementing or changing the implementation of libraries we all depend on directly or indirectly. IMHO unless thenables can be tracked without code changes async_hooks are completely useless.

[medikoo]

I'll reopen this until we settle on how that's expected to be addressed.

Currently the only way to somehow handle this case is via configuring thenable.then as getters, still that doesn't seem to be 100% bulletproof (and feels more as a hack, than valid solution, so I'm not convinced that should be coined as expected approach).

[AndreasMadsen]

Currently the only way to somehow handle this case is via configuring thenable.then as getters, still that doesn't seem to be 100% bulletproof (and feels more as a hack, than valid solution, so I'm not convinced that should be coined as expected approach).

It isn't, and it is not how native promises work either. The expectation is that the use imitates how native promises work using AsyncResource. It is possible that how native promises work, is also not desirable. Unfortunately, it doesn't appear to be possible to turn the discussion in that direction.

[medikoo]

It isn't, and it is not how native promises work either. The expectation is that the use imitates how native promises work using AsyncResource.

So if I understand right, you state that use case of lazy thenables is invalid (because it's not how native promises work) and we should not expect it to be covered by async_hooks (?)

[AndreasMadsen]

So if I understand right, you state that use case of lazy thenables is invalid (because it's not how native promises work).

Correct.

and we should not expect it to be covered by async_hooks.

I don't think it technically possible to cover thenables natively via V8. If it is, then it should be done in a unified way that appears the same regardless of whether it is a promise or a user-defined thenable.

[netproteus]

I think everyone is getting caught up in the detail of the current implementation and not thinking about the practically of using this api in real life.

If you are using a library that has an async api, perfectly compliant with the Promise/A+ spec you shouldn’t have to worry that somewhere in the depths it broke the context chain because it used a lasy thenable somewhere.

In that scenario you have 2 apis which are contractually broken in relation to each other. Given the body of code already implementing the Promise spec it is not going to change. So if that case can’t be covered the async_hooks api will never be useful for real world applications.

[AndreasMadsen]

I think everyone is getting caught up in the detail of the current implementation and not thinking about the practically of using this api in real life.

I would love to change the current implementation if what you suggest makes sense. But at the moment I have only seen problems being described, without any concrete solutions. It is not because I don't want to help, it is because I don't know how!

[medikoo]

I don't think it technically possible to cover thenables natively via V8

Isn't it V8 that schedules thenable.then invocation for next microtask? If it's so, why it can't also expose means that will allow to track that execution path?

[netproteus]

But at the moment I have only seen problems being described, without any concrete solutions.

I would love to help but I'm afraid I don't know enough about the internal's of node / v8 so my suggestions would be superficial. I'm very happy to help with testing or to provide more concrete uses cases or real world examples of where this is broken.

[rochdev]

From #26064:

if you want to track bluebird promises, bluebird needs to be async_hooks aware, or you need to patch async_hooks behaviour into it. async_hooks can't track what it doesn't know.

@devsnek I disagree with that for a few reasons.

In general I don't expect libraries to implement AsyncResource since async_hooks is only used directly by a very specific audience and most library maintainers are not part of this audience. This means they probably don't know about AsyncResource in general. It's also possible that the developer of an app defines a thenable themselves and again I don't expect them to know they have to do that.

I would also expect await to be able to provide the correct IDs since it receives the thenable directly and synchronously. This means it should be able to basically create the AsyncResource for the user. Maybe I'm not aware of limitations in Node that would prevent this, but in general I think this would be the only solution that would permanently and completely fix the issue.

As @Qard mentioned in #27599, without this functionality it makes async_hooks basically unusable given the potentially high number of existing and future libraries that can randomly cause this issue.

[netproteus]

Async hooks as they stand are a broken abstraction that either need fixing/reworking or removing from node.

It is in no way reasonable to expect anything implementing the Promises/A+ spec to have to care about implementing AsyncResource.

[Qard]

Because await, as a language construct, is designed in such a way as to accept a thenable, it can thus be concluded the thenables too are a language-level pattern and therefore need the same treatment as promises do in this regard.

[mcollina]

Async hooks as they stand are a broken abstraction that either need fixing/reworking or removing from node.

You are welcome to contribute: we need all the help we could get. There is a lot of work to be done in this area and it seems you are very passionate about it. The diag-wg is working on this among other things: https://github.com/nodejs/diagnostics.

---

@bmeurer @MayaLekova do you think it's possible to have a hook to implement the suggestion in https://github.com/nodejs/node/issues/22360#issuecomment-491444510?

I would also expect await to be able to provide the correct IDs since it receives the thenable directly and synchronously. This means it should be able to basically create the AsyncResource for the user. Maybe I'm not aware of limitations in Node that would prevent this, but in general I think this would be the only solution that would permanently and completely fix the issue.

[bmeurer]

@mcollina Potentially. 🤔

[rochdev]

The main issue to implement this could be that await is a native JavaScript construct interpreted directly by v8, meaning there may be no way for Node to hook into it directly. If that's the case, then I would tend to agree with @netproteus that async_hooks as they stand will never work and that there must be a solution built into v8.

In general, I feel this is the kind of thing that should be exposed by the engine since it is actually tracking a JavaScript context. New language constructs should always be supported out of the box and not break context. Was there ever a discussion to try and get context propagation built into v8? Maybe it's something that could even work with async_hooks to preserve backward compatibility?

[medikoo]

You are welcome to contribute: we need all the help we could get.

@mcollina if you follow the thread carefully, you'll read that the use case we stand for, is considered as _invalid_ and not to be supported. How do you imagine contribution process in given situation?

Also this goes down to V8 I believe. It's first V8 that should expose additional hooks, so they can be exposed in async_hooks.

[mcollina]

@mcollina if you follow the thread carefully, you'll read that the use case we stand for, is considered as invalid and not to be supported. How do you imagine contribution process in given situation?

Essentially joining the discussion about this topic and providing actionable solutions. Working on improving async_hooks. Stating that something should be removed/refactored without any will to do any work in that regard is not really a contribution. Node.js is maintained by its collaborators as per our governance process.

If anyone has a _better_ alternative to async_hooks, I think the whole @nodejs/diagnostics is willing to hear that! It took years of work to build this model - problematic as it is. async_hooks is not out of experimental yet because of the issues we are facing all the time with it.

It is true that if you need to maintain context, you have to deal with AsyncResource. There is currently no way around that, and there will likely never be. However, await could potentially call us if it encounters a thenable, so we could potentially support part of this automatically.

Also this goes down to V8 I believe. It's first V8 that should expose additional hooks, so they can be exposed in async_hooks.

Essentially yes. This is _not_ fixable in Node.js alone. Maybe this is something that the V8 can tackle without perf costs for the generic use case.

As a path forward, you'll need to raise this with the V8 team.

[benjamingr]

The fact bluebird doesn't support async hooks is totally our fault for not being good maintainers and taking good care of the code - there has been a PR open for like a year but we haven't reviewed it yet because we prefer stability over taking a risk and shipping something bad in a library so stable - and we point out people to native promises anyway :)

[joebowbeer]

@benjamingr Now that bluebird supports async hooks (e1c4a913) (right?) I'm interested to know if there's any update to this issue.

[sam-github]

It seems async_hooks is foundering on the same core problem that caused Domains to be deprecated - the fact that certain classes of user code (connection poolers in Domains, thenables in async_hooks - at least) must actively participate in the maintenance of the state.

Is hacking await from v8 out of the question?

[othiym23]

The main issue to implement this could be that await is a native JavaScript construct interpreted directly by v8, meaning there may be no way for Node to hook into it directly. If that's the case, then I would tend to agree with @netproteus that async_hooks as they stand will never work and that there must be a solution built into v8.

There is a proposal, first made in 2016, to move Angular's Zones, which are essentially the same thing as async_hooks but for a browser context, into ECMAscript. It has never moved beyond stage 0, but it is the most established route for getting user-accessible asynchronous context propagation added to V8. However, adding support for generic or unbranded thenables is something that would need to happen in addition to the base proposal.

It seems async_hooks is foundering on the same core problem that caused Domains to be deprecated - the fact that certain classes of user code (connection poolers in Domains, thenables in async_hooks - at least) must actively participate in the maintenance of the state.

Something that this discussion has danced around is the fact that there are cases where there are conflicts between the different use cases for async context propagation that require some knowledge of the intention of the code to resolve.

@AndreasMadsen has delimited the problem space pretty clearly around promises, so I'll re-include his description here:

Because there multiple places in your code that you may consider the "start".

• new Promise(): this would be the init event like you suggest.
• resolve(value): instead of init you should use the promiseResolve event.
• .then(): calling .then() creates a new promise. Instead of the init event of the parent promise, use the init of this "chainedPromise".

For example, when using something like continuation-local storage with a connection driver, like older versions of node-redis, that may be demultiplexing data coming over a single connection to a server into responses intended for multiple listeners, it's impossible for the code to derive automatically – without a hint in the form of glue code added by the driver maintainers – where the context should be joined / propagated to the asynchronous execution flow – should it be for the connection pool globally? Per listener? Per connection? It's immediately apparent to (most) users what the right thing is to do, and there are heuristic patterns that can be applied, but it's not something that can be derived completely automatically.

I see the problem with domains as being similar but different – it's also a mismatch between user expectation and what the functionality is intended to do. Domains have a similar problem with context propagation (which is why they were unsuitable for continuation-local-storage), but more fundamentally, users really want them to be a way to automatically recover when the running application has been put into an undefined state, which is both a reasonable desire and technically infeasible.

Where domains and async_hooks are similar is that they're both leaky abstractions. I disagree that this renders either one completely unfit for purpose, but it does make them harder to use and less desirable for a lot of use cases, like the ones that @medikoo and @netproteus outline. I've thought about this a lot over the years, and I just don't think this is a problem that can be completely solved from within the platform without _some_ support from package and application developers. People writing APM and developer tooling will always need to be aware of the special use cases of third-party code like Knex and the like. We can smooth a lot of the rough edges, but I don't think they can be completely eliminated.

Is hacking await from v8 out of the question?

Patching await or the microservices queue to connect AsyncResources to thenables would be a large improvement. I get why node maintainers don't want to support floating patches to V8, but they're not too uncommon in the Electron world and it's only a moderate pain. It would be nice for something like Zones to land in V8, but the proposal hasn't moved out of stage 0 for three years now, so it's probably not going to land anytime soon.

[Qard]

@sam-github It's possible. Someone just needs to put the time into it.

I already discussed it with some V8 folks a few months ago and the conclusion seemed to be that a change for that would be fine, but it's not a priority for them to work on it. I had _hoped_ to find some time to dig into it myself, but I haven't got to it yet. 😞

[Flarna]

I'm quite sure that a lot problems seen now (e.g. when "starts" a promise based transaction, support from libraries using connection pooling,...) would get solved or at least less problematic on the long run once await is fully supported by AsyncHooks.
I have the impression that more and more libraries/applications move from callback/pure Promise style to await and this allows to get the logical application flow automatically - as a result less need to move this responsibility into libraries.
At least this is the impression after discussion with colleagues working with .NET where await is used heavily since a while (and AsyncLocal is there too support CLS) and as far as I know they never had callback/Promise style Apis like Node.

As @qard said it's "just" a matter of doing the work - but this is not an easy task for a v8 newbie. Not even sure if it is easy for v8 experts to get this functional correct _and_ efficient.

[AndreasMadsen]

has delimited the problem space pretty clearly around promises, so I'll re-include his description here

@othiym23 Having thought about this for some years now. I'm now convinced that .then() is the correct choice for async_hooks and that other starting points should be exposed through a promise_hooks module without adding complexity to async_hooks. Unfortunately, that is not how it is currently done.

[gireeshpunathil]

In the diagnostic working group meeting today, we discussed and decided to remove this from the regular agenda (folks who originally brought this up not available to make progress)

[Flarna]

Is hacking await from v8 out of the question?

I think this is the only path forward here.
I never hacked v8... is there a "get started with v8" somewhere to reduce the boarding hurdle and get faster to the actual location in V8 where to hack?

[Flarna]

I think this is fixed via https://github.com/nodejs/node/pull/33778 (included in 14.5.0).

[Qard]

Yep.

[medikoo]

Wow @Qard, thank you!