Async Hooks do not recognize execution contexts created when processing thenables #22360
Comments
|
As I see, it goes down to 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: |
I think this is how to produce what you expected: Produces: |
|
@firedfox indeed I forgot about Note: it's about an extra event loop in which |
|
I've updated examples a bit more to better expose that we deal with different event loop, which is not reflected by |
|
Sorry I finally understand what you mean. You are right. It seems to be a problem. Produces: |
|
@firedfox I don't understand how your comment is relevant to this report? Again, bug is that async contexts in which This bug makes As long as it's the case, reliable long stack trace solution cannot be configured using |
|
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 |
I'm not sure if it's a valid explanation, as here it's native I suppose that if I take that code responsible for that is in C++ layer (?) |
|
The state of You can visualize the difference using the clinic bubbleprof tool thenable: Promise.all([
{
then() {
console.log('a')
}
}
])promise: Promise.all([
Promise.resolve('a').then(console.log)
])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. |
|
I don't see those two examples equivalent. In first one Anyway to make sure I understand you right - you acknowledge there's an event loop introduced by native |
|
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? :) |
|
@medikoo I'm just wandering by, but does this comment in the google stackdriver tracer address/work around your observation? |
|
@stephenh it doesn't look related to this issue. |
@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? |
|
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
changed the title
|
Thanks @AndreasMadsen I just checked and indeed that can be workaround with While 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 |
|
The only subtle issue is that, such new async resource should be created only in case |
|
Alternativly, just make Thenable inherit from AsyncResource. What you prefer, depends on how you want the async context to flow. |
As far as I understand it that won't work, as we need Generally current means do not leave us with any solid solution for that problem. As even if we hack in as a |
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: 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);
}
} |
|
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 So e.g. in following example result should be that 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:
|
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".
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. |
|
In case I'm after It's at So while
|
|
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. |
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 |
|
I don't think this has anything to do with it being a In this case, use the 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 |
|
I'll try to clarify it further:
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:
Won't work for this use case. |
|
@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. 😞 |
|
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 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. |
@othiym23 Having thought about this for some years now. I'm now convinced that |
gireeshpunathil
removed
the
diag-agenda
|
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) |
I think this is the only path forward here. |
|
I think this is fixed via #33778 (included in 14.5.0). |
|
Yep. |
|
Wow @Qard, thank you! |
Promise.allto observe input promises, invokespromise.thenin 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:
The outcome is:
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.
The text was updated successfully, but these errors were encountered: