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_wrap,src: promise hook integration #13000

Closed
wants to merge 16 commits into from
206 changes: 141 additions & 65 deletions src/async-wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: align arguments

Copy link
Author

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.

}
}
}
}

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FatalException() always exits the application in this case because ClearFatalException() removed all unhandledException callbacks. So I'll assume this return is so the compiler doesn't emit a warning.

I decided this would be the safest course of action when first implementing AsyncWrap because it was difficult to properly clean up if an async hook threw. It was difficult enough to properly cleanup if a user's callback threw (see process._fatalException() in lib/internal/bootstrap_node.js). If we can properly show it's safe to recover from an async hook throwing then I'm all for changing this behavior.

Copy link
Author

Choose a reason for hiding this comment

The 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());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I missed that, but you can drop this (and the if (env->in_domain()) { block that installs it, too) now

Copy link
Author

Choose a reason for hiding this comment

The 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";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two seem to be unused now?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were removed in 0afbcfc. Thanks.

if (type == PromiseHookType::kInit) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an analogue of env->in_domain() for async_hooks? It would be good to bail out here if we know async hooks is not enabled.

Copy link
Member

@addaleax addaleax May 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matthewloring What Trevor has been doing elsewhere in the code is testing env()->async_hooks()->fields()[AsyncHooks::kInit] == 0, I think that should work.

I still like the AddPromiseHook approach, though – it helps with keeping domains and async hooks separate, and more importantly I think we’d only want to set a promise hook if we’re actually consuming the information. Otherwise it seems like something that might come with unnecessary performance impact on Promise usage. Seems like that has already been addressed, sorry.

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

diff
diff --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.

Copy link
Member

@AndreasMadsen AndreasMadsen May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping to avoid attaching the new async hook related properties to promises in the case where async hooks were disabled.

How much will inverting the dependencies help, as suggested in #13000 (comment)? Personally, I see no reason to add properties to the promise object.

Copy link
Member

@AndreasMadsen AndreasMadsen May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax yes, but can a WeakMap (wrap = map[promise]) not be used to get around setting properties?

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, that is sad :(

Copy link
Contributor

Choose a reason for hiding this comment

The 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 kInit == !(kBefore == !(kAfter == !(kDestroy == 0))) then the hooks list is empty. I had consolidated this into kTotalHooks that was the sum of all active hooks, so the only necessary check was kTotalHooks == 0. I can re-add this if necessary easily. Already have the code from the previous implementation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting that @addaleax's suggestion above has been pulled to here to separate the discussion.

// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be faster to create an Eternal<ObjectTemplate> (e.g. adding this to ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES() in src/env.h) that can be reused for all NewInstance().

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: async_id is confusion as that indicates this store the double id. But in reality, it stores the PromiseWrap object. Maybe calling it env->promise_wrap() would be better.

Copy link
Author

Choose a reason for hiding this comment

The 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(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is promise_async_tag() used for, preventing garbage collection? If so I think a comment should be added.

Copy link
Author

Choose a reason for hiding this comment

The 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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want the user to be able to alter these? In src/async-wrap.cc I used

  v8::PropertyAttribute ReadOnlyDontDelete =
      static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete);

Though I probably should have added DontEnum to that list.

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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();
Copy link
Member

Choose a reason for hiding this comment

The 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 DomainPromiseHook? That way we don’t have to pay for domains or async hooks if we don’t use them

Copy link
Author

Choose a reason for hiding this comment

The 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 DomainPromiseHook (it is also highly unlikely that a project would use both domains and async hooks so keeping them logically separated would be good).

Copy link
Member

Choose a reason for hiding this comment

The 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kResolve only happens once per promise, when the promise is resolved, right? In that case, I think we can just put the current async id that caused the resolve on the PromiseWrap object. That should provide most of the additional information.

Yes, there is also the actual event but that information can be inferred when kBefore is emitted. In case .then() is never called, I don't this it is that interesting when the promise is resolved since it is never used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kResolve only happens once per promise

yep :)

when the promise is resolved, right?

You’d think so, wouldn’t you? 😄 This isn’t called when the promise is resolved, it’s at the beginning of when the resolve function (as in new Promise((resolve) => …)) or some equivalent of it is called. That might not be when the Promise is resolved, though (because you can call resolve() with other pending promises).

I’m not actually sure kResolve is a very interesting thing for async hooks?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if a promise's then correlates with kResolve, but a promise's then can be called multiple times.

Copy link
Member

@AndreasMadsen AndreasMadsen May 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn’t called when the promise is resolved, it’s at the beginning of when the resolve function (as in new Promise((resolve) => …)) or some equivalent of it is called.

Haha, that was actually what I meant, when resolve() is called. I always mix up my promise terminology.

Copy link
Member

@AndreasMadsen AndreasMadsen May 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not actually sure kResolve is a very interesting thing for async hooks?

This was perhaps the most discussed thing in the async_hooks EPS. This comment descibes the two viewpoints nodejs/node-eps#18 (comment), though I don't really agree with the "who needs what" part (stack-trace vs CLS) after talking with APM providers (@watson).

I think adding the async id that called resolve() to the PromiseWrap is the best option for satisfying both parties.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 new Promise() or .then() runs then the stack would be:

    at printLongErrorStack
    at catch0
    at si0
    at runner

Though it would be helpful to produce the following long stack:

    at printLongErrorStack
    at catch0
    at Fn
    at then0
    at si0
    at p_fn0
    at runner

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 AsyncEvent hooks would produce the first call stack; but it should also be possible to produce the second call stack (@matthewloring was this conversation with you?).

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 resolve or reject calls. At which point a stack can be produced then (somehow?) attached to the Promise handle that's responsible for the current execution scope.

Copy link
Author

Choose a reason for hiding this comment

The 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 kBefore. This approach does not allow async_hook users to "react" to a promise resolution (state cannot be observed/updated at resolution time). Would it be too much to add another async_hook type for the resolution of async work (promise or otherwise)? With the addition of the C++ embedder api I can imagine that other types of async work could notify when async operations completes even if the JS callback isn't invoked immediately.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach does not allow async_hook users to "react" to a promise resolution (state cannot be observed/updated at resolution time). Would it be too much to add another async_hook type for the resolution of async work (promise or otherwise)? With the addition of the C++ embedder api I can imagine that other types of async work could notify when async operations completes even if the JS callback isn't invoked immediately.

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.

Copy link
Author

Choose a reason for hiding this comment

The 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());
Copy link
Member

