[Feature Request]: want to get parent promise from PromiseHookInit through async_hooks

[JiaLiPassion]

• Version:
  v8.0.0-nightly20170527f84666f923

• Platform:

• Subsystem:

Chrome V8 PromiseHook init hook's signature is PromiseHookInit(promise,parentPromise),
current async_hooks seems not support to get the parentPromise, is that possible to get the
the parentPromise from async_hooks init callback?

Thank you very much!

[jasnell]

Ping @trevnorris @addaleax

[JiaLiPassion]

@jasnell , thank you ! I need this parent promise information to identify the promise which was newly created in Promise.then or not.

Also ping @AndreasMadsen, @matthewloring !

from the PromiseHook Sample https://docs.google.com/document/d/1rda3yKGHimKIhg5YeoAmCOtyURgsbTH_qaYR79FELlk/edit#

there is code like this.

const async_wrap = process.binding('async_wrap');
const chain = new Map();
const startTimes = new Map();
const endTimes = new Map();
let root;
 
function init(id, parentId) {
    if (parentId !== undefined) {
        chain.set(parentId, id);
    } else {
        root = id;
    }
}
 
function before(id) { startTimes.set(id, Date.now()); }
function after(id) { endTimes.set(id, Date.now()); }
 
async_wrap.setupHooks({init, before, after});
async_wrap.enable();
 
doSomeAsyncOp()
  .then(() => { .. })
  .then(() => { .. })
 
async_wrap.disable();
 
let totalTime = 0;
while(root = chain.get(root)) {
    totalTime += (endTimes.get(root) - startTimes.get(root));
}
 
console.log(totalTime);

const p = new Promise ( resolve => resolve(1) );
const p1 = p.then(val => { console.log(val == 1); return 2; });
const p2 = p.then(val => console.log(val == 1));
  

the call order will like

init( p )
init( p1, p )
init( p2, p )
 
before( p1 )
after( p1 )
 
before( p2 )
after( p2 )

This is the expected result, but current nodejs 8 nightly build, the code is

const async_hooks = require('async_hooks');

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

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

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

and the output is

init  2 PROMISE 1 {}
init  3 PROMISE 1 {}
init  4 PROMISE 1 {}
before 3
after 3
before 4
after 4

The parentId are all 1.

edit (AndreasMadsen): added syntax highlighting

[AndreasMadsen]

I need confirmation that I understand the question/feature request correctly.

Consider this code:

const async_hooks = require('async_hooks');

async_hooks.createHook({
  init(id, type, triggerId, resource) {
    process._rawDebug('init', {id, type, triggerId, resource});
  },
  before(id) {
    process._rawDebug('before', {id});
  },
  after(id) {
    process._rawDebug('after', {id});
  },
  destroy(id) {
    process._rawDebug('destroy', {id});
  }
}).enable();

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

const p3 = p2.then(function (val) {
  return 2;
});

This results in the following output:

init { id: 2,
  type: 'PROMISE',
  triggerId: 1,
  resource: Promise { <pending> } }
init { id: 3,
  type: 'PROMISE',
  triggerId: 1,
  resource: Promise { <pending> } }
before { id: 3 }
after { id: 3 }

You would like the init created by promise p3 (has id = 3) to have the trigger_id of promise p2 (has id = 2).

I think that is fair, it shouldn't be difficult implement. Honestly, the trigger id stuff is a little confusing to me, I would like to see a clear line when we set the trigger_id and when we just let it be the current_id.

[JiaLiPassion]

@AndreasMadsen , thank you for the reply.

You would like the init created by promise p3 (has id = 3) to have the trigger id of promise p2 (has id = 2).

Yes, that will help.

And If the implementation become like this, I have another question is

Init { id: 2,
type: 'PROMISE',
triggerId: 1,
resource: Promise { } }

Can I say that triggerId is 1 means this Promise is just a newly created Promise without any parent chain?

[AndreasMadsen]

Can I say that triggerId is 1 means this Promise is just a newly created Promise without any parent chain?

Not always, triggerId = 1 is just the trigger_id for the root, if you create a promise inside the callback of another asynchronous operation you will get another trigger_id.

