@AndreasMadsen , about the sample code you provided,

Consider the following code:

const p0 = new Promise((resolve) => resolve(1));
const p1 = p0.then(function () {
  // async_hooks.currentId() === asyncId(p1)
  p2 = p1.then(function () { // trigger_id = asyncId(p1)
    
  });
  return 2;
});

I make a test with this PR compiled node.

const async_hooks = require('async_hooks');

function init(id, provider, parentId, parentHandle) {
    process._rawDebug('init: ', id, 'triggerId: ', parentId, 'currentId',  async_hooks.currentId());
}

function before(id) { 
    process._rawDebug('before', id, async_hooks.currentId());
 }
function after(id) { 
    process._rawDebug('after', id, async_hooks.currentId());
 }
function destroy(id) {
    process._rawDebug('destroy', id);
}

async_hooks.createHook({init, before, after, destroy}).enable();
debugger;
const p = new Promise ( resolve => {
    process._rawDebug('execute p');
    resolve(1);} );
const p1 = p.then(val => { 
    process._rawDebug('p1 then', val); 
    const p2 = p1.then((val1) => {
      process._rawDebug('p2 then', val1); 
    });
    return 2; });

the output is

init:  2 triggerId:  1 currentId 1
execute p
init:  3 triggerId:  2 currentId 1
before 3 1
p1 then 1
init:  4 triggerId:  3 currentId 1
after 3 1
before 4 1
p2 then 2
after 4 1

p is promise without parent, so triggerId === currentId
p1 is promise , and p1's parent is p, so the triggerId is 2
p2 is promise , and p2's parent is p1, so the triggerId is 3,

I think this a correct result.

And I tried another case

setTimeout(() => {
    const p = new Promise(resolve => {
        process._rawDebug('execute p');
        resolve(1);
    });
    const p1 = p.then(val => {
        process._rawDebug('p1 then', val);
        const p2 = p1.then((val1) => {
            process._rawDebug('p2 then', val1);
        });
        return 2;
    });

}, 100);

the output is

init:  2 type: Timeout triggerId:  1 currentId 1
init:  3 type: TIMERWRAP triggerId:  1 currentId 1
before 3 3
before 2 2
init:  4 type: PROMISE triggerId:  2 currentId 2
execute p
init:  5 type: PROMISE triggerId:  4 currentId 2
after 2 2
after 3 3
before 5 0
p1 then 1
init:  6 type: PROMISE triggerId:  5 currentId 0
after 5 0
before 6 0
p2 then 2
after 6 0
destroy 2

I think the result is also ok, but I don't quite understand the why currentId will become 0.

Please review. Thank you very much!

Interesting, I think something is wrong with async_hooks.currentId().

init:  2 triggerId:  1 currentId 1
execute p
init:  3 triggerId:  2 currentId 1
before 3 1
p1 then 1
init:  4 triggerId:  3 currentId 1 // wrong, currentId should be 3
after 3 1
before 4 1
p2 then 2
after 4 1

PS: I very much doubt this change is responsible for the bug.

/cc @nodejs/async_hooks

@AndreasMadsen, I am not sure I understand the lifecycle of currentId, so I do not know what is the correct result,just like the another sample involve setTimeout will have a currentId 0,I do not understand why.

@JiaLiPassion The async_hooks.currentId() should just be the id from the latest before emit that hasn't been closed by the after emit.

In some cases, there is no before emit (and it is also not the root) then currentId will be 0, but that is not the case in both examples.

@AndreasMadsen , Thank you for the explanation.

The async_hooks.currentId() should just be the id from the latest before emit that hasn't been closed by the after emit.

Yeah, this is making sense.

In some cases, there is no before emit (and it is also not the root) then currentId will be 0, but that is not the case in both examples.

you mean there is only init emit, is that right?

And yes, in my setTimeout example, there is before emit, so I don't know why there is a 0 currentId. ( I have tried the nightly build version of nodejs, the currentId result is the same).

you mean there is only init emit, is that right?

For example in the TCP server you will get:

init TCP socket
before TCP server onconnection
....
after TCP server onconnection

This is because the TCP socket is created in C++ land and then send to the callback. Since this happens before any javascript the currentId is zero.

The code change looks good to me, aside from one edge case. I still am not 100 % sure about this being the right thing, but if Andreas says so, I trust him.

Sorry to burst your bubble, but the triggerId is something I really struggle with. I tried to explain some of my confusion in #11883 (review). Unfortunately, I still haven't gotten any wiser regarding when to set triggerId and when to let it default to currentId.

Also since this doesn't solve the problem that @JiaLiPassion originally asked, regarding detecting if a parentPromise exists, I would like to explore some alternatives.

tl;dr; I think this makes sense, but I'm generally uncertain about triggerId.

I would still really like to see a test that shows that getting the original execution scope remains possible, because that is something that actual use cases will rely on.

Yeah, we need to fix the currentId bug before that can happen. I think the test case that should follow with that should be sufficient.

@AndreasMadsen Regarding triggerId, I can definitely see that there are at least two valid perspectives:

• If it’s about tracking asynchronous causality, this PR might be the right one. It would make a Promise look like it descended from where the origin of the Promise chain was created. I am not sure that’s super helpful, though; a Promise can be resolved with another Promise, breaking the causality chain, and afaik we can’t track that well currently.
• If it’s about tracking asynchronous context, this PR is not the right one; basically, if you want to have the code after an await in an async fn look like it runs in a context descended from the one before await, then the current behaviour is what you want.

Writing this comment, it still seems to me like we should maybe just expose the parent Promises async id as an extra argument to the init callback.

@addaleax It is the first one, but there is a thin line between causality and parent resource, which is what causes my confusion.

From the documentation:

