-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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: use resource objects for Promises #13452
Changes from all commits
a187daa
0875682
464d2af
2652910
4acaed3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,7 @@ using v8::Context; | |
using v8::Float64Array; | ||
using v8::Function; | ||
using v8::FunctionCallbackInfo; | ||
using v8::FunctionTemplate; | ||
using v8::HandleScope; | ||
using v8::HeapProfiler; | ||
using v8::Integer; | ||
|
@@ -44,8 +45,10 @@ using v8::Local; | |
using v8::MaybeLocal; | ||
using v8::Number; | ||
using v8::Object; | ||
using v8::ObjectTemplate; | ||
using v8::Promise; | ||
using v8::PromiseHookType; | ||
using v8::PropertyCallbackInfo; | ||
using v8::RetainedObjectInfo; | ||
using v8::String; | ||
using v8::Symbol; | ||
|
@@ -282,37 +285,86 @@ bool AsyncWrap::EmitAfter(Environment* env, double async_id) { | |
class PromiseWrap : public AsyncWrap { | ||
public: | ||
PromiseWrap(Environment* env, Local<Object> object, bool silent) | ||
: AsyncWrap(env, object, PROVIDER_PROMISE, silent) {} | ||
: AsyncWrap(env, object, PROVIDER_PROMISE, silent) { | ||
MakeWeak(this); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @addaleax , sorry for very basic question, why we add MakeWeak(this); in this PR, since now we only keep the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This question is not basic – it’s something I didn’t quite figure out until a couple days ago myself. And I’m not sure whether you’ve seen it, but the current The thing is, And it needs to then make handle weak, because otherwise all V8 knows is that there is a persistent handle to the object; it doesn’t know how we use it, so it can’t garbage collect it. By making it weak, we tell V8 to destroy it once there are no other (non-weak) handles left, and to run our registered destroy callback (which will I agree, it’s all a bit tricky to see through. I’m mostly moving it here because that helps with avoiding duplication, and because that’s how we use it elsewhere in the code as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @addaleax , thank you for the explanation. I just read And I am not quite understand is why we don't need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It was there, it was just called after the constructor instead of being a part of it: https://github.com/nodejs/node/pull/13452/files#diff-5e552c79e1538215f1621d1774852e71L315 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @addaleax , oh, yes, it was there, thanks. Currently I have no further questions! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
And... I'm to blame for this one. In fe2df3b I carefully went through to make sure Basically I would be happy if we removed the call to
Be careful. AsyncWrap doesn't use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
thank you for pointing out this one. |
||
} | ||
size_t self_size() const override { return sizeof(*this); } | ||
|
||
static constexpr int kPromiseField = 1; | ||
static constexpr int kParentIdField = 2; | ||
static constexpr int kInternalFieldCount = 3; | ||
|
||
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 GetParentId(Local<String> property, | ||
const PropertyCallbackInfo<Value>& info); | ||
}; | ||
|
||
PromiseWrap* PromiseWrap::New(Environment* env, | ||
Local<Promise> promise, | ||
PromiseWrap* parent_wrap, | ||
bool silent) { | ||
Local<Object> object = env->promise_wrap_template() | ||
->NewInstance(env->context()).ToLocalChecked(); | ||
object->SetInternalField(PromiseWrap::kPromiseField, promise); | ||
if (parent_wrap != nullptr) { | ||
object->SetInternalField(PromiseWrap::kParentIdField, | ||
Number::New(env->isolate(), | ||
parent_wrap->get_id())); | ||
} | ||
CHECK_EQ(promise->GetAlignedPointerFromInternalField(0), nullptr); | ||
promise->SetInternalField(0, object); | ||
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::GetParentId(Local<String> property, | ||
const PropertyCallbackInfo<Value>& info) { | ||
info.GetReturnValue().Set(info.Holder()->GetInternalField(kParentIdField)); | ||
} | ||
|
||
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); | ||
Local<Value> resource_object_value = promise->GetInternalField(0); | ||
PromiseWrap* wrap = nullptr; | ||
if (resource_object_value->IsObject()) { | ||
Local<Object> resource_object = resource_object_value.As<Object>(); | ||
wrap = Unwrap<PromiseWrap>(resource_object); | ||
} | ||
if (type == PromiseHookType::kInit || wrap == nullptr) { | ||
bool silent = type != PromiseHookType::kInit; | ||
PromiseWrap* parent_wrap = nullptr; | ||
|
||
// 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<Promise> parent_promise = parent.As<Promise>(); | ||
auto parent_wrap = Unwrap<PromiseWrap>(parent_promise); | ||
Local<Value> parent_resource = parent_promise->GetInternalField(0); | ||
if (parent_resource->IsObject()) { | ||
parent_wrap = Unwrap<PromiseWrap>(parent_resource.As<Object>()); | ||
} | ||
|
||
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); | ||
parent_wrap = PromiseWrap::New(env, parent_promise, nullptr, true); | ||
} | ||
// 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); | ||
|
||
wrap = PromiseWrap::New(env, promise, parent_wrap, silent); | ||
} else if (type == PromiseHookType::kResolve) { | ||
// TODO(matthewloring): need to expose this through the async hooks api. | ||
} | ||
|
@@ -351,6 +403,22 @@ static void SetupHooks(const FunctionCallbackInfo<Value>& args) { | |
SET_HOOK_FN(destroy); | ||
env->AddPromiseHook(PromiseHook, nullptr); | ||
#undef SET_HOOK_FN | ||
|
||
{ | ||
Local<FunctionTemplate> ctor = | ||
FunctionTemplate::New(env->isolate()); | ||
ctor->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "PromiseWrap")); | ||
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(), "parentId"), | ||
PromiseWrap::GetParentId); | ||
env->set_promise_wrap_template(promise_wrap_template); | ||
} | ||
} | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
'use strict'; | ||
const common = require('../common'); | ||
const assert = require('assert'); | ||
const async_hooks = require('async_hooks'); | ||
|
||
const initCalls = []; | ||
|
||
async_hooks.createHook({ | ||
init: common.mustCall((id, type, triggerId, resource) => { | ||
assert.strictEqual(type, 'PROMISE'); | ||
initCalls.push({id, triggerId, resource}); | ||
}, 2) | ||
}).enable(); | ||
|
||
const a = Promise.resolve(42); | ||
const b = a.then(common.mustCall()); | ||
|
||
assert.strictEqual(initCalls[0].triggerId, 1); | ||
assert.strictEqual(initCalls[0].resource.parentId, undefined); | ||
assert.strictEqual(initCalls[0].resource.promise, a); | ||
assert.strictEqual(initCalls[1].triggerId, initCalls[0].id); | ||
assert.strictEqual(initCalls[1].resource.parentId, initCalls[0].id); | ||
assert.strictEqual(initCalls[1].resource.promise, b); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add
PROMISE
to the provider list inasync_wrap
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve kept it out of it because it doesn’t quite qualify as “provided by the built-in Node.js modules”, but if you think it’s better, I can just drop this sentence and reword the existing text a bit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No just leave it out for now. @trevnorris suggested that maybe we should add all types to the
Providers
list. See #13287 (comment)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. Seems we don't even include
Timer
orTickObject
in that list. Or even as a side comment.Anyway, only argument I'd have for adding it to the above list is because it shows up in
process.binding('async_wrap').Providers
. But that's not a strong argument. I'll leave this up to you.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should delete the list. The user shouldn't really on a defined list of types because the Embedder API allows for extra types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndreasMadsen The reason I decided to pass the "type" (history note: using
Provider
was not the name I wanted to use, but switched long ago b/c of pressure to change on the PR) was for to allow filtering resources. I'd like to have a list of all "core types" (which would includeTickObject
andTimer
) and I'd expect module authors to document their own "types".Though because this relies a bit on internals could we put a note like "subject to change in future major (or minor?) releases without deprecation"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a note saying resource objects are not to be trusted:
I guess we can be more specific.