From 1a2cf6696fa59c9723c423c4ff4c1167ab82155c Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Fri, 12 Oct 2018 08:56:26 +0200 Subject: [PATCH] async_hooks: remove promise object from resource While it doesn't make any difference now. In the future PromiseHooks could be refactored to provide an asyncId instead of the promise object. That would make escape analysis on promises possible. Escape analysis on promises could lead to a more efficient destroy hook, if provide by PromiseHooks as well. But at the very least would allow the destroy hook to be emitted earlier. The destroy hook not being emitted on promises frequent enough is a known and reported issue. See https://github.com/nodejs/node/issues/14446 and https://github.com/Jeff-Lewis/cls-hooked/issues/11. While all this is speculation for now, it all depends on the promise object not being a part of the PromiseWrap resource object. Ref: https://github.com/nodejs/node/issues/14446 Ref: https://github.com/nodejs/diagnostics/issues/188 PR-URL: https://github.com/nodejs/node/pull/23443 Refs: https://github.com/nodejs/node/issues/14446 Refs: https://github.com/nodejs/diagnostics/issues/188 Reviewed-By: Benedikt Meurer Reviewed-By: Gus Caplan Reviewed-By: Colin Ihrig Reviewed-By: Anna Henningsen Reviewed-By: Matteo Collina Reviewed-By: George Adams --- doc/api/async_hooks.md | 3 +-- lib/domain.js | 7 ------- src/async_wrap.cc | 16 ++-------------- .../test-async-hooks-promise-enable-disable.js | 3 --- test/parallel/test-async-hooks-promise.js | 4 +--- test/parallel/test-domain-promise.js | 6 +----- 6 files changed, 5 insertions(+), 34 deletions(-) diff --git a/doc/api/async_hooks.md b/doc/api/async_hooks.md index ff3e14b96c2..4fc070c974d 100644 --- a/doc/api/async_hooks.md +++ b/doc/api/async_hooks.md @@ -301,8 +301,7 @@ currently not considered public, but using the Embedder API, users can provide and document their own resource objects. For example, such a resource object could contain the SQL query being executed. -In the case of Promises, the `resource` object will have `promise` property -that refers to the `Promise` that is being initialized, and an +In the case of Promises, the `resource` object will have an `isChainedPromise` property, set to `true` if the promise has a parent promise, and `false` otherwise. For example, in the case of `b = a.then(handler)`, `a` is considered a parent `Promise` of `b`. Here, `b` is considered a chained promise. diff --git a/lib/domain.js b/lib/domain.js index e8ae3ff1003..0caeb624b41 100644 --- a/lib/domain.js +++ b/lib/domain.js @@ -55,13 +55,6 @@ const asyncHook = createHook({ // if this operation is created while in a domain, let's mark it pairing.set(asyncId, process.domain); resource.domain = process.domain; - if (resource.promise !== undefined && - resource.promise instanceof Promise) { - // resource.promise instanceof Promise make sure that the - // promise comes from the same context - // see https://github.com/nodejs/node/issues/15673 - resource.promise.domain = process.domain; - } } }, before(asyncId) { diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 29bd64f2c5e..17636ceb31c 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -185,16 +185,13 @@ class PromiseWrap : public AsyncWrap { SET_MEMORY_INFO_NAME(PromiseWrap) SET_SELF_SIZE(PromiseWrap) - static constexpr int kPromiseField = 1; - static constexpr int kIsChainedPromiseField = 2; - static constexpr int kInternalFieldCount = 3; + static constexpr int kIsChainedPromiseField = 1; + static constexpr int kInternalFieldCount = 2; static PromiseWrap* New(Environment* env, Local promise, PromiseWrap* parent_wrap, bool silent); - static void GetPromise(Local property, - const PropertyCallbackInfo& info); static void getIsChainedPromise(Local property, const PropertyCallbackInfo& info); }; @@ -205,7 +202,6 @@ PromiseWrap* PromiseWrap::New(Environment* env, bool silent) { Local object = env->promise_wrap_template() ->NewInstance(env->context()).ToLocalChecked(); - object->SetInternalField(PromiseWrap::kPromiseField, promise); object->SetInternalField(PromiseWrap::kIsChainedPromiseField, parent_wrap != nullptr ? v8::True(env->isolate()) : @@ -215,11 +211,6 @@ PromiseWrap* PromiseWrap::New(Environment* env, return new PromiseWrap(env, object, silent); } -void PromiseWrap::GetPromise(Local property, - const PropertyCallbackInfo& info) { - info.GetReturnValue().Set(info.Holder()->GetInternalField(kPromiseField)); -} - void PromiseWrap::getIsChainedPromise(Local property, const PropertyCallbackInfo& info) { info.GetReturnValue().Set( @@ -315,9 +306,6 @@ static void SetupHooks(const FunctionCallbackInfo& args) { Local promise_wrap_template = ctor->InstanceTemplate(); promise_wrap_template->SetInternalFieldCount( PromiseWrap::kInternalFieldCount); - promise_wrap_template->SetAccessor( - FIXED_ONE_BYTE_STRING(env->isolate(), "promise"), - PromiseWrap::GetPromise); promise_wrap_template->SetAccessor( FIXED_ONE_BYTE_STRING(env->isolate(), "isChainedPromise"), PromiseWrap::getIsChainedPromise); diff --git a/test/parallel/test-async-hooks-promise-enable-disable.js b/test/parallel/test-async-hooks-promise-enable-disable.js index 150ccc88b0d..47a259166ee 100644 --- a/test/parallel/test-async-hooks-promise-enable-disable.js +++ b/test/parallel/test-async-hooks-promise-enable-disable.js @@ -4,7 +4,6 @@ const common = require('../common'); const assert = require('assert'); const async_hooks = require('async_hooks'); const EXPECTED_INITS = 2; -let p_resource = null; let p_er = null; let p_inits = 0; @@ -23,7 +22,6 @@ const mustCallInit = common.mustCall(function init(id, type, tid, resource) { if (type !== 'PROMISE') return; p_inits++; - p_resource = resource.promise; }, EXPECTED_INITS); const hook = async_hooks.createHook({ @@ -36,7 +34,6 @@ new Promise(common.mustCall((res) => { })).then(common.mustCall((val) => { hook.enable().enable(); const p = new Promise((res) => res(val)); - assert.strictEqual(p, p_resource); hook.disable(); return p; })).then(common.mustCall((val2) => { diff --git a/test/parallel/test-async-hooks-promise.js b/test/parallel/test-async-hooks-promise.js index be1509c7fc2..637d287b506 100644 --- a/test/parallel/test-async-hooks-promise.js +++ b/test/parallel/test-async-hooks-promise.js @@ -21,11 +21,9 @@ async_hooks.createHook({ }).enable(); const a = Promise.resolve(42); -const b = a.then(common.mustCall()); +a.then(common.mustCall()); assert.strictEqual(initCalls[0].triggerId, 1); assert.strictEqual(initCalls[0].resource.isChainedPromise, false); -assert.strictEqual(initCalls[0].resource.promise, a); assert.strictEqual(initCalls[1].triggerId, initCalls[0].id); assert.strictEqual(initCalls[1].resource.isChainedPromise, true); -assert.strictEqual(initCalls[1].resource.promise, b); diff --git a/test/parallel/test-domain-promise.js b/test/parallel/test-domain-promise.js index 1c6e6295608..70409252235 100644 --- a/test/parallel/test-domain-promise.js +++ b/test/parallel/test-domain-promise.js @@ -50,7 +50,6 @@ const vm = require('vm'); d2.run(common.mustCall(() => { p.then(common.mustCall((v) => { assert.strictEqual(process.domain, d2); - assert.strictEqual(p.domain, d1); })); })); } @@ -64,9 +63,8 @@ const vm = require('vm'); })); d2.run(common.mustCall(() => { - p.then(p.domain.bind(common.mustCall((v) => { + p.then(d1.bind(common.mustCall((v) => { assert.strictEqual(process.domain, d1); - assert.strictEqual(p.domain, d1); }))); })); } @@ -83,7 +81,6 @@ const vm = require('vm'); d2.run(common.mustCall(() => { p.then(common.mustCall((v) => { assert.strictEqual(process.domain, d2); - assert.strictEqual(p.domain, d1); })); })); })); @@ -100,7 +97,6 @@ const vm = require('vm'); d2.run(common.mustCall(() => { p.catch(common.mustCall((v) => { assert.strictEqual(process.domain, d2); - assert.strictEqual(p.domain, d1); })); })); }