Choose a reason for hiding this comment

The 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 external_wrap.As<v8::External>()->Value() is fine)

Copy link
Contributor

Choose a reason for hiding this comment

The 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 class Local:

  template <class S>
  V8_INLINE Local<S> As() const {
    return Local<S>::Cast(*this);
  }

@matthewloring To satiate my paranoia, mind adding a CHECK_NE(wrap, nullptr) just below?

Copy link
Member

Choose a reason for hiding this comment

The 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 .As<>(), that’s all.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax Added the check here 34ee31f

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);

Expand All @@ -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
}

Expand Down Expand Up @@ -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(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grabbing the Local<Context> using Environment isn't free. Maybe pass it in?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grabbing the Local<Context> using Environment isn't free

Are you sure? We use the StrongPersistentToLocal utility to get a Local out of the Persistent properties, which is somewhat icky but should compile down to a no-op.

Copy link
Contributor

Choose a reason for hiding this comment

The 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;
}
Expand Down
1 change: 1 addition & 0 deletions src/async-wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ namespace node {
V(PIPECONNECTWRAP) \
V(PIPEWRAP) \
V(PROCESSWRAP) \
V(PROMISE) \
V(QUERYWRAP) \
V(RANDOMBYTESREQUEST) \
V(SHUTDOWNWRAP) \
Expand Down
2 changes: 2 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,8 @@ namespace node {
V(preference_string, "preference") \
V(priority_string, "priority") \
V(produce_cached_data_string, "produceCachedData") \
V(promise_async_id, "_promise_async_wrap") \
V(promise_async_tag, "_promise_async_wrap_tag") \
V(raw_string, "raw") \
V(read_host_object_string, "_readHostObject") \
V(readable_string, "readable") \
Expand Down
55 changes: 0 additions & 55 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

The 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));
}

Expand Down
51 changes: 51 additions & 0 deletions test/async-hooks/test-promise.js
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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't check the destroy hook. I'm okay with skipping it because it involves the gc and that can be a bit fiddly. But a comment should be added explaining why it isn't checked.

Copy link
Author

Choose a reason for hiding this comment

The 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');
}
Loading