Feature Request: Regular Promises vs. async/await Promises with async_hooks

[astormnewrelic]

Is your feature request related to a problem? Please describe.

This is a follow on feature request (or perhaps secretly a question again) from a previous feature request -- please see discussion here for more context on the use case. On @addaleax's suggestion I'm exploring whether async_hooks can solve this problem. I'm trying to create tooling that will programmatically identify cases of deep recursion in an async functions for code I didn't write.

My understanding of the current async_hooks API is that calling a function that looks
like this

 const foo = async () => {
   await bar()
   return 'foo'
 }

will initializes (i.e. the init callback for an async hook) three async-resources whose type is PROMISE.

There's one promise for the return from an async function.

Then there's two promises from await bar(). As @jasnell mentioned in the other thread, await is just syntactic sugar that turns await bar() into something roughly equivalent to
bar().then([the code after await]) -- I presume these two promises from await is some of the rough in roughly equivalent)

Describe the solution you'd like
My naive feature request would be for async hooks to differentiate between "regular" promises and promises that are created on the end-user-programmer's behalf (i.e. the promises from returning from an async function or the two promises created from an await). I don't know enough to suggest whether this would be different types, or via properties on the PromiseWrap, or some third thing.

If this isn't possible/feasible -- I'd be interested in learning where/how Node ends up calling the callbacks for an async hook. I was able to track things down myself to
emitHook which seems to be the boundary between javascript code and native code, but it's not clear to me how or where the native code calls emitHook. I realize this may be a rather large question -- while I'd love a large answer, a "here's the file and line number in native code where
emitHook is invoked" would be perfectly acceptable.

Describe alternatives you've considered
We tried the --stack-size option, but this post-await recursion doesn't grow the stack.

[addaleax]

@astormnewrelic As a starting point, #32891 might be something you want to look at if you want to understand how exactly Node.js is integrated with V8 when it comes to tracking Promises. That PR changes how the data is transmitted in a few cases, so basically the entire sequence of events from C++ to JS should be visible in there. (Plus, it doesn't just show the current state of that, but also a possible future state of it.)

I'm not sure what the current state is exactly, but we've run into the issue before that await can take any thenable as an argument, not just a built-in Promise, and that that doesn't necessarily integrate well with async tracking -- for example, bluebird provides its own custom AsyncResource implementation to allow Node.js to track what it does.

/cc @nodejs/async_hooks

[puzpuzpuz]

I'm not sure what the current state is exactly, but we've run into the issue before that await can take any thenable as an argument, not just a built-in Promise, and that that doesn't necessarily integrate well with async tracking -- for example, bluebird provides its own custom AsyncResource implementation to allow Node.js to track what it does.

This problem is even worse, considering that bluebird starts using AsyncResource (thus, becomes async_hooks-friendly) only when configured (requires v3.6.0+).

@astormnewrelic you should be looking at the PromiseHook native function which is the main V8->Node.js entry point for native Promise support in async_hooks:

node/src/async_wrap.cc

Line 229 in c15a27c

static void PromiseHook(PromiseHookType type, Local<Promise> promise,

AFAIK Node.js, as the runtime, is not aware of async/await syntax, i.e. the support is implemented (and encapsulated) in V8. That makes me thinking that the requested feature has to be implemented in V8, so it would send additional information when calling PromiseHook function.

[Qard]

It's possible, though fairly awkward, to differentiate between raw promises and async/await just with async_hooks events.

As for thenables: they are still broken. V8 needs a change to emit PromiseHook events when awaiting a thenable. Currently the then(...) method will be run in a phantom microtask tick with no events emitted for it, so any continuation-tracking built with async_hooks will be lost.

[puzpuzpuz]

It's possible, though fairly awkward, to differentiate between raw promises and async/await just with async_hooks events.

Sounds interesting. Could you describe this logic?

As for thenables: they are still broken. V8 needs a change to emit PromiseHook events when awaiting a thenable. Currently the then(...) method will be run in a phantom microtask tick with no events emitted for it, so any continuation-tracking built with async_hooks will be lost.

So that phantom microtask tick leads for context loss even for well-behaved thenables which use AsyncResource API? That's sounds like a no-go for thenables in combination with async_hooks.

[Qard]

The logic is complicated and difficult to describe specifics, but basically you need to analyze the timing of events. In async/await, inits are grouped together at the start of the outer promise, so you can look for a specific shape to the event timeline. It's not 100% accurate though--could easily mistakes one for the other.

As for how thenables and AsyncResource work together: it depends on the implementation. If you create an AsyncResource before giving the object to the await and just doing the before/after part in the then(...) handler, then it should be fine. If I recall correctly though, trying to create the AsyncResource inside the then(...), but not the callback, failed because the whole call to then(...) is placed inside the phantom microtask.

[mmarchini]

AFAIK Node.js, as the runtime, is not aware of async/await syntax, i.e. the support is implemented (and encapsulated) in V8. That makes me thinking that the requested feature has to be implemented in V8, so it would send additional information when calling PromiseHook function.

I might be wrong, but I think there's also some language spec requirements which force V8 to create at least two Promises when dealing with async/await: one promise when calling the async function, another when awaiting on an async function. I think it was four in the past: one when calling an async function, three when awaiting (well, one promise plus two microtask ticks), but the V8 team was able to propose changes to TC39 to improve the situation, which can be observed when comparing async_hooks between Node.js v10 and v12:

require("async_hooks").createHook({init: (asyncId, type) => process._rawDebug(asyncId, type)}).enable();

async function bar() {}

async function foo() { await bar() }

foo()

$ nvm use 10
Now using node v10.20.1 (npm v6.14.4)

$ node index.js
5 'PROMISE'
6 'PROMISE'
7 'PROMISE'
8 'PROMISE'
9 'PROMISE'

$ nvm use 12
Now using node v12.16.2 (npm v6.14.4)

$ node index.js
2 PROMISE                                                                                                                                             
3 PROMISE                                                                                                                                             
4 PROMISE                                                                                                                                             

Note we have three outputs on v12, one for the foo() call, one for the bar() call, and one for await .... If we remove await from the code above, we get one output for each async function called:

$ nvm use 10
Now using node v10.20.1 (npm v6.14.4)

$ node index.js
5 'PROMISE'
6 'PROMISE'

$ nvm use 12
Now using node v12.16.2 (npm v6.14.4)

$ node index.js
2 PROMISE
3 PROMISE

The V8 team has a nice writeup on this in https://v8.dev/blog/fast-async#await-under-the-hood, and the normative change to ECMAScript spec was merged in tc39/ecma262#1250.

[jasnell]

Closing this given that it's just not clear what there is to do (if anything) and there's been no further discussion.