Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async_hooks: remove promise object from resource #23443

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions doc/api/async_hooks.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 0 additions & 7 deletions lib/domain.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
16 changes: 2 additions & 14 deletions src/async_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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> promise,
PromiseWrap* parent_wrap,
bool silent);
static void GetPromise(Local<String> property,
const PropertyCallbackInfo<Value>& info);
static void getIsChainedPromise(Local<String> property,
const PropertyCallbackInfo<Value>& info);
};
Expand All @@ -205,7 +202,6 @@ PromiseWrap* PromiseWrap::New(Environment* env,
bool silent) {
Local<Object> 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()) :
Expand All @@ -215,11 +211,6 @@ PromiseWrap* PromiseWrap::New(Environment* env,
return new PromiseWrap(env, object, silent);
}

void PromiseWrap::GetPromise(Local<String> property,
const PropertyCallbackInfo<Value>& info) {
info.GetReturnValue().Set(info.Holder()->GetInternalField(kPromiseField));
}

void PromiseWrap::getIsChainedPromise(Local<String> property,
const PropertyCallbackInfo<Value>& info) {
info.GetReturnValue().Set(
Expand Down Expand Up @@ -315,9 +306,6 @@ static void SetupHooks(const FunctionCallbackInfo<Value>& args) {
Local<ObjectTemplate> 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);
Expand Down
3 changes: 0 additions & 3 deletions test/parallel/test-async-hooks-promise-enable-disable.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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({
Expand All @@ -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) => {
Expand Down
4 changes: 1 addition & 3 deletions test/parallel/test-async-hooks-promise.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
6 changes: 1 addition & 5 deletions test/parallel/test-domain-promise.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}));
}));
}
Expand All @@ -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);
})));
}));
}
Expand All @@ -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);
}));
}));
}));
Expand All @@ -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);
}));
}));
}
Expand Down