GeezerVison is showing github.com — [Feature Request]: want to get parent promise from PromiseHookInit through async_hooks · Issue #13302 · nodejs/node
[Feature Request]: want to get parent promise from PromiseHookInit through async_hooks #13302Description
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! Activityjasnell commented on May 30, 2017 JiaLiPassion commented on May 30, 2017 ContributorAuthor More actions @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 commented on May 30, 2017 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 commented on May 30, 2017 ContributorAuthor More actions @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 commented on May 30, 2017
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 commented on May 30, 2017 ContributorAuthor More actions changed the title [-][Question]: Is possible to get parent promise from PromiseHookInit through async_hooks[/-] [+][Feature Request]: want to get parent promise from PromiseHookInit through async_hooks[/+] on May 31, 2017JiaLiPassion commented on May 31, 2017 ContributorAuthor More actions AndreasMadsen commented on May 31, 2017 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 commented on May 31, 2017 ContributorAuthor More actions @AndreasMadsen , thank you, so I will just leave this issue open here. AndreasMadsen commented on May 31, 2017
thank you, so I will just leave this issue open here.
Exactly! JiaLiPassion commented on Jun 1, 2017 ContributorAuthor More actions @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 commented on Jun 1, 2017 @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 commented on Jun 1, 2017 ContributorAuthor More actions @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 commented on Jun 1, 2017
I think parent will always be promise or undefined, is that right?
Yes JiaLiPassion commented on Jun 1, 2017 ContributorAuthor More actions @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 commented on Jun 1, 2017 addaleax commented on Jun 1, 2017 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 commented on Jun 1, 2017 ContributorAuthor More actions JiaLiPassion commented on Jun 1, 2017 ContributorAuthor More actions @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
- keep the current
init callback, and add a new API such as async_hooks.triggerId() to get the parent promise in this case.
or
- add a new parameter in
init callback, so it will become
init(id, type, parentId, parentHandle, triggerId)
is that correct? AndreasMadsen commented on Jun 1, 2017
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().
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|