From 8741e3c7501cbe66696bd3fad4a0a923e76ba11d Mon Sep 17 00:00:00 2001 From: "JiaLi.Passion" Date: Thu, 1 Jun 2017 20:28:06 +0900 Subject: [PATCH] async_hooks: use parent promise as triggerId async_hooks init callback will be triggered when promise newly created, in previous version, the parent promise which pass from chrome V8 PromiseHook is ignored, so we can't tell the promise is a pure new promise or a chained promise. In this commit, we use the parent promise's id as triggerId to trigger the init callback. Fixes: https://github.com/nodejs/node/issues/13302 PR-URL: https://github.com/nodejs/node/pull/13367 Reviewed-By: Andreas Madsen Reviewed-By: Anna Henningsen --- src/async-wrap.cc | 17 ++++++++ ...promise.chain-promise-before-init-hooks.js | 38 +++++++++++++++++ test/async-hooks/test-promise.js | 2 +- .../test-promise.promise-before-init-hooks.js | 42 +++++++++++++++++++ 4 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 test/async-hooks/test-promise.chain-promise-before-init-hooks.js create mode 100644 test/async-hooks/test-promise.promise-before-init-hooks.js diff --git a/src/async-wrap.cc b/src/async-wrap.cc index b5d3d959bd..0f5c800f40 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -294,6 +294,23 @@ static void PromiseHook(PromiseHookType type, Local promise, PromiseWrap* wrap = Unwrap(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->IsPromise()) { + // parent promise exists, current promise + // is a chained promise, so we set parent promise's id as + // current promise's triggerId + Local parent_promise = parent.As(); + auto parent_wrap = Unwrap(parent_promise); + + if (parent_wrap == nullptr) { + // create a new PromiseWrap for parent promise with silent parameter + parent_wrap = new PromiseWrap(env, parent_promise, true); + parent_wrap->MakeWeak(parent_wrap); + } + // get id from parentWrap + double trigger_id = parent_wrap->get_id(); + env->set_init_trigger_id(trigger_id); + } wrap = new PromiseWrap(env, promise, silent); wrap->MakeWeak(wrap); } else if (type == PromiseHookType::kResolve) { diff --git a/test/async-hooks/test-promise.chain-promise-before-init-hooks.js b/test/async-hooks/test-promise.chain-promise-before-init-hooks.js new file mode 100644 index 0000000000..2bbc8d773b --- /dev/null +++ b/test/async-hooks/test-promise.chain-promise-before-init-hooks.js @@ -0,0 +1,38 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const initHooks = require('./init-hooks'); +const { checkInvocations } = require('./hook-checks'); + +const p = new Promise(common.mustCall(function executor(resolve, reject) { + resolve(5); +})); + +p.then(function afterresolution(val) { + assert.strictEqual(val, 5); + return val; +}); + +// init hooks after chained promise is created +const hooks = initHooks(); +hooks._allowNoInit = true; +hooks.enable(); + + +process.on('exit', function onexit() { + hooks.disable(); + hooks.sanityCheck('PROMISE'); + + // Because the init event was never emitted the hooks listener doesn't + // know what the type was. Thus check for Unknown rather than PROMISE. + const as = hooks.activitiesOfTypes('PROMISE'); + const unknown = hooks.activitiesOfTypes('Unknown'); + assert.strictEqual(as.length, 0); + assert.strictEqual(unknown.length, 1); + + const a0 = unknown[0]; + assert.strictEqual(a0.type, 'Unknown'); + assert.strictEqual(typeof a0.uid, 'number'); + checkInvocations(a0, { before: 1, after: 1 }, 'when process exits'); +}); diff --git a/test/async-hooks/test-promise.js b/test/async-hooks/test-promise.js index 30cf94d28b..940e44a939 100644 --- a/test/async-hooks/test-promise.js +++ b/test/async-hooks/test-promise.js @@ -46,7 +46,7 @@ function onexit() { const a1 = as[1]; assert.strictEqual(a1.type, 'PROMISE', 'promise request'); assert.strictEqual(typeof a1.uid, 'number', 'uid is a number'); - assert.strictEqual(a1.triggerId, 1, 'parent uid 1'); + assert.strictEqual(a1.triggerId, a0.uid); // We expect a destroy hook as well but we cannot guarentee predictable gc. checkInvocations(a1, { init: 1, before: 1, after: 1 }, 'when process exits'); } diff --git a/test/async-hooks/test-promise.promise-before-init-hooks.js b/test/async-hooks/test-promise.promise-before-init-hooks.js new file mode 100644 index 0000000000..123e5e2da9 --- /dev/null +++ b/test/async-hooks/test-promise.promise-before-init-hooks.js @@ -0,0 +1,42 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const initHooks = require('./init-hooks'); +const { checkInvocations } = require('./hook-checks'); + +const p = new Promise(common.mustCall(function executor(resolve, reject) { + resolve(5); +})); + +// init hooks after promise was created +const hooks = initHooks({allowNoInit: true}); +hooks.enable(); + +p.then(function afterresolution(val) { + assert.strictEqual(val, 5); + const as = hooks.activitiesOfTypes('PROMISE'); + assert.strictEqual(as.length, 1, 'one activity'); + checkInvocations(as[0], { init: 1, before: 1 }, + 'after resolution child promise'); + return val; +}); + +process.on('exit', function onexit() { + hooks.disable(); + hooks.sanityCheck('PROMISE'); + + const as = hooks.activitiesOfTypes('PROMISE'); + assert.strictEqual(as.length, 1); + + const a0 = as[0]; + assert.strictEqual(a0.type, 'PROMISE'); + assert.strictEqual(typeof a0.uid, 'number'); + // we can't get the asyncId from the parent dynamically, since init was + // never called. However, it is known that the parent promise was created + // immediately before the child promise, thus there should only be one + // difference in id. + assert.strictEqual(a0.triggerId, a0.uid - 1); + // We expect a destroy hook as well but we cannot guarentee predictable gc. + checkInvocations(a0, { init: 1, before: 1, after: 1 }, 'when process exits'); +});