triggerId is the asyncId of the resource that caused (or "triggered") the
new resource to initialize and that caused init to call. This is different
from async_hooks.currentId() that only shows when a resource was created,
while triggerId shows why a resource was created.

@AndreasMadsen , @addaleax ,

Writing this comment, it still seems to me like we should maybe just expose the parent Promises async id as an extra argument to the init callback.

So if expose the parent Promise async id as an extra argument, it can resolve my original issue, that if the argument exists, the the promise is a chained one, is that correct?

If it’s about tracking asynchronous context, this PR is not the right one; basically, if you want to have the code after an await in an async fn look like it runs in a context descended from the one before await, then the current behaviour is what you want.

do you mean the case like this?

async function test() {
    await new Promise(resolve => resolve(1));
}

process._rawDebug('before', async_hooks.currentId());
test();
process._rawDebug('after', async_hooks.currentId());

@AndreasMadsen Okay, makes sense.

So if expose the parent Promise async id as an extra argument, it can resolve my original issue, that if the argument exists, the the promise is a chained one, is that correct?

@JiaLiPassion Yes, I’d say so.

do you mean the case like this?

More like this:

async function test() {
  process._rawDebug('before', async_hooks.currentId());
  await new Promise(resolve => resolve(1));
  process._rawDebug('after', async_hooks.currentId());
}

Writing this comment, it still seems to me like we should maybe just expose the parent Promises async id as an extra argument to the init callback.

I think extra arguments to init just for the purpose of promises is overdoing it. That additional API will no purpose for any other resource in async_hooks but promises. If some information is just necessary for promises then I think a documented PromiseWrap resource is the way to go.

If those informations are just necessary for promises then I think a documented PromiseWrap resource is the way to go.

That works for me; but it means that we’d have to switch the resource argument that’s passed to init from the promise itself to a distinct object, right?

That works for me; but it means that we’d have to switch the resource argument that’s passed to init from the promise itself to a distinct object, right?

I think that would be the best. It is also what we discussed in the original Promise PR and what @Fishrock123 advocated for.

@AndreasMadsen , sorry I just lost...

That works for me; but it means that we’d have to switch the resource argument that’s passed to init from the promise itself to a distinct object, right?
I think that would be the best. It is also what we discussed in the original Promise PR and what @Fishrock123 advocated for.

Do you mean use AsyncResource in async_hook.js to pass the parent promise information?

Do you mean use AsyncResource in async_hook.js to pass the parent promise information?

Sort of, but we definitely shouldn't go through JS for that. For the purpose of a resource object, AsyncResource is just an object. The resource object should look something like:

resource = {
  promise // reference to promise,
  parentPromise // reference to parentPromise or undefined
}

@AndreasMadsen , can I just change the how to create a new PromiseWrap to something like

    Local<Object> promise_resource = Object::New(env->isolate());
    FORCE_SET_TARGET_FIELD(promise_resource, "promise", promise);
    FORCE_SET_TARGET_FIELD(promise_resource, "parentPromise", parent_promise);

    wrap = new PromiseWrap(env, promise_resource, silent);
 

@JiaLiPassion I don't think that is enough. This is as far as my V8-C++ knowledge goes, I'm not sure how to tie in the GC of promise with promise_resource (this is important for emitting destroy) and I don't know the most performant way of doing this.

@hayes Thanks for the explanation. I misunderstood what you meant by triggerId. Please see #13000 (comment)

Plese create an issue, but don't do it around the idea the triggerId is wrong. Just around the idea that we don't have the kResolve timing information.

quick point of clarification, setting the triggered as I am proposing here does not prevent any existing behavior, it simply adds the ability to link the execution of a callback back to the async resource that caused it to be scheduled.

I'm pretty sure it does. But we can discuss that in the issue.

Please keep this issue on track, this PR is just about fixing triggerId

@hayes will create issues for the other problems. If something is missing you are welcome to create a new issue.

@AndreasMadsen , I have updated the test cases and assert message.
#13367 (comment)

please review.

@JiaLiPassion Yes, that is how you’d check whether a parent promise exists. (Now that you mention it: Maybe handle.parent should refer to the parent PromiseWrap, not the parent promise itself. I’m not sure.)

@addaleax , got it , thank you very much!!!

Would you please make another PR to implement it?

Would you please make another PR to implement it?

@JiaLiPassion I’ll wait until you land this PR, then I can do that. :)

Left a few style nits.

CI: https://ci.nodejs.org/job/node-test-commit/10333/

I will fix these, there are some other changes I would like.

@AndreasMadsen , ok, thank you! so I don't need to update the source which were reviewed by @addaleax ?

@JiaLiPassion yep, I fixed it for you :)

I didn't really understand why the triggerId was what it was, so I checked out your code and investigated it. Nothing was wrong, but I made it a bit more robust for future changes.

@AndreasMadsen , thank you very much!!! you and @addaleax give me great help!!!

Landed in 8741e3c 🎉

@JiaLiPassion I believe this is your first commit to core, if so, welcome on board! Thank you for the work you have been putting into this, and I think I speak for everyone in this thread if I say that we’d love to see more of that. If you’re interested, we can probably add you to the @nodejs/async_hooks review team that gets @mentioned on issues and PRs related to async_hooks.

@jasnell I’ve left you off as a reviewer because the PR has undergone quite some change since you reviewed, and wasn’t really ready at that point anyway. I hope that’s okay.

@addaleax , Thank you so much for all the great help, I am so glad to join here and definitely want to do more in the future.

If you’re interested, we can probably add you to the @nodejs/async_hooks review team that gets @mentioned on issues and PRs related to async_hooks.

Yes!!! I am very happy to join here and it will be my honor to join @nodejs/async_hooks review team to do more contributions and want to learn more from here.

Thank you and @AndreasMadsen again for all the great help!!!