-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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_wrap,src: promise hook integration #13000
Changes from 4 commits
ef35de1
563c1b0
4aaadc0
70f9675
0afbcfc
833ad39
5717d75
34ee31f
e0c194b
e1c11e7
d47e97a
012171a
4e38bda
13313bb
9b9c310
81ff7f6
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 |
---|---|---|
|
@@ -44,6 +44,8 @@ using v8::Local; | |
using v8::MaybeLocal; | ||
using v8::Number; | ||
using v8::Object; | ||
using v8::Promise; | ||
using v8::PromiseHookType; | ||
using v8::RetainedObjectInfo; | ||
using v8::Symbol; | ||
using v8::TryCatch; | ||
|
@@ -177,6 +179,136 @@ static void PushBackDestroyId(Environment* env, double id) { | |
} | ||
|
||
|
||
static bool PreCallbackExecution(AsyncWrap* wrap) { | ||
AsyncHooks* async_hooks = wrap->env()->async_hooks(); | ||
if (wrap->env()->using_domains()) { | ||
Local<Value> domain_v = wrap->object()->Get(wrap->env()->domain_string()); | ||
if (domain_v->IsObject()) { | ||
Local<Object> domain = domain_v.As<Object>(); | ||
if (domain->Get(wrap->env()->disposed_string())->IsTrue()) | ||
return false; | ||
Local<Value> enter_v = domain->Get(wrap->env()->enter_string()); | ||
if (enter_v->IsFunction()) { | ||
if (enter_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) { | ||
FatalError("node::AsyncWrap::MakeCallback", | ||
"domain enter callback threw, please report this"); | ||
} | ||
} | ||
} | ||
} | ||
|
||
if (async_hooks->fields()[AsyncHooks::kBefore] > 0) { | ||
Local<Value> uid = Number::New(wrap->env()->isolate(), wrap->get_id()); | ||
Local<Function> fn = wrap->env()->async_hooks_before_function(); | ||
TryCatch try_catch(wrap->env()->isolate()); | ||
MaybeLocal<Value> ar = fn->Call( | ||
wrap->env()->context(), Undefined(wrap->env()->isolate()), 1, &uid); | ||
if (ar.IsEmpty()) { | ||
ClearFatalExceptionHandlers(wrap->env()); | ||
FatalException(wrap->env()->isolate(), try_catch); | ||
return false; | ||
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.
I decided this would be the safest course of action when first implementing 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. That makes sense, maybe we can look into that more in a follow on pr. |
||
} | ||
} | ||
return true; | ||
} | ||
|
||
|
||
static bool PostCallbackExecution(AsyncWrap* wrap) { | ||
AsyncHooks* async_hooks = wrap->env()->async_hooks(); | ||
|
||
// If the callback failed then the after() hooks will be called at the end | ||
// of _fatalException(). | ||
if (async_hooks->fields()[AsyncHooks::kAfter] > 0) { | ||
Local<Value> uid = Number::New(wrap->env()->isolate(), wrap->get_id()); | ||
Local<Function> fn = wrap->env()->async_hooks_after_function(); | ||
TryCatch try_catch(wrap->env()->isolate()); | ||
MaybeLocal<Value> ar = fn->Call( | ||
wrap->env()->context(), Undefined(wrap->env()->isolate()), 1, &uid); | ||
if (ar.IsEmpty()) { | ||
ClearFatalExceptionHandlers(wrap->env()); | ||
FatalException(wrap->env()->isolate(), try_catch); | ||
return false; | ||
} | ||
} | ||
|
||
if (wrap->env()->using_domains()) { | ||
Local<Value> domain_v = wrap->object()->Get(wrap->env()->domain_string()); | ||
if (domain_v->IsObject()) { | ||
Local<Object> domain = domain_v.As<Object>(); | ||
if (domain->Get(wrap->env()->disposed_string())->IsTrue()) | ||
return false; | ||
Local<Value> exit_v = domain->Get(wrap->env()->exit_string()); | ||
if (exit_v->IsFunction()) { | ||
if (exit_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) { | ||
FatalError("node::AsyncWrap::MakeCallback", | ||
"domain exit callback threw, please report this"); | ||
} | ||
} | ||
} | ||
} | ||
|
||
return true; | ||
} | ||
|
||
class PromiseWrap : public AsyncWrap { | ||
public: | ||
PromiseWrap(Environment* env, Local<Object> object) | ||
: AsyncWrap(env, object, PROVIDER_PROMISE) {} | ||
size_t self_size() const override { return sizeof(*this); } | ||
}; | ||
|
||
|
||
static void GetPromiseDomain(Local<v8::Name> name, | ||
const v8::PropertyCallbackInfo<v8::Value>& info) { | ||
Local<Context> context = info.GetIsolate()->GetCurrentContext(); | ||
Environment* env = Environment::GetCurrent(context); | ||
info.GetReturnValue().Set( | ||
info.Data().As<Object>()->Get(context, | ||
env->domain_string()).ToLocalChecked()); | ||
} | ||
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. I guess I missed that, but you can drop this (and 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. Removed. |
||
|
||
|
||
static void PromiseHook(PromiseHookType type, Local<Promise> promise, | ||
Local<Value> parent, void* arg) { | ||
Local<Context> context = promise->CreationContext(); | ||
Environment* env = Environment::GetCurrent(context); | ||
const char async_id_key[] = "__async_wrap"; | ||
const char tag_id_key[] = "__async_wrap_tag"; | ||
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. These two seem to be unused now? 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. These were removed in 0afbcfc. Thanks. |
||
if (type == PromiseHookType::kInit) { | ||
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. Is there an analogue of 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. @matthewloring What Trevor has been doing elsewhere in the code is testing
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 As far as I can tell, the line above is only used to avoid invoking init hooks if none have been registered. I was hoping to avoid attaching the new async hook related properties to promises in the case where async hooks were disabled. Adding these properties causes ~30x increase in the time it takes to construct/resolve promises (adding the property causes all of V8's map checks to fail and prevent promises from ever running on fast paths). I am hoping to fix this by adding an internal field to promises (https://codereview.chromium.org/2889863002/) but it would be good to avoid modifying promises if async hooks are not in use. This slow down may also come in to play when domains are active as that also seems to modify promise shape but I haven't run any benchmarks against domains yet. 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. @matthewloring I think this might be enough for what you want? I didn’t try it with this PR but it passes testing on its own. diffdiff --git a/lib/async_hooks.js b/lib/async_hooks.js
index 867b5eb52da1..49f8fa5becf2 100644
--- a/lib/async_hooks.js
+++ b/lib/async_hooks.js
@@ -49,12 +49,7 @@ const before_symbol = Symbol('before');
const after_symbol = Symbol('after');
const destroy_symbol = Symbol('destroy');
-// Setup the callbacks that node::AsyncWrap will call when there are hooks to
-// process. They use the same functions as the JS embedder API.
-async_wrap.setupHooks({ init,
- before: emitBeforeN,
- after: emitAfterN,
- destroy: emitDestroyN });
+let setupHooksCalled = false;
// Used to fatally abort the process if a callback throws.
function fatalError(e) {
@@ -103,6 +98,16 @@ class AsyncHook {
if (hooks_array.includes(this))
return;
+ if (!setupHooksCalled) {
+ setupHooksCalled = true;
+ // Setup the callbacks that node::AsyncWrap will call when there are
+ // hooks to process. They use the same functions as the JS embedder API.
+ async_wrap.setupHooks({ init,
+ before: emitBeforeN,
+ after: emitAfterN,
+ destroy: emitDestroyN });
+ }
+
// createHook() has already enforced that the callbacks are all functions,
// so here simply increment the count of whether each callbacks exists or
// not. 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.
How much will inverting the dependencies help, as suggested in #13000 (comment)? Personally, I see no reason to add properties to the promise object. 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 yes, but can a WeakMap ( 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. @AndreasMadsen As I’ve recently learned, no :( http://iamstef.net/n/shapeshifting.html 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. I see, that is sad :( 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. @matthewloring I actually had a call like this previously, but removed it b/c it wasn't needed anywhere in the initial PR. Basically if 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. |
||
// Unfortunately, promises don't have internal fields. Need a surrogate that | ||
// async wrap can wrap. | ||
Local<v8::ObjectTemplate> tem = v8::ObjectTemplate::New(env->isolate()); | ||
tem->SetInternalFieldCount(1); | ||
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. Would it be faster to create an 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. The need for a custom object template will be going away as soon as we move to an internal property on promises (blocked on #13175). I can modify this but I intend to get the promise internal property refactoring in before 8.0. 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. I've added this for now. |
||
Local<Object> obj = tem->NewInstance(context).ToLocalChecked(); | ||
PromiseWrap* wrap = new PromiseWrap(env, obj); | ||
promise->DefineOwnProperty(context, | ||
env->promise_async_id(), | ||
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. nit: 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. Done. |
||
v8::External::New(env->isolate(), wrap), | ||
v8::PropertyAttribute::DontEnum).FromJust(); | ||
promise->DefineOwnProperty(context, | ||
env->promise_async_tag(), | ||
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. What is 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. The only reference to it is held by the promise. We use its lifespan to approximate when the promise was garbage collected. I've added a comment describing this. |
||
obj, v8::PropertyAttribute::DontEnum).FromJust(); | ||
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. Do we want the user to be able to alter these? In v8::PropertyAttribute ReadOnlyDontDelete =
static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete); Though I probably should have added 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 property will be going away as soon as we move to an internal property on promises (blocked on #13175). I can modify this but I intend to get the promise internal property refactoring in before 8.0. 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. I've added this for now. |
||
if (env->in_domain()) { | ||
obj->Set(context, env->domain_string(), | ||
env->domain_array()->Get(context, 0).ToLocalChecked()).FromJust(); | ||
promise->SetAccessor(context, env->domain_string(), | ||
GetPromiseDomain, NULL, obj).FromJust(); | ||
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. Is there anything speaking against removing the domain handling inside this promise hook and re-adding 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. My goal with the refactor was to try and have the domain entry and exit logic + promise hook before/after logic live in one place to avoid having complex parallel code in multiple places. I think this can be achieved with re-adding 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. Yea, I get what you were aiming at, and I do like the idea a lot. But I’d just rather prefer not to have domain usage depend on using async hooks, that’s all; I don’t care that much how that’s done. (I.e. don’t assume I’d think the solution I pushed here is optimal – I’m sure there’s room for improvement) |
||
} | ||
} else if (type == PromiseHookType::kResolve) { | ||
// TODO(matthewloring): need to expose this through the async hooks api. | ||
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.
Yes, there is also the actual event but that information can be inferred when 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.
yep :)
You’d think so, wouldn’t you? 😄 This isn’t called when the promise is resolved, it’s at the beginning of when the I’m not actually sure 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. I'm not sure if a promise's 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.
Haha, that was actually what I meant, when 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 was perhaps the most discussed thing in the I think adding the async id that called 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 is an example scenario that almost captures all the possibilities that Promises and async hooks can interact: (function runner() {
const p = new Promise(function p_fn0(res) {
setImmediate(function si0() {
res('foo')
});
}).then(function then0(val) {
return Fn(val);
});
setImmediate(function si0() {
p.catch(function catch0(err) {
printLongErrorStack(err); // not defined
});
});
function Fn(val) {
if (val === 'foo') throw new Error('bam');
}
})(); If each Promise executor, onFulfilled or onRejected only collect the stack when
Though it would be helpful to produce the following long stack:
I've had some lengthy discussions about this and, though I don't remember with whom, I recall agreeing that the logical place to execute the So for this PR the first stack above is expected, but we should also address how to produce the second. IIRC the API in in place to allow watching for 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. Yup, this conversation was with me and I think it captures the concern with attaching the async id to the promise resource at resolution time so that it can be read in 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. @matthewloring Adding another type that fires at a different time wouldn't be a problem. 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.
While it is not a technical issue to add another type (I assume you mean event type and not resource type), I think it would be better to do in a future PR where we have a better chance to evaluate the needs for such event type for other cases than promises. So far the only concern mentioned in the EPS has been regarding context across async boundaries, which "attaching the async id to the promise resource" does solve, although not in a particularly nice way. 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. Addressing this in a follow on seems fine to me. |
||
} | ||
Local<v8::Value> external_wrap = | ||
promise->Get(context, env->promise_async_id()).ToLocalChecked(); | ||
PromiseWrap* wrap = | ||
static_cast<PromiseWrap*>(v8::External::Cast(*external_wrap)->Value()); | ||
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. Getting a déjà-vu, maybe I already commented before, but we generally prefer not to dereference handles explicitly (i.e. using 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 Don't they basically result in the same operation? e.g. from template <class S>
V8_INLINE Local<S> As() const {
return Local<S>::Cast(*this);
} @matthewloring To satiate my paranoia, mind adding a 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. @trevnorris Yes, sorry for not being clear; they result in the same compiled code. This was just a style thing, the overwhelming majority of our codebase uses 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. |
||
if (type == PromiseHookType::kBefore) { | ||
PreCallbackExecution(wrap); | ||
} else if (type == PromiseHookType::kAfter) { | ||
PostCallbackExecution(wrap); | ||
} | ||
} | ||
|
||
|
||
static void SetupHooks(const FunctionCallbackInfo<Value>& args) { | ||
Environment* env = Environment::GetCurrent(args); | ||
|
||
|
@@ -201,6 +333,7 @@ static void SetupHooks(const FunctionCallbackInfo<Value>& args) { | |
SET_HOOK_FN(before); | ||
SET_HOOK_FN(after); | ||
SET_HOOK_FN(destroy); | ||
env->AddPromiseHook(PromiseHook, nullptr); | ||
#undef SET_HOOK_FN | ||
} | ||
|
||
|
@@ -416,87 +549,30 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb, | |
Local<Value>* argv) { | ||
CHECK(env()->context() == env()->isolate()->GetCurrentContext()); | ||
|
||
AsyncHooks* async_hooks = env()->async_hooks(); | ||
Local<Object> context = object(); | ||
Local<Object> domain; | ||
Local<Value> uid; | ||
bool has_domain = false; | ||
|
||
Environment::AsyncCallbackScope callback_scope(env()); | ||
|
||
if (env()->using_domains()) { | ||
Local<Value> domain_v = context->Get(env()->domain_string()); | ||
has_domain = domain_v->IsObject(); | ||
if (has_domain) { | ||
domain = domain_v.As<Object>(); | ||
if (domain->Get(env()->disposed_string())->IsTrue()) | ||
return Local<Value>(); | ||
} | ||
} | ||
|
||
if (has_domain) { | ||
Local<Value> enter_v = domain->Get(env()->enter_string()); | ||
if (enter_v->IsFunction()) { | ||
if (enter_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) { | ||
FatalError("node::AsyncWrap::MakeCallback", | ||
"domain enter callback threw, please report this"); | ||
} | ||
} | ||
} | ||
|
||
// Want currentId() to return the correct value from the callbacks. | ||
AsyncHooks::ExecScope exec_scope(env(), get_id(), get_trigger_id()); | ||
Environment::AsyncHooks::ExecScope exec_scope(env(), | ||
get_id(), | ||
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. Grabbing 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.
Are you sure? We use 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. @addaleax Note sure what I was smoking. My comment makes no sense. Never mind. |
||
get_trigger_id()); | ||
|
||
if (async_hooks->fields()[AsyncHooks::kBefore] > 0) { | ||
uid = Number::New(env()->isolate(), get_id()); | ||
Local<Function> fn = env()->async_hooks_before_function(); | ||
TryCatch try_catch(env()->isolate()); | ||
MaybeLocal<Value> ar = fn->Call( | ||
env()->context(), Undefined(env()->isolate()), 1, &uid); | ||
if (ar.IsEmpty()) { | ||
ClearFatalExceptionHandlers(env()); | ||
FatalException(env()->isolate(), try_catch); | ||
return Local<Value>(); | ||
} | ||
if (!PreCallbackExecution(this)) { | ||
return Local<Value>(); | ||
} | ||
|
||
// Finally... Get to running the user's callback. | ||
MaybeLocal<Value> ret = cb->Call(env()->context(), context, argc, argv); | ||
MaybeLocal<Value> ret = cb->Call(env()->context(), object(), argc, argv); | ||
|
||
Local<Value> ret_v; | ||
if (!ret.ToLocal(&ret_v)) { | ||
return Local<Value>(); | ||
} | ||
|
||
// If the callback failed then the after() hooks will be called at the end | ||
// of _fatalException(). | ||
if (async_hooks->fields()[AsyncHooks::kAfter] > 0) { | ||
if (uid.IsEmpty()) | ||
uid = Number::New(env()->isolate(), get_id()); | ||
Local<Function> fn = env()->async_hooks_after_function(); | ||
TryCatch try_catch(env()->isolate()); | ||
MaybeLocal<Value> ar = fn->Call( | ||
env()->context(), Undefined(env()->isolate()), 1, &uid); | ||
if (ar.IsEmpty()) { | ||
ClearFatalExceptionHandlers(env()); | ||
FatalException(env()->isolate(), try_catch); | ||
return Local<Value>(); | ||
} | ||
if (!PostCallbackExecution(this)) { | ||
return Local<Value>(); | ||
} | ||
|
||
// The execution scope of the id and trigger_id only go this far. | ||
exec_scope.Dispose(); | ||
|
||
if (has_domain) { | ||
Local<Value> exit_v = domain->Get(env()->exit_string()); | ||
if (exit_v->IsFunction()) { | ||
if (exit_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) { | ||
FatalError("node::AsyncWrap::MakeCallback", | ||
"domain exit callback threw, please report this"); | ||
} | ||
} | ||
} | ||
|
||
if (callback_scope.in_makecallback()) { | ||
return ret_v; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -143,7 +143,6 @@ using v8::Number; | |
using v8::Object; | ||
using v8::ObjectTemplate; | ||
using v8::Promise; | ||
using v8::PromiseHookType; | ||
using v8::PromiseRejectMessage; | ||
using v8::PropertyCallbackInfo; | ||
using v8::ScriptOrigin; | ||
|
@@ -1117,58 +1116,6 @@ bool ShouldAbortOnUncaughtException(Isolate* isolate) { | |
} | ||
|
||
|
||
void DomainPromiseHook(PromiseHookType type, | ||
Local<Promise> promise, | ||
Local<Value> parent, | ||
void* arg) { | ||
Environment* env = static_cast<Environment*>(arg); | ||
Local<Context> context = env->context(); | ||
|
||
if (type == PromiseHookType::kResolve) return; | ||
if (type == PromiseHookType::kInit && env->in_domain()) { | ||
promise->Set(context, | ||
env->domain_string(), | ||
env->domain_array()->Get(context, | ||
0).ToLocalChecked()).FromJust(); | ||
return; | ||
} | ||
|
||
// Loosely based on node::MakeCallback(). | ||
Local<Value> domain_v = | ||
promise->Get(context, env->domain_string()).ToLocalChecked(); | ||
if (!domain_v->IsObject()) | ||
return; | ||
|
||
Local<Object> domain = domain_v.As<Object>(); | ||
if (domain->Get(context, env->disposed_string()) | ||
.ToLocalChecked()->IsTrue()) { | ||
return; | ||
} | ||
|
||
if (type == PromiseHookType::kBefore) { | ||
Local<Value> enter_v = | ||
domain->Get(context, env->enter_string()).ToLocalChecked(); | ||
if (enter_v->IsFunction()) { | ||
if (enter_v.As<Function>()->Call(context, domain, 0, nullptr).IsEmpty()) { | ||
FatalError("node::PromiseHook", | ||
"domain enter callback threw, please report this " | ||
"as a bug in Node.js"); | ||
} | ||
} | ||
} else { | ||
Local<Value> exit_v = | ||
domain->Get(context, env->exit_string()).ToLocalChecked(); | ||
if (exit_v->IsFunction()) { | ||
if (exit_v.As<Function>()->Call(context, domain, 0, nullptr).IsEmpty()) { | ||
FatalError("node::MakeCallback", | ||
"domain exit callback threw, please report this " | ||
"as a bug in Node.js"); | ||
} | ||
} | ||
} | ||
} | ||
|
||
|
||
void SetupDomainUse(const FunctionCallbackInfo<Value>& args) { | ||
Environment* env = Environment::GetCurrent(args); | ||
|
||
|
@@ -1208,8 +1155,6 @@ void SetupDomainUse(const FunctionCallbackInfo<Value>& args) { | |
Local<ArrayBuffer> array_buffer = | ||
ArrayBuffer::New(env->isolate(), fields, sizeof(*fields) * fields_count); | ||
|
||
env->AddPromiseHook(DomainPromiseHook, static_cast<void*>(env)); | ||
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. Is this being removed intentionally? |
||
|
||
args.GetReturnValue().Set(Uint32Array::New(array_buffer, 0, fields_count)); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
'use strict'; | ||
|
||
const common = require('../common'); | ||
const assert = require('assert'); | ||
const initHooks = require('./init-hooks'); | ||
const { checkInvocations } = require('./hook-checks'); | ||
|
||
const hooks = initHooks(); | ||
|
||
hooks.enable(); | ||
|
||
const p = (new Promise(common.mustCall(executor))); | ||
p.then(afterresolution); | ||
|
||
function executor(resolve, reject) { | ||
const as = hooks.activitiesOfTypes('PROMISE'); | ||
assert.strictEqual(as.length, 1, 'one activities'); | ||
const a = as[0]; | ||
checkInvocations(a, { init: 1 }, 'while in promise executor'); | ||
resolve(5); | ||
} | ||
|
||
function afterresolution(val) { | ||
assert.strictEqual(val, 5); | ||
const as = hooks.activitiesOfTypes('PROMISE'); | ||
assert.strictEqual(as.length, 2, 'two activities'); | ||
checkInvocations(as[0], { init: 1 }, 'after resolution parent promise'); | ||
checkInvocations(as[1], { init: 1, before: 1 }, | ||
'after resolution child promise'); | ||
} | ||
|
||
process.on('exit', onexit); | ||
function onexit() { | ||
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. We don't check 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. Yup, done! |
||
hooks.disable(); | ||
hooks.sanityCheck('PROMISE'); | ||
|
||
const as = hooks.activitiesOfTypes('PROMISE'); | ||
assert.strictEqual(as.length, 2, 'two activities'); | ||
|
||
const a0 = as[0]; | ||
assert.strictEqual(a0.type, 'PROMISE', 'promise request'); | ||
assert.strictEqual(typeof a0.uid, 'number', 'uid is a number'); | ||
assert.strictEqual(a0.triggerId, 1, 'parent uid 1'); | ||
checkInvocations(a0, { init: 1 }, 'when process exits'); | ||
|
||
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'); | ||
checkInvocations(a1, { init: 1, before: 1, after: 1 }, 'when process exits'); | ||
} |
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.
style nit: align arguments
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.
@addaleax Fixed this here 5717d75.