If we implement as suggested, you can check if triggerId === async_hooks.currentId(). If that is true then the promise has no parent (there might be an edge case I haven't thought of).

[JiaLiPassion]

@AndreasMadsen , Thank you! Got it!

[JiaLiPassion]

@AndreasMadsen , should we add this issue to nodejs/diagnostics#29 as a feature request? Thank you!

[AndreasMadsen]

I want to close nodejs/diagnostics#29 as soon as documentation, N-API, and NAN is fixed. From a management perspective, we are moving from a project management to maintenance management, thus I see no need to create roadmaps etc.. If there are issues in nodejs/diagnostics#29 that have not been fixed by then I will create matching issues in nodejs/node so we can just filter by the async_hooks label.

[JiaLiPassion]

@AndreasMadsen , thank you, so I will just leave this issue open here.

[AndreasMadsen]

thank you, so I will just leave this issue open here.

Exactly!

[JiaLiPassion]

@AndreasMadsen , I tried to understand how to implement this one, I read the source in async-wrap.cc

static void PromiseHook(PromiseHookType type, Local<Promise> promise,
                        Local<Value> parent, void* arg) {
  Local<Context> context = promise->CreationContext();
  Environment* env = Environment::GetCurrent(context);
  PromiseWrap* wrap = Unwrap<PromiseWrap>(promise);
  if (type == PromiseHookType::kInit || wrap == nullptr) {
    bool silent = type != PromiseHookType::kInit;
    wrap = new PromiseWrap(env, promise, silent);
    wrap->MakeWeak(wrap);
  } else if (type == PromiseHookType::kResolve) {
    // TODO(matthewloring): need to expose this through the async hooks api.
  }
  CHECK_NE(wrap, nullptr);
  if (type == PromiseHookType::kBefore) {
    PreCallbackExecution(wrap, false);
  } else if (type == PromiseHookType::kAfter) {
    PostCallbackExecution(wrap, false);
  }
}

When type=PromiseHookType::kInit, the parent promise is ignored, so the trigger id is still 1,
I wonder how to get the asyncId from the parent promise? Should we add the asyncId into the AsyncWrapper Class?

I am very new to c++,sorry for the basic questions.

Also ping @matthewloring , thank you so much for the great work to add PromiseHook into async_hooks, please help to look into this when you have time. I want to add this into zone.js for nodejs v8.x.

Thank you very much!

[AndreasMadsen]

@JiaLiPassion I think you can get the id of the parent promise with trigger_id = Unwrap<PromiseWrap>(parent)->get_id() and pass it to new PromiseWrap with env->set_init_trigger_id(trigger_id). new PromiseWrap does not take the trigger_id as a parameter, but instead reads it from a global variable.

Of course parent is not always a promise in which case this will break.

[JiaLiPassion]

@AndreasMadsen , Thank you very much!

Of course parent is not always a promise in which case this will break.

the code here,

static void PromiseHook(PromiseHookType type, Local<Promise> promise,
                        Local<Value> parent, void* arg) {

and the spec here, https://docs.google.com/document/d/1rda3yKGHimKIhg5YeoAmCOtyURgsbTH_qaYR79FELlk/edit#

PromiseHookInit(promise,parentPromise)
 
PromiseHookInit is called when a new promise is created. When a new promise is created as part of the chain in the case of Promise.then or in the intermediate promises created by Promise.{race, all}/AsyncFunctionAwait, we pass the parent promise otherwise we pass undefined.

I think parent will always be promise or undefined, is that right?

[AndreasMadsen]

I think parent will always be promise or undefined, is that right?

Yes

[JiaLiPassion]

@AndreasMadsen , sorry for so many basic questions, I don't know how to write C++ code at all, basically the updated logic will look like

static void PromiseHook(PromiseHookType type, Local<Promise> promise,
                        Local<Value> parent, void* arg) {
  Local<Context> context = promise->CreationContext();
  Environment* env = Environment::GetCurrent(context);
  PromiseWrap* wrap = Unwrap<PromiseWrap>(promise);
  if (type == PromiseHookType::kInit || wrap == nullptr) {
    bool silent = type != PromiseHookType::kInit;
    // set parent promise's async Id as this promise's triggerId
    if (!parent->IsUndefined()) {
      // parent promise exists, current promise
      // if a chained promise
      double trigger_id = Unwrap<PromiseWrap>(parent)->get_id();
      env->set_init_trigger_id(trigger_id);
      wrap = new PromiseWrap(env, promise, silent);
    } else {
      wrap = new PromiseWrap(env, promise, silent);
    }
    wrap->MakeWeak(wrap);
  } else if (type == PromiseHookType::kResolve) {
  ....

Is that correct? And when I compile, it seems

      double trigger_id = Unwrap<PromiseWrap>(parent)->get_id();

cause compile error with

../src/async-wrap.cc:303:47: note: in instantiation of function template specialization 'v8::Local<v8::Object>::Local<v8::Value>'

And if we call

      env->set_init_trigger_id(trigger_id);

When should we clear this trigger_id?

Thank you for your time!

[AndreasMadsen]

cause compile error with

I think you need to cast the Local <Value> type to an Local<Object> type. I'm no V8-C++ expert either, so I don't remember how. https://v8docs.nodesource.com/ is useful for looking up API.

edit: okay, I guess ->ToObject() https://v8docs.nodesource.com/node-8.0/dc/d0a/classv8_1_1_value.html#aa3a511d716854457a3faba9995803d85 will cast it :)

When should we clear this trigger_id?

That is done automatically when you call new PromiseWrap.

[addaleax]

parent can be undefined, so you’ll need to check parent->IsPromise() first. If that succeeds, parent.As<Promise>() will give you a Local<Promise>.

I haven’t read the full thread here, but it seems the idea is to make the triggerId for Promises come from the parent Promise, not from the context which actually created the Promise.

I agree that exposing it seems like a good idea, but I think the creation context id needs to remain accessible in some way; So we might need an API change, either in the style of async_hooks.triggerId() or by adding a new parameter to init for that.

(For example, I think for use cases like continuation-local storage the current code actually makes more sense.)

[JiaLiPassion]

@AndreasMadsen , Thank you very much! I will try it.

[JiaLiPassion]

@addaleax , Thank you for reply.

I haven’t read the full thread here, but it seems the idea is to make the triggerId for Promises come from the parent Promise, not from the context which actually created the Promise.

Yes, I want to know the newly created promise is a then chained promise or not.

I agree that exposing it seems like a good idea, but I think the creation context id needs to remain accessible in some way; So we might need an API change, either in the style of async_hooks.triggerId() or by adding a new parameter to init for that.

(For example, I think for use cases like continuation-local storage the current code actually makes more sense.)

So, you suggest we

1. keep the current init callback, and add a new API such as async_hooks.triggerId() to get the parent promise in this case.

or

2. add a new parameter in init callback, so it will become

init(id, type, parentId, parentHandle, triggerId)

is that correct?

[AndreasMadsen]

I agree that exposing it seems like a good idea, but I think the creation context id needs to remain accessible in some way;

@addaleax We have that, it is async_hooks.currentId().