Skip to content

async_wrap,src: promise hook integration #13000

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.

Sign up for GitHub

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

Closed
matthewloring wants to merge 16 commits into from
Closed

async_wrap,src: promise hook integration #13000

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
ef35de1
async_wrap,src: promise hook integration
Apr 19, 2017
563c1b0
Reintroduce promise hook sharing
May 15, 2017
4aaadc0
Cleanup
May 15, 2017
70f9675
Clean up comments
May 23, 2017
0afbcfc
remove unused variables
addaleax May 21, 2017
833ad39
use .As<>() for casting
addaleax May 21, 2017
5717d75
align arguments (trevnorris review)
addaleax May 21, 2017
34ee31f
add CHECK_NE() (trevnorris review)
addaleax May 21, 2017
e0c194b
re-untangle domain/async_hooks promise support
addaleax May 21, 2017
e1c11e7
async_hooks: only set up hooks if used
addaleax May 21, 2017
d47e97a
Clarify use of additional object to track promise lifespan.
May 23, 2017
012171a
Clarifying comment about destroy hooks in test-promise.js
May 23, 2017
4e38bda
Remove redundant domain tracking
May 23, 2017
13313bb
Fix lint errors
May 24, 2017
9b9c310
Performance improvements
May 24, 2017
81ff7f6
Appease linter
May 24, 2017
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
17 changes: 11 additions & 6 deletions lib/async_hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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.
Expand Down
216 changes: 151 additions & 65 deletions src/async-wrap.cc
View file
Open in desktop
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,141 @@ static void PushBackDestroyId(Environment* env, double id) {
}


bool DomainEnter(Environment* env, Local<Object> object) {
Local<Value> domain_v = object->Get(env->domain_string());
if (domain_v->IsObject()) {
Local<Object> domain = domain_v.As<Object>();
if (domain->Get(env->disposed_string())->IsTrue())
return true;
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");
}
}
}
return false;
}


bool DomainExit(Environment* env, v8::Local<v8::Object> object) {
Local<Value> domain_v = object->Get(env->domain_string());
if (domain_v->IsObject()) {
Local<Object> domain = domain_v.As<Object>();
if (domain->Get(env->disposed_string())->IsTrue())
return true;
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");
}
}
}
return false;
}


static bool PreCallbackExecution(AsyncWrap* wrap, bool run_domain_cbs) {
AsyncHooks* async_hooks = wrap->env()->async_hooks();

if (wrap->env()->using_domains() && run_domain_cbs) {
bool is_disposed = DomainEnter(wrap->env(), wrap->object());
if (is_disposed)
return false;
}

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

@trevnorris trevnorris May 19, 2017

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

@matthewloring matthewloring May 24, 2017

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, bool run_domain_cbs) {
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() && run_domain_cbs) {
bool is_disposed = DomainExit(wrap->env(), wrap->object());
if (is_disposed)
return false;
}

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 PromiseHook(PromiseHookType type, Local<Promise> promise,
Local<Value> parent, void* arg) {
Local<Context> context = promise->CreationContext();
Environment* env = Environment::GetCurrent(context);
if (type == PromiseHookType::kInit) {
Copy link
Author

@matthewloring matthewloring May 15, 2017

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 •
edited
Loading

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

@matthewloring matthewloring May 17, 2017

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

@addaleax addaleax May 17, 2017

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 •
edited
Loading

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 •
edited
Loading

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

@addaleax addaleax May 17, 2017

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

@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 see, that is sad :(

Copy link
Contributor

@trevnorris trevnorris May 17, 2017

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

@matthewloring matthewloring May 23, 2017

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<Object> obj =
env->async_hooks_promise_object()->NewInstance(context).ToLocalChecked();
PromiseWrap* wrap = new PromiseWrap(env, obj);
v8::PropertyAttribute hidden =
static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete | v8::DontEnum);
promise->DefineOwnProperty(context,
env->promise_wrap(),
v8::External::New(env->isolate(), wrap),
hidden).FromJust();
// The async tag will be destroyed at the same time as the promise as the
// only reference to it is held by the promise. This allows the promise
// wrap instance to be notified when the promise is destroyed.
promise->DefineOwnProperty(context,
env->promise_async_tag(),
Copy link
Member

@AndreasMadsen AndreasMadsen May 21, 2017

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

@matthewloring matthewloring May 23, 2017

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, hidden).FromJust();
} else if (type == PromiseHookType::kResolve) {
// TODO(matthewloring): need to expose this through the async hooks api.
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.

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

@addaleax addaleax May 12, 2017

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?

Copy link

@Jeff-Lewis Jeff-Lewis 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 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 •
edited
Loading

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 •
edited
Loading

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

@trevnorris trevnorris May 19, 2017

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

@matthewloring matthewloring May 22, 2017

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

@trevnorris trevnorris May 23, 2017

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

@AndreasMadsen AndreasMadsen May 23, 2017

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

@matthewloring matthewloring May 23, 2017

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_wrap()).ToLocalChecked();
PromiseWrap* wrap =
static_cast<PromiseWrap*>(external_wrap.As<v8::External>()->Value());
CHECK_NE(wrap, nullptr);
if (type == PromiseHookType::kBefore) {
PreCallbackExecution(wrap, false);
} else if (type == PromiseHookType::kAfter) {
PostCallbackExecution(wrap, false);
}
}


static void SetupHooks(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

Expand All @@ -201,6 +338,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 @@ -262,6 +400,11 @@ void AsyncWrap::Initialize(Local<Object> target,
env->SetMethod(target, "clearIdStack", ClearIdStack);
env->SetMethod(target, "addIdToDestroyList", QueueDestroyId);

Local<v8::ObjectTemplate> promise_object_template =
v8::ObjectTemplate::New(env->isolate());
promise_object_template->SetInternalFieldCount(1);
env->set_async_hooks_promise_object(promise_object_template);

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

Expand Down Expand Up @@ -416,87 +559,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>();
}
}
Environment::AsyncHooks::ExecScope exec_scope(env(),
get_id(),
Copy link
Contributor

@trevnorris trevnorris May 19, 2017

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

@addaleax addaleax May 21, 2017

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

@trevnorris trevnorris May 23, 2017

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

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, true)) {
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, true)) {
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
4 changes: 4 additions & 0 deletions src/async-wrap.h
View file
Open in desktop
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 Expand Up @@ -123,6 +124,9 @@ class AsyncWrap : public BaseObject {

void LoadAsyncWrapperInfo(Environment* env);

bool DomainEnter(Environment* env, v8::Local<v8::Object> object);
bool DomainExit(Environment* env, v8::Local<v8::Object> object);

} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Expand Down
3 changes: 3 additions & 0 deletions src/env.h
View file
Open in desktop
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_wrap, "_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 Expand Up @@ -256,6 +258,7 @@ namespace node {
V(async_hooks_init_function, v8::Function) \
V(async_hooks_before_function, v8::Function) \
V(async_hooks_after_function, v8::Function) \
V(async_hooks_promise_object, v8::ObjectTemplate) \
V(binding_cache_object, v8::Object) \
V(buffer_constructor_function, v8::Function) \
V(buffer_prototype_object, v8::Object) \
Expand Down
35 changes: 3 additions & 32 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1124,7 +1124,6 @@ void DomainPromiseHook(PromiseHookType type,
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(),
Expand All @@ -1133,38 +1132,10 @@ void DomainPromiseHook(PromiseHookType type,
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");
}
}
DomainEnter(env, promise);
} else if (type == PromiseHookType::kAfter) {
DomainExit(env, promise);
}
}

Expand Down
Loading