From b5319a56214dc0b94159a3a0623e648e9cc4cf4a Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Thu, 11 Feb 2016 13:09:52 -0700
Subject: [PATCH 01/18] src: fix MakeCallback error handling
Implementations of error handling between node::MakeCallback() and
AsyncWrap::MakeCallback() do not return at the same point. Make both
executions work the same by moving the early return if there's a caught
exception just after the AsyncWrap post callback. Since the domain's
call stack is cleared on a caught exception there is no reason to call
its exit() callback.
Remove the SetVerbose() statement in the AsyncWrap pre/post callback
calls since it does not affect the callback call.
PR-URL: https://github.com/nodejs/node/pull/4507
Reviewed-By: Fedor Indutny <fedor@indutny.com>
---
src/async-wrap.cc | 13 +++++--------
src/node.cc | 13 +++++--------
2 files changed, 10 insertions(+), 16 deletions(-)
diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index c9f5caad1e4ea8..29ea139f5f91c0 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -207,25 +207,22 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
}
if (ran_init_callback() && !pre_fn.IsEmpty()) {
- try_catch.SetVerbose(false);
pre_fn->Call(context, 0, nullptr);
if (try_catch.HasCaught())
FatalError("node::AsyncWrap::MakeCallback", "pre hook threw");
- try_catch.SetVerbose(true);
}
Local<Value> ret = cb->Call(context, argc, argv);
- if (try_catch.HasCaught()) {
- return Undefined(env()->isolate());
- }
-
if (ran_init_callback() && !post_fn.IsEmpty()) {
- try_catch.SetVerbose(false);
post_fn->Call(context, 0, nullptr);
if (try_catch.HasCaught())
FatalError("node::AsyncWrap::MakeCallback", "post hook threw");
- try_catch.SetVerbose(true);
+ }
+
+ // If the return value is empty then the callback threw.
+ if (ret.IsEmpty()) {
+ return Undefined(env()->isolate());
}
if (has_domain) {
diff --git a/src/node.cc b/src/node.cc
index 8264bfe8bba8a3..b048166cd9d8df 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1165,21 +1165,22 @@ Local<Value> MakeCallback(Environment* env,
}
if (ran_init_callback && !pre_fn.IsEmpty()) {
- try_catch.SetVerbose(false);
pre_fn->Call(object, 0, nullptr);
if (try_catch.HasCaught())
FatalError("node::MakeCallback", "pre hook threw");
- try_catch.SetVerbose(true);
}
Local<Value> ret = callback->Call(recv, argc, argv);
if (ran_init_callback && !post_fn.IsEmpty()) {
- try_catch.SetVerbose(false);
post_fn->Call(object, 0, nullptr);
if (try_catch.HasCaught())
FatalError("node::MakeCallback", "post hook threw");
- try_catch.SetVerbose(true);
+ }
+
+ // If the return value is empty then the callback threw.
+ if (ret.IsEmpty()) {
+ return Undefined(env->isolate());
}
if (has_domain) {
@@ -1191,10 +1192,6 @@ Local<Value> MakeCallback(Environment* env,
}
}
- if (try_catch.HasCaught()) {
- return Undefined(env->isolate());
- }
-
if (!env->KickNextTick())
return Undefined(env->isolate());
From 19dfb2adc1424326348fc2216470bf3b003ab1e1 Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Tue, 5 Jan 2016 15:33:21 -0700
Subject: [PATCH 02/18] src: add AsyncCallbackScope
Add a scope that will allow MakeCallback to know whether or not it's
currently running. This will prevent nextTickQueue and the
MicrotaskQueue from being processed recursively. It is also required to
wrap the bootloading stage since it doesn't run within a MakeCallback.
Ref: https://github.com/nodejs/node-v0.x-archive/issues/9245
PR-URL: https://github.com/nodejs/node/pull/4507
Reviewed-By: Fedor Indutny <fedor@indutny.com>
---
src/async-wrap.cc | 8 +++-----
src/env-inl.h | 15 +++++++++++++++
src/env.cc | 8 ++------
src/env.h | 16 +++++++++++++++-
src/node.cc | 9 +++++++--
src/node_http_parser.cc | 4 +++-
src/node_internals.h | 2 --
7 files changed, 45 insertions(+), 17 deletions(-)
diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index 29ea139f5f91c0..01dcaf277c1840 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -184,6 +184,8 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
Local<Object> domain;
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();
@@ -236,7 +238,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
Environment::TickInfo* tick_info = env()->tick_info();
- if (tick_info->in_tick()) {
+ if (callback_scope.in_makecallback()) {
return ret;
}
@@ -249,12 +251,8 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
return ret;
}
- tick_info->set_in_tick(true);
-
env()->tick_callback_function()->Call(process, 0, nullptr);
- tick_info->set_in_tick(false);
-
if (try_catch.HasCaught()) {
tick_info->set_last_threw(true);
return Undefined(env()->isolate());
diff --git a/src/env-inl.h b/src/env-inl.h
index f73e9c6ba2000a..eebb68eb463e93 100644
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -88,6 +88,20 @@ inline void Environment::AsyncHooks::set_enable_callbacks(uint32_t flag) {
fields_[kEnableCallbacks] = flag;
}
+inline Environment::AsyncCallbackScope::AsyncCallbackScope(Environment* env)
+ : env_(env) {
+ env_->makecallback_cntr_++;
+}
+
+inline Environment::AsyncCallbackScope::~AsyncCallbackScope() {
+ env_->makecallback_cntr_--;
+ CHECK_GE(env_->makecallback_cntr_, 0);
+}
+
+inline bool Environment::AsyncCallbackScope::in_makecallback() {
+ return env_->makecallback_cntr_ > 1;
+}
+
inline Environment::DomainFlag::DomainFlag() {
for (int i = 0; i < kFieldsCount; ++i) fields_[i] = 0;
}
@@ -210,6 +224,7 @@ inline Environment::Environment(v8::Local<v8::Context> context,
using_domains_(false),
printed_error_(false),
trace_sync_io_(false),
+ makecallback_cntr_(0),
async_wrap_uid_(0),
debugger_agent_(this),
http_parser_buffer_(nullptr),
diff --git a/src/env.cc b/src/env.cc
index e28866efd06894..8fa6e5c43afe06 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -57,10 +57,10 @@ void Environment::PrintSyncTrace() const {
}
-bool Environment::KickNextTick() {
+bool Environment::KickNextTick(Environment::AsyncCallbackScope* scope) {
TickInfo* info = tick_info();
- if (info->in_tick()) {
+ if (scope->in_makecallback()) {
return true;
}
@@ -73,15 +73,11 @@ bool Environment::KickNextTick() {
return true;
}
- info->set_in_tick(true);
-
// process nextTicks after call
TryCatch try_catch;
try_catch.SetVerbose(true);
tick_callback_function()->Call(process_object(), 0, nullptr);
- info->set_in_tick(false);
-
if (try_catch.HasCaught()) {
info->set_last_threw(true);
return false;
diff --git a/src/env.h b/src/env.h
index 1531d9911e310b..8ae681ff43c86f 100644
--- a/src/env.h
+++ b/src/env.h
@@ -294,6 +294,19 @@ class Environment {
DISALLOW_COPY_AND_ASSIGN(AsyncHooks);
};
+ class AsyncCallbackScope {
+ public:
+ explicit AsyncCallbackScope(Environment* env);
+ ~AsyncCallbackScope();
+
+ inline bool in_makecallback();
+
+ private:
+ Environment* env_;
+
+ DISALLOW_COPY_AND_ASSIGN(AsyncCallbackScope);
+ };
+
class DomainFlag {
public:
inline uint32_t* fields();
@@ -446,7 +459,7 @@ class Environment {
inline int64_t get_async_wrap_uid();
- bool KickNextTick();
+ bool KickNextTick(AsyncCallbackScope* scope);
inline uint32_t* heap_statistics_buffer() const;
inline void set_heap_statistics_buffer(uint32_t* pointer);
@@ -541,6 +554,7 @@ class Environment {
bool using_domains_;
bool printed_error_;
bool trace_sync_io_;
+ size_t makecallback_cntr_;
int64_t async_wrap_uid_;
debugger::Agent debugger_agent_;
diff --git a/src/node.cc b/src/node.cc
index b048166cd9d8df..ef59a103c30eac 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1132,6 +1132,8 @@ Local<Value> MakeCallback(Environment* env,
bool ran_init_callback = false;
bool has_domain = false;
+ Environment::AsyncCallbackScope callback_scope(env);
+
// TODO(trevnorris): Adding "_asyncQueue" to the "this" in the init callback
// is a horrible way to detect usage. Rethink how detection should happen.
if (recv->IsObject()) {
@@ -1192,7 +1194,7 @@ Local<Value> MakeCallback(Environment* env,
}
}
- if (!env->KickNextTick())
+ if (!env->KickNextTick(&callback_scope))
return Undefined(env->isolate());
return ret;
@@ -4100,7 +4102,10 @@ static void StartNodeInstance(void* arg) {
if (instance_data->use_debug_agent())
StartDebug(env, debug_wait_connect);
- LoadEnvironment(env);
+ {
+ Environment::AsyncCallbackScope callback_scope(env);
+ LoadEnvironment(env);
+ }
env->set_trace_sync_io(trace_sync_io);
diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc
index 8c976b2e9f0b4c..c6254d57c840ed 100644
--- a/src/node_http_parser.cc
+++ b/src/node_http_parser.cc
@@ -579,6 +579,8 @@ class Parser : public BaseObject {
if (!cb->IsFunction())
return;
+ Environment::AsyncCallbackScope callback_scope(parser->env());
+
// Hooks for GetCurrentBuffer
parser->current_buffer_len_ = nread;
parser->current_buffer_data_ = buf->base;
@@ -588,7 +590,7 @@ class Parser : public BaseObject {
parser->current_buffer_len_ = 0;
parser->current_buffer_data_ = nullptr;
- parser->env()->KickNextTick();
+ parser->env()->KickNextTick(&callback_scope);
}
diff --git a/src/node_internals.h b/src/node_internals.h
index 3df7676d2a9d51..6e3f7204777835 100644
--- a/src/node_internals.h
+++ b/src/node_internals.h
@@ -69,8 +69,6 @@ v8::Local<v8::Value> MakeCallback(Environment* env,
int argc = 0,
v8::Local<v8::Value>* argv = nullptr);
-bool KickNextTick();
-
// Convert a struct sockaddr to a { address: '1.2.3.4', port: 1234 } JS object.
// Sets address and port properties on the info object and returns it.
// If |info| is omitted, a new object is returned.
From 7d369f1fb63cac32fff69fa2d68beefb0a55a9bb Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Thu, 11 Feb 2016 13:10:47 -0700
Subject: [PATCH 03/18] src: remove unused of TickInfo::last_threw()
Environment::TickInfo::last_threw() is no longer in use.
Also pass Isolate to few methods and fix whitespace alignment.
PR-URL: https://github.com/nodejs/node/pull/4507
Reviewed-By: Fedor Indutny <fedor@indutny.com>
---
src/async-wrap.cc | 5 ++---
src/env-inl.h | 10 +---------
src/env.cc | 3 +--
src/env.h | 3 ---
src/node.cc | 10 +++++-----
5 files changed, 9 insertions(+), 22 deletions(-)
diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index 01dcaf277c1840..789e357c732773 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -173,8 +173,8 @@ void LoadAsyncWrapperInfo(Environment* env) {
Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
- int argc,
- Local<Value>* argv) {
+ int argc,
+ Local<Value>* argv) {
CHECK(env()->context() == env()->isolate()->GetCurrentContext());
Local<Function> pre_fn = env()->async_hooks_pre_function();
@@ -254,7 +254,6 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
env()->tick_callback_function()->Call(process, 0, nullptr);
if (try_catch.HasCaught()) {
- tick_info->set_last_threw(true);
return Undefined(env()->isolate());
}
diff --git a/src/env-inl.h b/src/env-inl.h
index eebb68eb463e93..f50ec13732e015 100644
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -118,7 +118,7 @@ inline uint32_t Environment::DomainFlag::count() const {
return fields_[kCount];
}
-inline Environment::TickInfo::TickInfo() : in_tick_(false), last_threw_(false) {
+inline Environment::TickInfo::TickInfo() : in_tick_(false) {
for (int i = 0; i < kFieldsCount; ++i)
fields_[i] = 0;
}
@@ -139,10 +139,6 @@ inline uint32_t Environment::TickInfo::index() const {
return fields_[kIndex];
}
-inline bool Environment::TickInfo::last_threw() const {
- return last_threw_;
-}
-
inline uint32_t Environment::TickInfo::length() const {
return fields_[kLength];
}
@@ -155,10 +151,6 @@ inline void Environment::TickInfo::set_index(uint32_t value) {
fields_[kIndex] = value;
}
-inline void Environment::TickInfo::set_last_threw(bool value) {
- last_threw_ = value;
-}
-
inline Environment::ArrayBufferAllocatorInfo::ArrayBufferAllocatorInfo() {
for (int i = 0; i < kFieldsCount; ++i)
fields_[i] = 0;
diff --git a/src/env.cc b/src/env.cc
index 8fa6e5c43afe06..a9b108c47a49be 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -74,12 +74,11 @@ bool Environment::KickNextTick(Environment::AsyncCallbackScope* scope) {
}
// process nextTicks after call
- TryCatch try_catch;
+ TryCatch try_catch(isolate());
try_catch.SetVerbose(true);
tick_callback_function()->Call(process_object(), 0, nullptr);
if (try_catch.HasCaught()) {
- info->set_last_threw(true);
return false;
}
diff --git a/src/env.h b/src/env.h
index 8ae681ff43c86f..5849d7a92107e2 100644
--- a/src/env.h
+++ b/src/env.h
@@ -332,12 +332,10 @@ class Environment {
inline uint32_t* fields();
inline int fields_count() const;
inline bool in_tick() const;
- inline bool last_threw() const;
inline uint32_t index() const;
inline uint32_t length() const;
inline void set_in_tick(bool value);
inline void set_index(uint32_t value);
- inline void set_last_threw(bool value);
private:
friend class Environment; // So we can call the constructor.
@@ -351,7 +349,6 @@ class Environment {
uint32_t fields_[kFieldsCount];
bool in_tick_;
- bool last_threw_;
DISALLOW_COPY_AND_ASSIGN(TickInfo);
};
diff --git a/src/node.cc b/src/node.cc
index ef59a103c30eac..fc39b8399a1ffa 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1119,10 +1119,10 @@ void SetupPromises(const FunctionCallbackInfo<Value>& args) {
Local<Value> MakeCallback(Environment* env,
- Local<Value> recv,
- const Local<Function> callback,
- int argc,
- Local<Value> argv[]) {
+ Local<Value> recv,
+ const Local<Function> callback,
+ int argc,
+ Local<Value> argv[]) {
// If you hit this assertion, you forgot to enter the v8::Context first.
CHECK_EQ(env->context(), env->isolate()->GetCurrentContext());
@@ -1154,7 +1154,7 @@ Local<Value> MakeCallback(Environment* env,
}
}
- TryCatch try_catch;
+ TryCatch try_catch(env->isolate());
try_catch.SetVerbose(true);
if (has_domain) {
From 0da31a1e1eb44b0601953d140b770e433796bdd9 Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Wed, 10 Feb 2016 12:48:44 -0700
Subject: [PATCH 04/18] src: remove unused TickInfo::in_tick()
PR-URL: https://github.com/nodejs/node/pull/4507
Reviewed-By: Fedor Indutny <fedor@indutny.com>
---
src/env-inl.h | 10 +---------
src/env.h | 3 ---
2 files changed, 1 insertion(+), 12 deletions(-)
diff --git a/src/env-inl.h b/src/env-inl.h
index f50ec13732e015..9084cb5159e58a 100644
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -118,7 +118,7 @@ inline uint32_t Environment::DomainFlag::count() const {
return fields_[kCount];
}
-inline Environment::TickInfo::TickInfo() : in_tick_(false) {
+inline Environment::TickInfo::TickInfo() {
for (int i = 0; i < kFieldsCount; ++i)
fields_[i] = 0;
}
@@ -131,10 +131,6 @@ inline int Environment::TickInfo::fields_count() const {
return kFieldsCount;
}
-inline bool Environment::TickInfo::in_tick() const {
- return in_tick_;
-}
-
inline uint32_t Environment::TickInfo::index() const {
return fields_[kIndex];
}
@@ -143,10 +139,6 @@ inline uint32_t Environment::TickInfo::length() const {
return fields_[kLength];
}
-inline void Environment::TickInfo::set_in_tick(bool value) {
- in_tick_ = value;
-}
-
inline void Environment::TickInfo::set_index(uint32_t value) {
fields_[kIndex] = value;
}
diff --git a/src/env.h b/src/env.h
index 5849d7a92107e2..44318da1fa3d0f 100644
--- a/src/env.h
+++ b/src/env.h
@@ -331,10 +331,8 @@ class Environment {
public:
inline uint32_t* fields();
inline int fields_count() const;
- inline bool in_tick() const;
inline uint32_t index() const;
inline uint32_t length() const;
- inline void set_in_tick(bool value);
inline void set_index(uint32_t value);
private:
@@ -348,7 +346,6 @@ class Environment {
};
uint32_t fields_[kFieldsCount];
- bool in_tick_;
DISALLOW_COPY_AND_ASSIGN(TickInfo);
};
From a6a738286036ae9d6ee9f61dd528df453da7fc09 Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Thu, 11 Feb 2016 13:57:26 -0700
Subject: [PATCH 05/18] src: remove TryCatch in MakeCallback
After attempting to use ReThrow() and Reset() there were cases where
firing the domain's error handlers was not happening. Or in some cases
reentering MakeCallback would still cause the domain enter callback to
abort (because the error had not been Reset yet).
In order for the script to properly stop execution when a subsequent
call to MakeCallback throws it must not be located within a TryCatch.
PR-URL: https://github.com/nodejs/node/pull/4507
Reviewed-By: Fedor Indutny <fedor@indutny.com>
---
src/async-wrap.cc | 28 +++++++++++-----------------
src/env.cc | 13 ++++---------
src/node.cc | 27 ++++++++++++---------------
3 files changed, 27 insertions(+), 41 deletions(-)
diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index 789e357c732773..a7a9822238329a 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -196,33 +196,28 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
}
}
- TryCatch try_catch(env()->isolate());
- try_catch.SetVerbose(true);
-
if (has_domain) {
Local<Value> enter_v = domain->Get(env()->enter_string());
if (enter_v->IsFunction()) {
- enter_v.As<Function>()->Call(domain, 0, nullptr);
- if (try_catch.HasCaught())
- return Undefined(env()->isolate());
+ if (enter_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) {
+ FatalError("node::AsyncWrap::MakeCallback",
+ "domain enter callback threw, please report this");
+ }
}
}
if (ran_init_callback() && !pre_fn.IsEmpty()) {
- pre_fn->Call(context, 0, nullptr);
- if (try_catch.HasCaught())
+ if (pre_fn->Call(context, 0, nullptr).IsEmpty())
FatalError("node::AsyncWrap::MakeCallback", "pre hook threw");
}
Local<Value> ret = cb->Call(context, argc, argv);
if (ran_init_callback() && !post_fn.IsEmpty()) {
- post_fn->Call(context, 0, nullptr);
- if (try_catch.HasCaught())
+ if (post_fn->Call(context, 0, nullptr).IsEmpty())
FatalError("node::AsyncWrap::MakeCallback", "post hook threw");
}
- // If the return value is empty then the callback threw.
if (ret.IsEmpty()) {
return Undefined(env()->isolate());
}
@@ -230,9 +225,10 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
if (has_domain) {
Local<Value> exit_v = domain->Get(env()->exit_string());
if (exit_v->IsFunction()) {
- exit_v.As<Function>()->Call(domain, 0, nullptr);
- if (try_catch.HasCaught())
- return Undefined(env()->isolate());
+ if (exit_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) {
+ FatalError("node::AsyncWrap::MakeCallback",
+ "domain exit callback threw, please report this");
+ }
}
}
@@ -251,9 +247,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
return ret;
}
- env()->tick_callback_function()->Call(process, 0, nullptr);
-
- if (try_catch.HasCaught()) {
+ if (env()->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
return Undefined(env()->isolate());
}
diff --git a/src/env.cc b/src/env.cc
index a9b108c47a49be..144a37f2e3cb67 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -11,6 +11,7 @@ using v8::Message;
using v8::StackFrame;
using v8::StackTrace;
using v8::TryCatch;
+using v8::Value;
void Environment::PrintSyncTrace() const {
if (!trace_sync_io_)
@@ -73,16 +74,10 @@ bool Environment::KickNextTick(Environment::AsyncCallbackScope* scope) {
return true;
}
- // process nextTicks after call
- TryCatch try_catch(isolate());
- try_catch.SetVerbose(true);
- tick_callback_function()->Call(process_object(), 0, nullptr);
+ Local<Value> ret =
+ tick_callback_function()->Call(process_object(), 0, nullptr);
- if (try_catch.HasCaught()) {
- return false;
- }
-
- return true;
+ return !ret.IsEmpty();
}
} // namespace node
diff --git a/src/node.cc b/src/node.cc
index fc39b8399a1ffa..5b4b2f8c8affa0 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1154,33 +1154,28 @@ Local<Value> MakeCallback(Environment* env,
}
}
- TryCatch try_catch(env->isolate());
- try_catch.SetVerbose(true);
-
if (has_domain) {
Local<Value> enter_v = domain->Get(env->enter_string());
if (enter_v->IsFunction()) {
- enter_v.As<Function>()->Call(domain, 0, nullptr);
- if (try_catch.HasCaught())
- return Undefined(env->isolate());
+ if (enter_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) {
+ FatalError("node::MakeCallback",
+ "domain enter callback threw, please report this");
+ }
}
}
if (ran_init_callback && !pre_fn.IsEmpty()) {
- pre_fn->Call(object, 0, nullptr);
- if (try_catch.HasCaught())
+ if (pre_fn->Call(object, 0, nullptr).IsEmpty())
FatalError("node::MakeCallback", "pre hook threw");
}
Local<Value> ret = callback->Call(recv, argc, argv);
if (ran_init_callback && !post_fn.IsEmpty()) {
- post_fn->Call(object, 0, nullptr);
- if (try_catch.HasCaught())
+ if (post_fn->Call(object, 0, nullptr).IsEmpty())
FatalError("node::MakeCallback", "post hook threw");
}
- // If the return value is empty then the callback threw.
if (ret.IsEmpty()) {
return Undefined(env->isolate());
}
@@ -1188,14 +1183,16 @@ Local<Value> MakeCallback(Environment* env,
if (has_domain) {
Local<Value> exit_v = domain->Get(env->exit_string());
if (exit_v->IsFunction()) {
- exit_v.As<Function>()->Call(domain, 0, nullptr);
- if (try_catch.HasCaught())
- return Undefined(env->isolate());
+ if (exit_v.As<Function>()->Call(domain, 0, nullptr).IsEmpty()) {
+ FatalError("node::MakeCallback",
+ "domain exit callback threw, please report this");
+ }
}
}
- if (!env->KickNextTick(&callback_scope))
+ if (!env->KickNextTick(&callback_scope)) {
return Undefined(env->isolate());
+ }
return ret;
}
From 1adb057b5b92f4ea7a99c7287f2acf6c8165e66f Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Wed, 6 Jan 2016 10:39:40 -0700
Subject: [PATCH 06/18] test: add addons test for MakeCallback
Make sure that calling MakeCallback multiple times within the same stack
does not allow the nextTickQueue or MicrotaskQueue to be processed in
any more than the first MakeCallback call.
Check that domains enter/exit poperly with multiple MakeCallback calls
and that errors are handled as expected
PR-URL: https://github.com/nodejs/node/pull/4507
Reviewed-By: Fedor Indutny <fedor@indutny.com>
---
test/addons/make-callback-recurse/binding.cc | 31 ++++
test/addons/make-callback-recurse/binding.gyp | 8 +
test/addons/make-callback-recurse/test.js | 170 ++++++++++++++++++
3 files changed, 209 insertions(+)
create mode 100644 test/addons/make-callback-recurse/binding.cc
create mode 100644 test/addons/make-callback-recurse/binding.gyp
create mode 100644 test/addons/make-callback-recurse/test.js
diff --git a/test/addons/make-callback-recurse/binding.cc b/test/addons/make-callback-recurse/binding.cc
new file mode 100644
index 00000000000000..1c575910ef66f5
--- /dev/null
+++ b/test/addons/make-callback-recurse/binding.cc
@@ -0,0 +1,31 @@
+#include "node.h"
+#include "v8.h"
+
+#include "../../../src/util.h"
+
+using v8::Function;
+using v8::FunctionCallbackInfo;
+using v8::Isolate;
+using v8::Local;
+using v8::Object;
+using v8::Value;
+
+namespace {
+
+void MakeCallback(const FunctionCallbackInfo<Value>& args) {
+ CHECK(args[0]->IsObject());
+ CHECK(args[1]->IsFunction());
+ Isolate* isolate = args.GetIsolate();
+ Local<Object> recv = args[0].As<Object>();
+ Local<Function> method = args[1].As<Function>();
+
+ node::MakeCallback(isolate, recv, method, 0, nullptr);
+}
+
+void Initialize(Local<Object> target) {
+ NODE_SET_METHOD(target, "makeCallback", MakeCallback);
+}
+
+} // namespace anonymous
+
+NODE_MODULE(binding, Initialize)
diff --git a/test/addons/make-callback-recurse/binding.gyp b/test/addons/make-callback-recurse/binding.gyp
new file mode 100644
index 00000000000000..3bfb84493f3e87
--- /dev/null
+++ b/test/addons/make-callback-recurse/binding.gyp
@@ -0,0 +1,8 @@
+{
+ 'targets': [
+ {
+ 'target_name': 'binding',
+ 'sources': [ 'binding.cc' ]
+ }
+ ]
+}
diff --git a/test/addons/make-callback-recurse/test.js b/test/addons/make-callback-recurse/test.js
new file mode 100644
index 00000000000000..f924ac61916ba5
--- /dev/null
+++ b/test/addons/make-callback-recurse/test.js
@@ -0,0 +1,170 @@
+'use strict';
+
+const common = require('../../common');
+const assert = require('assert');
+const domain = require('domain');
+const binding = require('./build/Release/binding');
+const makeCallback = binding.makeCallback;
+
+// Make sure this is run in the future.
+const mustCallCheckDomains = common.mustCall(checkDomains);
+
+
+// Make sure that using MakeCallback allows the error to propagate.
+assert.throws(function() {
+ makeCallback({}, function() {
+ throw new Error('hi from domain error');
+ });
+});
+
+
+// Check the execution order of the nextTickQueue and MicrotaskQueue in
+// relation to running multiple MakeCallback's from bootstrap,
+// node::MakeCallback() and node::AsyncWrap::MakeCallback().
+// TODO(trevnorris): Is there a way to verify this is being run during
+// bootstrap?
+(function verifyExecutionOrder(arg) {
+ const results_arr = [];
+
+ // Processing of the MicrotaskQueue is manually handled by node. They are not
+ // processed until after the nextTickQueue has been processed.
+ Promise.resolve(1).then(common.mustCall(function() {
+ results_arr.push(7);
+ }));
+
+ // The nextTick should run after all immediately invoked calls.
+ process.nextTick(common.mustCall(function() {
+ results_arr.push(3);
+
+ // Run same test again but while processing the nextTickQueue to make sure
+ // the following MakeCallback call breaks in the middle of processing the
+ // queue and allows the script to run normally.
+ process.nextTick(common.mustCall(function() {
+ results_arr.push(6);
+ }));
+
+ makeCallback({}, common.mustCall(function() {
+ results_arr.push(4);
+ }));
+
+ results_arr.push(5);
+ }));
+
+ results_arr.push(0);
+
+ // MakeCallback is calling the function immediately, but should then detect
+ // that a script is already in the middle of execution and return before
+ // either the nextTickQueue or MicrotaskQueue are processed.
+ makeCallback({}, common.mustCall(function() {
+ results_arr.push(1);
+ }));
+
+ // This should run before either the nextTickQueue or MicrotaskQueue are
+ // processed. Previously MakeCallback would not detect this circumstance
+ // and process them immediately.
+ results_arr.push(2);
+
+ setImmediate(common.mustCall(function() {
+ for (var i = 0; i < results_arr.length; i++) {
+ assert.equal(results_arr[i],
+ i,
+ `verifyExecutionOrder(${arg}) results: ${results_arr}`);
+ }
+ if (arg === 1) {
+ // The tests are first run on bootstrap during LoadEnvironment() in
+ // src/node.cc. Now run the tests through node::MakeCallback().
+ setImmediate(function() {
+ makeCallback({}, common.mustCall(function() {
+ verifyExecutionOrder(2);
+ }));
+ });
+ } else if (arg === 2) {
+ // setTimeout runs via the TimerWrap, which runs through
+ // AsyncWrap::MakeCallback(). Make sure there are no conflicts using
+ // node::MakeCallback() within it.
+ setTimeout(common.mustCall(function() {
+ verifyExecutionOrder(3);
+ }), 10);
+ } else if (arg === 3) {
+ mustCallCheckDomains();
+ } else {
+ throw new Error('UNREACHABLE');
+ }
+ }));
+}(1));
+
+
+function checkDomains() {
+ // Check that domains are properly entered/exited when called in multiple
+ // levels from both node::MakeCallback() and AsyncWrap::MakeCallback
+ setImmediate(common.mustCall(function() {
+ const d1 = domain.create();
+ const d2 = domain.create();
+ const d3 = domain.create();
+
+ makeCallback({ domain: d1 }, common.mustCall(function() {
+ assert.equal(d1, process.domain);
+ makeCallback({ domain: d2 }, common.mustCall(function() {
+ assert.equal(d2, process.domain);
+ makeCallback({ domain: d3 }, common.mustCall(function() {
+ assert.equal(d3, process.domain);
+ }));
+ assert.equal(d2, process.domain);
+ }));
+ assert.equal(d1, process.domain);
+ }));
+ }));
+
+ setTimeout(common.mustCall(function() {
+ const d1 = domain.create();
+ const d2 = domain.create();
+ const d3 = domain.create();
+
+ makeCallback({ domain: d1 }, common.mustCall(function() {
+ assert.equal(d1, process.domain);
+ makeCallback({ domain: d2 }, common.mustCall(function() {
+ assert.equal(d2, process.domain);
+ makeCallback({ domain: d3 }, common.mustCall(function() {
+ assert.equal(d3, process.domain);
+ }));
+ assert.equal(d2, process.domain);
+ }));
+ assert.equal(d1, process.domain);
+ }));
+ }), 1);
+
+ // Make sure nextTick, setImmediate and setTimeout can all recover properly
+ // after a thrown makeCallback call.
+ process.nextTick(common.mustCall(function() {
+ const d = domain.create();
+ d.on('error', common.mustCall(function(e) {
+ assert.equal(e.message, 'throw from domain 3');
+ }));
+ makeCallback({ domain: d }, function() {
+ throw new Error('throw from domain 3');
+ });
+ throw new Error('UNREACHABLE');
+ }));
+
+ setImmediate(common.mustCall(function() {
+ const d = domain.create();
+ d.on('error', common.mustCall(function(e) {
+ assert.equal(e.message, 'throw from domain 2');
+ }));
+ makeCallback({ domain: d }, function() {
+ throw new Error('throw from domain 2');
+ });
+ throw new Error('UNREACHABLE');
+ }));
+
+ setTimeout(common.mustCall(function() {
+ const d = domain.create();
+ d.on('error', common.mustCall(function(e) {
+ assert.equal(e.message, 'throw from domain 1');
+ }));
+ makeCallback({ domain: d }, function() {
+ throw new Error('throw from domain 1');
+ });
+ throw new Error('UNREACHABLE');
+ }));
+}
From a321e6d44e0377933f08326ceaae1aca4cf06366 Mon Sep 17 00:00:00 2001
From: Brian White <mscdex@mscdex.net>
Date: Mon, 15 Feb 2016 04:45:20 -0500
Subject: [PATCH 07/18] src: remove unnecessary check
The value's type is unsigned so it will always be >= 0.
PR-URL: https://github.com/nodejs/node/pull/5233
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
src/env-inl.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/env-inl.h b/src/env-inl.h
index 9084cb5159e58a..6f19ff50cb536f 100644
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -95,7 +95,6 @@ inline Environment::AsyncCallbackScope::AsyncCallbackScope(Environment* env)
inline Environment::AsyncCallbackScope::~AsyncCallbackScope() {
env_->makecallback_cntr_--;
- CHECK_GE(env_->makecallback_cntr_, 0);
}
inline bool Environment::AsyncCallbackScope::in_makecallback() {
From 331b85c39bd835c65ce49e32dd1b7bec334a702c Mon Sep 17 00:00:00 2001
From: Andreas Madsen <amwebdk@gmail.com>
Date: Sat, 9 Jan 2016 10:58:55 +0100
Subject: [PATCH 08/18] async_wrap: add uid to all asyncWrap hooks
By doing this users can use a Map object for storing information
instead of modifying the handle object.
PR-URL: #4600
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
---
src/async-wrap.cc | 5 ++-
test/parallel/test-async-wrap-uid.js | 57 ++++++++++++++++++++++++++++
2 files changed, 60 insertions(+), 2 deletions(-)
create mode 100644 test/parallel/test-async-wrap-uid.js
diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index a7a9822238329a..11a696cc2329a3 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -179,6 +179,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
Local<Function> pre_fn = env()->async_hooks_pre_function();
Local<Function> post_fn = env()->async_hooks_post_function();
+ Local<Value> uid = Integer::New(env()->isolate(), get_uid());
Local<Object> context = object();
Local<Object> process = env()->process_object();
Local<Object> domain;
@@ -207,14 +208,14 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
}
if (ran_init_callback() && !pre_fn.IsEmpty()) {
- if (pre_fn->Call(context, 0, nullptr).IsEmpty())
+ if (pre_fn->Call(context, 1, &uid).IsEmpty())
FatalError("node::AsyncWrap::MakeCallback", "pre hook threw");
}
Local<Value> ret = cb->Call(context, argc, argv);
if (ran_init_callback() && !post_fn.IsEmpty()) {
- if (post_fn->Call(context, 0, nullptr).IsEmpty())
+ if (post_fn->Call(context, 1, &uid).IsEmpty())
FatalError("node::AsyncWrap::MakeCallback", "post hook threw");
}
diff --git a/test/parallel/test-async-wrap-uid.js b/test/parallel/test-async-wrap-uid.js
new file mode 100644
index 00000000000000..ad2dd5027cfa1d
--- /dev/null
+++ b/test/parallel/test-async-wrap-uid.js
@@ -0,0 +1,57 @@
+'use strict';
+
+require('../common');
+const fs = require('fs');
+const assert = require('assert');
+const async_wrap = process.binding('async_wrap');
+
+const storage = new Map();
+async_wrap.setupHooks(init, pre, post, destroy);
+async_wrap.enable();
+
+function init(provider, uid) {
+ storage.set(uid, {
+ init: true,
+ pre: false,
+ post: false,
+ destroy: false
+ });
+}
+
+function pre(uid) {
+ storage.get(uid).pre = true;
+}
+
+function post(uid) {
+ storage.get(uid).post = true;
+}
+
+function destroy(uid) {
+ storage.get(uid).destroy = true;
+}
+
+fs.access(__filename, function(err) {
+ assert.ifError(err);
+});
+
+fs.access(__filename, function(err) {
+ assert.ifError(err);
+});
+
+async_wrap.disable();
+
+process.once('exit', function() {
+ assert.strictEqual(storage.size, 2);
+
+ for (const item of storage) {
+ const uid = item[0];
+ const value = item[1];
+ assert.strictEqual(typeof uid, 'number');
+ assert.deepStrictEqual(value, {
+ init: true,
+ pre: true,
+ post: true,
+ destroy: true
+ });
+ }
+});
From 1c9cd4a3a6d10c6dd91c5af1264c2647221e3ad1 Mon Sep 17 00:00:00 2001
From: Andreas Madsen <amwebdk@gmail.com>
Date: Sat, 9 Jan 2016 12:27:53 +0100
Subject: [PATCH 09/18] async_wrap: make uid the first argument in init
All other hooks have uid as the first argument, this makes it concistent
for all hooks.
PR-URL: #4600
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
---
src/async-wrap-inl.h | 2 +-
test/parallel/test-async-wrap-check-providers.js | 4 ++--
test/parallel/test-async-wrap-disabled-propagate-parent.js | 2 +-
test/parallel/test-async-wrap-propagate-parent.js | 2 +-
test/parallel/test-async-wrap-uid.js | 2 +-
5 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h
index f2d2c3ecf1c7b4..d1197edc05f20c 100644
--- a/src/async-wrap-inl.h
+++ b/src/async-wrap-inl.h
@@ -40,8 +40,8 @@ inline AsyncWrap::AsyncWrap(Environment* env,
v8::HandleScope scope(env->isolate());
v8::Local<v8::Value> argv[] = {
- v8::Int32::New(env->isolate(), provider),
v8::Integer::New(env->isolate(), get_uid()),
+ v8::Int32::New(env->isolate(), provider),
Null(env->isolate())
};
diff --git a/test/parallel/test-async-wrap-check-providers.js b/test/parallel/test-async-wrap-check-providers.js
index 907b9fa3159926..c08c7a43d445e0 100644
--- a/test/parallel/test-async-wrap-check-providers.js
+++ b/test/parallel/test-async-wrap-check-providers.js
@@ -29,8 +29,8 @@ if (common.isAix) {
}
}
-function init(id) {
- keyList = keyList.filter((e) => e != pkeys[id]);
+function init(id, provider) {
+ keyList = keyList.filter((e) => e != pkeys[provider]);
}
function noop() { }
diff --git a/test/parallel/test-async-wrap-disabled-propagate-parent.js b/test/parallel/test-async-wrap-disabled-propagate-parent.js
index 55d2e59efb9ee7..f7badaffdffcb7 100644
--- a/test/parallel/test-async-wrap-disabled-propagate-parent.js
+++ b/test/parallel/test-async-wrap-disabled-propagate-parent.js
@@ -9,7 +9,7 @@ const providers = Object.keys(async_wrap.Providers);
let cntr = 0;
let client;
-function init(type, id, parent) {
+function init(id, type, parent) {
if (parent) {
cntr++;
// Cannot assert in init callback or will abort.
diff --git a/test/parallel/test-async-wrap-propagate-parent.js b/test/parallel/test-async-wrap-propagate-parent.js
index 6001539c624678..31aa814100069b 100644
--- a/test/parallel/test-async-wrap-propagate-parent.js
+++ b/test/parallel/test-async-wrap-propagate-parent.js
@@ -8,7 +8,7 @@ const async_wrap = process.binding('async_wrap');
let cntr = 0;
let client;
-function init(type, id, parent) {
+function init(id, type, parent) {
if (parent) {
cntr++;
// Cannot assert in init callback or will abort.
diff --git a/test/parallel/test-async-wrap-uid.js b/test/parallel/test-async-wrap-uid.js
index ad2dd5027cfa1d..4e664f1bd46ebb 100644
--- a/test/parallel/test-async-wrap-uid.js
+++ b/test/parallel/test-async-wrap-uid.js
@@ -9,7 +9,7 @@ const storage = new Map();
async_wrap.setupHooks(init, pre, post, destroy);
async_wrap.enable();
-function init(provider, uid) {
+function init(uid) {
storage.set(uid, {
init: true,
pre: false,
From 051103df35be0fcba774133ac1dc9f79b2ba8b40 Mon Sep 17 00:00:00 2001
From: Andreas Madsen <amwebdk@gmail.com>
Date: Wed, 20 Jan 2016 18:20:43 +0100
Subject: [PATCH 10/18] async_wrap: add parent uid to init hook
When the parent uid is required it is not necessary to store the uid in
the parent handle object.
PR-URL: #4600
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
---
src/async-wrap-inl.h | 7 +++++--
.../test-async-wrap-disabled-propagate-parent.js | 13 ++++++++++---
test/parallel/test-async-wrap-propagate-parent.js | 15 ++++++++++++---
3 files changed, 27 insertions(+), 8 deletions(-)
diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h
index d1197edc05f20c..eeea5973cac57b 100644
--- a/src/async-wrap-inl.h
+++ b/src/async-wrap-inl.h
@@ -42,11 +42,14 @@ inline AsyncWrap::AsyncWrap(Environment* env,
v8::Local<v8::Value> argv[] = {
v8::Integer::New(env->isolate(), get_uid()),
v8::Int32::New(env->isolate(), provider),
+ Null(env->isolate()),
Null(env->isolate())
};
- if (parent != nullptr)
- argv[2] = parent->object();
+ if (parent != nullptr) {
+ argv[2] = v8::Integer::New(env->isolate(), parent->get_uid());
+ argv[3] = parent->object();
+ }
v8::MaybeLocal<v8::Value> ret =
init_fn->Call(env->context(), object, arraysize(argv), argv);
diff --git a/test/parallel/test-async-wrap-disabled-propagate-parent.js b/test/parallel/test-async-wrap-disabled-propagate-parent.js
index f7badaffdffcb7..152af4bcde80fc 100644
--- a/test/parallel/test-async-wrap-disabled-propagate-parent.js
+++ b/test/parallel/test-async-wrap-disabled-propagate-parent.js
@@ -6,16 +6,23 @@ const net = require('net');
const async_wrap = process.binding('async_wrap');
const providers = Object.keys(async_wrap.Providers);
+const uidSymbol = Symbol('uid');
+
let cntr = 0;
let client;
-function init(id, type, parent) {
- if (parent) {
+function init(uid, type, parentUid, parentHandle) {
+ this[uidSymbol] = uid;
+
+ if (parentHandle) {
cntr++;
// Cannot assert in init callback or will abort.
process.nextTick(() => {
assert.equal(providers[type], 'TCPWRAP');
- assert.equal(parent, server._handle, 'server doesn\'t match parent');
+ assert.equal(parentUid, server._handle[uidSymbol],
+ 'server uid doesn\'t match parent uid');
+ assert.equal(parentHandle, server._handle,
+ 'server handle doesn\'t match parent handle');
assert.equal(this, client._handle, 'client doesn\'t match context');
});
}
diff --git a/test/parallel/test-async-wrap-propagate-parent.js b/test/parallel/test-async-wrap-propagate-parent.js
index 31aa814100069b..e6cb5fa77bdaf7 100644
--- a/test/parallel/test-async-wrap-propagate-parent.js
+++ b/test/parallel/test-async-wrap-propagate-parent.js
@@ -4,16 +4,25 @@ const common = require('../common');
const assert = require('assert');
const net = require('net');
const async_wrap = process.binding('async_wrap');
+const providers = Object.keys(async_wrap.Providers);
+
+const uidSymbol = Symbol('uid');
let cntr = 0;
let client;
-function init(id, type, parent) {
- if (parent) {
+function init(uid, type, parentUid, parentHandle) {
+ this[uidSymbol] = uid;
+
+ if (parentHandle) {
cntr++;
// Cannot assert in init callback or will abort.
process.nextTick(() => {
- assert.equal(parent, server._handle, 'server doesn\'t match parent');
+ assert.equal(providers[type], 'TCPWRAP');
+ assert.equal(parentUid, server._handle[uidSymbol],
+ 'server uid doesn\'t match parent uid');
+ assert.equal(parentHandle, server._handle,
+ 'server handle doesn\'t match parent handle');
assert.equal(this, client._handle, 'client doesn\'t match context');
});
}
From 8f4438d8c3f25c19489f44f2f987951b8e5cc783 Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Fri, 19 Feb 2016 13:13:04 -0700
Subject: [PATCH 11/18] http_parser: use `MakeCallback`
Make `HTTPParser` an instance of `AsyncWrap` and make it use
`MakeCallback`. This means that async wrap hooks will be called on
consumed TCP sockets as well as on non-consumed ones.
Additional uses of `AsyncCallbackScope` are necessary to prevent
improper state from progressing that triggers failure in the
test-http-pipeline-flood.js test. Optimally this wouldn't be necessary,
but for the time being it's the most sure way to allow operations to
proceed as they have.
Fix: https://github.com/nodejs/node/issues/4416
PR-URL: https://github.com/nodejs/node/pull/5419
Reviewed-By: Fedor Indutny <fedor@indutny.com>
---
src/async-wrap.h | 1 +
src/node_http_parser.cc | 27 ++++++++++++-------
.../test-async-wrap-check-providers.js | 3 +++
3 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/src/async-wrap.h b/src/async-wrap.h
index 5db29600bcd180..cb0c9e211a8923 100644
--- a/src/async-wrap.h
+++ b/src/async-wrap.h
@@ -17,6 +17,7 @@ namespace node {
V(FSREQWRAP) \
V(GETADDRINFOREQWRAP) \
V(GETNAMEINFOREQWRAP) \
+ V(HTTPPARSER) \
V(JSSTREAM) \
V(PIPEWRAP) \
V(PIPECONNECTWRAP) \
diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc
index c6254d57c840ed..87126bfb33a497 100644
--- a/src/node_http_parser.cc
+++ b/src/node_http_parser.cc
@@ -3,8 +3,8 @@
#include "node_http_parser.h"
#include "node_revert.h"
-#include "base-object.h"
-#include "base-object-inl.h"
+#include "async-wrap.h"
+#include "async-wrap-inl.h"
#include "env.h"
#include "env-inl.h"
#include "stream_base.h"
@@ -148,10 +148,10 @@ struct StringPtr {
};
-class Parser : public BaseObject {
+class Parser : public AsyncWrap {
public:
Parser(Environment* env, Local<Object> wrap, enum http_parser_type type)
- : BaseObject(env, wrap),
+ : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_HTTPPARSER),
current_buffer_len_(0),
current_buffer_data_(nullptr) {
Wrap(object(), this);
@@ -165,6 +165,11 @@ class Parser : public BaseObject {
}
+ size_t self_size() const override {
+ return sizeof(*this);
+ }
+
+
HTTP_CB(on_message_begin) {
num_fields_ = num_values_ = 0;
url_.Reset();
@@ -286,8 +291,10 @@ class Parser : public BaseObject {
argv[A_UPGRADE] = Boolean::New(env()->isolate(), parser_.upgrade);
+ Environment::AsyncCallbackScope callback_scope(env());
+
Local<Value> head_response =
- cb.As<Function>()->Call(obj, arraysize(argv), argv);
+ MakeCallback(cb.As<Function>(), arraysize(argv), argv);
if (head_response.IsEmpty()) {
got_exception_ = true;
@@ -322,7 +329,7 @@ class Parser : public BaseObject {
Integer::NewFromUnsigned(env()->isolate(), length)
};
- Local<Value> r = cb.As<Function>()->Call(obj, arraysize(argv), argv);
+ Local<Value> r = MakeCallback(cb.As<Function>(), arraysize(argv), argv);
if (r.IsEmpty()) {
got_exception_ = true;
@@ -345,7 +352,9 @@ class Parser : public BaseObject {
if (!cb->IsFunction())
return 0;
- Local<Value> r = cb.As<Function>()->Call(obj, 0, nullptr);
+ Environment::AsyncCallbackScope callback_scope(env());
+
+ Local<Value> r = MakeCallback(cb.As<Function>(), 0, nullptr);
if (r.IsEmpty()) {
got_exception_ = true;
@@ -585,7 +594,7 @@ class Parser : public BaseObject {
parser->current_buffer_len_ = nread;
parser->current_buffer_data_ = buf->base;
- cb.As<Function>()->Call(obj, 1, &ret);
+ parser->MakeCallback(cb.As<Function>(), 1, &ret);
parser->current_buffer_len_ = 0;
parser->current_buffer_data_ = nullptr;
@@ -659,7 +668,7 @@ class Parser : public BaseObject {
url_.ToString(env())
};
- Local<Value> r = cb.As<Function>()->Call(obj, arraysize(argv), argv);
+ Local<Value> r = MakeCallback(cb.As<Function>(), arraysize(argv), argv);
if (r.IsEmpty())
got_exception_ = true;
diff --git a/test/parallel/test-async-wrap-check-providers.js b/test/parallel/test-async-wrap-check-providers.js
index c08c7a43d445e0..3a2c80d9cbe173 100644
--- a/test/parallel/test-async-wrap-check-providers.js
+++ b/test/parallel/test-async-wrap-check-providers.js
@@ -11,6 +11,7 @@ const tls = require('tls');
const zlib = require('zlib');
const ChildProcess = require('child_process').ChildProcess;
const StreamWrap = require('_stream_wrap').StreamWrap;
+const HTTPParser = process.binding('http_parser').HTTPParser;
const async_wrap = process.binding('async_wrap');
const pkeys = Object.keys(async_wrap.Providers);
@@ -106,6 +107,8 @@ zlib.createGzip();
new ChildProcess();
+new HTTPParser(HTTPParser.REQUEST);
+
process.on('exit', function() {
if (keyList.length !== 0) {
process._rawDebug('Not all keys have been used:');
From 71d786271c4216b7eb0011d548414562645f2d9c Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Fri, 4 Mar 2016 15:14:27 -0700
Subject: [PATCH 12/18] src,http: fix uncaughtException miss in http
In AsyncWrap::MakeCallback always return empty handle if there is an
error. In the future this should change to return a v8::MaybeLocal, but
that major change will have to wait for v6.x, and these changes are
meant to be backported to v4.x.
The HTTParser call to AsyncWrap::MakeCallback failed because it expected
a thrown call to return an empty handle.
In node::MakeCallback return an empty handle if the call is
in_makecallback(), otherwise return v8::Undefined() as usual to preserve
backwards compatibility.
Fixes: https://github.com/nodejs/node/issues/5555
PR-URL: https://github.com/nodejs/node/pull/5591
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
---
src/async-wrap.cc | 6 ++---
src/node.cc | 6 ++++-
.../test-http-catch-uncaughtexception.js | 23 +++++++++++++++++++
3 files changed, 31 insertions(+), 4 deletions(-)
create mode 100644 test/parallel/test-http-catch-uncaughtexception.js
diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index 11a696cc2329a3..e2054031014680 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -193,7 +193,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
if (has_domain) {
domain = domain_v.As<Object>();
if (domain->Get(env()->disposed_string())->IsTrue())
- return Undefined(env()->isolate());
+ return Local<Value>();
}
}
@@ -220,7 +220,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
}
if (ret.IsEmpty()) {
- return Undefined(env()->isolate());
+ return ret;
}
if (has_domain) {
@@ -249,7 +249,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
}
if (env()->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
- return Undefined(env()->isolate());
+ return Local<Value>();
}
return ret;
diff --git a/src/node.cc b/src/node.cc
index 5b4b2f8c8affa0..056b3711f0c8da 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1177,7 +1177,11 @@ Local<Value> MakeCallback(Environment* env,
}
if (ret.IsEmpty()) {
- return Undefined(env->isolate());
+ if (callback_scope.in_makecallback())
+ return ret;
+ // NOTE: Undefined() is returned here for backwards compatibility.
+ else
+ return Undefined(env->isolate());
}
if (has_domain) {
diff --git a/test/parallel/test-http-catch-uncaughtexception.js b/test/parallel/test-http-catch-uncaughtexception.js
new file mode 100644
index 00000000000000..c4054aafbfb699
--- /dev/null
+++ b/test/parallel/test-http-catch-uncaughtexception.js
@@ -0,0 +1,23 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+const uncaughtCallback = common.mustCall(function(er) {
+ assert.equal(er.message, 'get did fail');
+});
+
+process.on('uncaughtException', uncaughtCallback);
+
+const server = http.createServer(function(req, res) {
+ res.writeHead(200, { 'Content-Type': 'text/plain' });
+ res.end('bye');
+}).listen(common.PORT, function() {
+ http.get({ port: common.PORT }, function(res) {
+ res.resume();
+ throw new Error('get did fail');
+ }).on('close', function() {
+ server.close();
+ });
+});
From d94eec1617eca6028555961efcc2ab403fdf6185 Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Fri, 11 Mar 2016 12:55:59 -0700
Subject: [PATCH 13/18] src,http_parser: remove KickNextTick call
Now that HTTPParser uses MakeCallback it is unnecessary to manually
process the nextTickQueue.
The KickNextTick function is now no longer needed so code has moved back
to node::MakeCallback to simplify implementation.
Include minor cleanup moving Environment::tick_info() call below the
early return to save an operation.
PR-URL: https://github.com/nodejs/node/pull/5756
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
---
src/async-wrap.cc | 7 ++++---
src/env.cc | 23 -----------------------
src/env.h | 2 --
src/node.cc | 18 +++++++++++++++++-
src/node_http_parser.cc | 4 ----
5 files changed, 21 insertions(+), 33 deletions(-)
diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index e2054031014680..dde07aa0756a3e 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -181,7 +181,6 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
Local<Function> post_fn = env()->async_hooks_post_function();
Local<Value> uid = Integer::New(env()->isolate(), get_uid());
Local<Object> context = object();
- Local<Object> process = env()->process_object();
Local<Object> domain;
bool has_domain = false;
@@ -233,16 +232,18 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
}
}
- Environment::TickInfo* tick_info = env()->tick_info();
-
if (callback_scope.in_makecallback()) {
return ret;
}
+ Environment::TickInfo* tick_info = env()->tick_info();
+
if (tick_info->length() == 0) {
env()->isolate()->RunMicrotasks();
}
+ Local<Object> process = env()->process_object();
+
if (tick_info->length() == 0) {
tick_info->set_index(0);
return ret;
diff --git a/src/env.cc b/src/env.cc
index 144a37f2e3cb67..2509a8fd44b6a2 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -57,27 +57,4 @@ void Environment::PrintSyncTrace() const {
fflush(stderr);
}
-
-bool Environment::KickNextTick(Environment::AsyncCallbackScope* scope) {
- TickInfo* info = tick_info();
-
- if (scope->in_makecallback()) {
- return true;
- }
-
- if (info->length() == 0) {
- isolate()->RunMicrotasks();
- }
-
- if (info->length() == 0) {
- info->set_index(0);
- return true;
- }
-
- Local<Value> ret =
- tick_callback_function()->Call(process_object(), 0, nullptr);
-
- return !ret.IsEmpty();
-}
-
} // namespace node
diff --git a/src/env.h b/src/env.h
index 44318da1fa3d0f..7b6ffc8b87c8ee 100644
--- a/src/env.h
+++ b/src/env.h
@@ -453,8 +453,6 @@ class Environment {
inline int64_t get_async_wrap_uid();
- bool KickNextTick(AsyncCallbackScope* scope);
-
inline uint32_t* heap_statistics_buffer() const;
inline void set_heap_statistics_buffer(uint32_t* pointer);
diff --git a/src/node.cc b/src/node.cc
index 056b3711f0c8da..6b095a6f5816b6 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1194,7 +1194,23 @@ Local<Value> MakeCallback(Environment* env,
}
}
- if (!env->KickNextTick(&callback_scope)) {
+ if (callback_scope.in_makecallback()) {
+ return ret;
+ }
+
+ Environment::TickInfo* tick_info = env->tick_info();
+
+ if (tick_info->length() == 0) {
+ env->isolate()->RunMicrotasks();
+ }
+
+ Local<Object> process = env->process_object();
+
+ if (tick_info->length() == 0) {
+ tick_info->set_index(0);
+ }
+
+ if (env->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
return Undefined(env->isolate());
}
diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc
index 87126bfb33a497..6b15aa8c72d531 100644
--- a/src/node_http_parser.cc
+++ b/src/node_http_parser.cc
@@ -588,8 +588,6 @@ class Parser : public AsyncWrap {
if (!cb->IsFunction())
return;
- Environment::AsyncCallbackScope callback_scope(parser->env());
-
// Hooks for GetCurrentBuffer
parser->current_buffer_len_ = nread;
parser->current_buffer_data_ = buf->base;
@@ -598,8 +596,6 @@ class Parser : public AsyncWrap {
parser->current_buffer_len_ = 0;
parser->current_buffer_data_ = nullptr;
-
- parser->env()->KickNextTick(&callback_scope);
}
From 780dd2d3d43f113bb82736bc1fcf2d446741a9a8 Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Wed, 23 Mar 2016 17:02:04 -0600
Subject: [PATCH 14/18] src: reword command and add ternary
Make comment clear that Undefined() is returned for legacy
compatibility. This will change in the future as a semver-major change,
but to be able to port this to previous releases it needs to stay as is.
PR-URL: https://github.com/nodejs/node/pull/5756
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
---
src/node.cc | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/node.cc b/src/node.cc
index 6b095a6f5816b6..074589542f59ba 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1177,11 +1177,10 @@ Local<Value> MakeCallback(Environment* env,
}
if (ret.IsEmpty()) {
- if (callback_scope.in_makecallback())
- return ret;
- // NOTE: Undefined() is returned here for backwards compatibility.
- else
- return Undefined(env->isolate());
+ // NOTE: For backwards compatibility with public API we return Undefined()
+ // if the top level call threw.
+ return callback_scope.in_makecallback() ?
+ ret : Undefined(env->isolate()).As<Value>();
}
if (has_domain) {
From 182b9de523af9c1b687dc16f4f7a18909cb314e0 Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Mon, 22 Feb 2016 16:34:35 -0700
Subject: [PATCH 15/18] async_wrap: setupHooks now accepts object
The number of callbacks accepted to setupHooks was getting unwieldy.
Instead change the implementation to accept an object with all callbacks
PR-URL: https://github.com/nodejs/node/pull/5756
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
---
src/async-wrap.cc | 35 ++++++++++++++-----
.../test-async-wrap-check-providers.js | 2 +-
...st-async-wrap-disabled-propagate-parent.js | 2 +-
.../test-async-wrap-propagate-parent.js | 2 +-
.../parallel/test-async-wrap-throw-no-init.js | 4 +--
test/parallel/test-async-wrap-uid.js | 2 +-
6 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index dde07aa0756a3e..570ed2f165b2ba 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -121,18 +121,35 @@ static void SetupHooks(const FunctionCallbackInfo<Value>& args) {
if (env->async_hooks()->callbacks_enabled())
return env->ThrowError("hooks should not be set while also enabled");
-
- if (!args[0]->IsFunction())
+ if (!args[0]->IsObject())
+ return env->ThrowTypeError("first argument must be an object");
+
+ Local<Object> fn_obj = args[0].As<Object>();
+
+ Local<Value> init_v = fn_obj->Get(
+ env->context(),
+ FIXED_ONE_BYTE_STRING(env->isolate(), "init")).ToLocalChecked();
+ Local<Value> pre_v = fn_obj->Get(
+ env->context(),
+ FIXED_ONE_BYTE_STRING(env->isolate(), "pre")).ToLocalChecked();
+ Local<Value> post_v = fn_obj->Get(
+ env->context(),
+ FIXED_ONE_BYTE_STRING(env->isolate(), "post")).ToLocalChecked();
+ Local<Value> destroy_v = fn_obj->Get(
+ env->context(),
+ FIXED_ONE_BYTE_STRING(env->isolate(), "destroy")).ToLocalChecked();
+
+ if (!init_v->IsFunction())
return env->ThrowTypeError("init callback must be a function");
- env->set_async_hooks_init_function(args[0].As<Function>());
+ env->set_async_hooks_init_function(init_v.As<Function>());
- if (args[1]->IsFunction())
- env->set_async_hooks_pre_function(args[1].As<Function>());
- if (args[2]->IsFunction())
- env->set_async_hooks_post_function(args[2].As<Function>());
- if (args[3]->IsFunction())
- env->set_async_hooks_destroy_function(args[3].As<Function>());
+ if (pre_v->IsFunction())
+ env->set_async_hooks_pre_function(pre_v.As<Function>());
+ if (post_v->IsFunction())
+ env->set_async_hooks_post_function(post_v.As<Function>());
+ if (destroy_v->IsFunction())
+ env->set_async_hooks_destroy_function(destroy_v.As<Function>());
}
diff --git a/test/parallel/test-async-wrap-check-providers.js b/test/parallel/test-async-wrap-check-providers.js
index 3a2c80d9cbe173..5ae8654d337303 100644
--- a/test/parallel/test-async-wrap-check-providers.js
+++ b/test/parallel/test-async-wrap-check-providers.js
@@ -36,7 +36,7 @@ function init(id, provider) {
function noop() { }
-async_wrap.setupHooks(init, noop, noop);
+async_wrap.setupHooks({ init });
async_wrap.enable();
diff --git a/test/parallel/test-async-wrap-disabled-propagate-parent.js b/test/parallel/test-async-wrap-disabled-propagate-parent.js
index 152af4bcde80fc..f0daaeb06d0031 100644
--- a/test/parallel/test-async-wrap-disabled-propagate-parent.js
+++ b/test/parallel/test-async-wrap-disabled-propagate-parent.js
@@ -30,7 +30,7 @@ function init(uid, type, parentUid, parentHandle) {
function noop() { }
-async_wrap.setupHooks(init, noop, noop);
+async_wrap.setupHooks({ init });
async_wrap.enable();
const server = net.createServer(function(c) {
diff --git a/test/parallel/test-async-wrap-propagate-parent.js b/test/parallel/test-async-wrap-propagate-parent.js
index e6cb5fa77bdaf7..34a00c22eeeafd 100644
--- a/test/parallel/test-async-wrap-propagate-parent.js
+++ b/test/parallel/test-async-wrap-propagate-parent.js
@@ -30,7 +30,7 @@ function init(uid, type, parentUid, parentHandle) {
function noop() { }
-async_wrap.setupHooks(init, noop, noop);
+async_wrap.setupHooks({ init });
async_wrap.enable();
const server = net.createServer(function(c) {
diff --git a/test/parallel/test-async-wrap-throw-no-init.js b/test/parallel/test-async-wrap-throw-no-init.js
index 768e38e8eff389..ccf77f66dc7232 100644
--- a/test/parallel/test-async-wrap-throw-no-init.js
+++ b/test/parallel/test-async-wrap-throw-no-init.js
@@ -7,14 +7,14 @@ const async_wrap = process.binding('async_wrap');
assert.throws(function() {
async_wrap.setupHooks(null);
-}, /init callback must be a function/);
+}, /first argument must be an object/);
assert.throws(function() {
async_wrap.enable();
}, /init callback is not assigned to a function/);
// Should not throw
-async_wrap.setupHooks(() => {});
+async_wrap.setupHooks({ init: () => {} });
async_wrap.enable();
assert.throws(function() {
diff --git a/test/parallel/test-async-wrap-uid.js b/test/parallel/test-async-wrap-uid.js
index 4e664f1bd46ebb..5bf3a8856e0e3f 100644
--- a/test/parallel/test-async-wrap-uid.js
+++ b/test/parallel/test-async-wrap-uid.js
@@ -6,7 +6,7 @@ const assert = require('assert');
const async_wrap = process.binding('async_wrap');
const storage = new Map();
-async_wrap.setupHooks(init, pre, post, destroy);
+async_wrap.setupHooks({ init, pre, post, destroy });
async_wrap.enable();
function init(uid) {
From 61dd47f9135b3b8d31bc0eb3ff7900adddb0217a Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Mon, 22 Feb 2016 22:50:56 -0700
Subject: [PATCH 16/18] async_wrap: notify post if intercepted exception
The second argument of the post callback is a boolean indicating whether
the callback threw and was intercepted by uncaughtException or a domain.
Currently node::MakeCallback has no way of retrieving a uid for the
object. This is coming in a future patch.
PR-URL: https://github.com/nodejs/node/pull/5756
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
---
src/async-wrap.cc | 5 ++-
src/node.cc | 7 +++-
.../test-async-wrap-post-did-throw.js | 34 +++++++++++++++++++
3 files changed, 44 insertions(+), 2 deletions(-)
create mode 100644 test/parallel/test-async-wrap-post-did-throw.js
diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index 570ed2f165b2ba..7338f051cd4aee 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -9,6 +9,7 @@
#include "v8-profiler.h"
using v8::Array;
+using v8::Boolean;
using v8::Context;
using v8::Function;
using v8::FunctionCallbackInfo;
@@ -231,7 +232,9 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
Local<Value> ret = cb->Call(context, argc, argv);
if (ran_init_callback() && !post_fn.IsEmpty()) {
- if (post_fn->Call(context, 1, &uid).IsEmpty())
+ Local<Value> did_throw = Boolean::New(env()->isolate(), ret.IsEmpty());
+ Local<Value> vals[] = { uid, did_throw };
+ if (post_fn->Call(context, arraysize(vals), vals).IsEmpty())
FatalError("node::AsyncWrap::MakeCallback", "post hook threw");
}
diff --git a/src/node.cc b/src/node.cc
index 074589542f59ba..0281f31d2bf47b 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1172,7 +1172,12 @@ Local<Value> MakeCallback(Environment* env,
Local<Value> ret = callback->Call(recv, argc, argv);
if (ran_init_callback && !post_fn.IsEmpty()) {
- if (post_fn->Call(object, 0, nullptr).IsEmpty())
+ Local<Value> did_throw = Boolean::New(env->isolate(), ret.IsEmpty());
+ // Currently there's no way to retrieve an uid from node::MakeCallback().
+ // This needs to be fixed.
+ Local<Value> vals[] =
+ { Undefined(env->isolate()).As<Value>(), did_throw };
+ if (post_fn->Call(object, arraysize(vals), vals).IsEmpty())
FatalError("node::MakeCallback", "post hook threw");
}
diff --git a/test/parallel/test-async-wrap-post-did-throw.js b/test/parallel/test-async-wrap-post-did-throw.js
new file mode 100644
index 00000000000000..9781983f58987e
--- /dev/null
+++ b/test/parallel/test-async-wrap-post-did-throw.js
@@ -0,0 +1,34 @@
+'use strict';
+
+require('../common');
+const assert = require('assert');
+const async_wrap = process.binding('async_wrap');
+var asyncThrows = 0;
+var uncaughtExceptionCount = 0;
+
+process.on('uncaughtException', (e) => {
+ assert.equal(e.message, 'oh noes!', 'error messages do not match');
+});
+
+process.on('exit', () => {
+ process.removeAllListeners('uncaughtException');
+ assert.equal(uncaughtExceptionCount, 1);
+ assert.equal(uncaughtExceptionCount, asyncThrows);
+});
+
+function init() { }
+function post(id, threw) {
+ if (threw)
+ uncaughtExceptionCount++;
+}
+
+async_wrap.setupHooks({ init, post });
+async_wrap.enable();
+
+// Timers still aren't supported, so use crypto API.
+// It's also important that the callback not happen in a nextTick, like many
+// error events in core.
+require('crypto').randomBytes(0, () => {
+ asyncThrows++;
+ throw new Error('oh noes!');
+});
From 8b1618a02254cff90571eba2cdad9f7c440844d9 Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Mon, 14 Mar 2016 12:35:22 -0600
Subject: [PATCH 17/18] async_wrap: don't abort on callback exception
Rather than abort if the init/pre/post/final/destroy callbacks throw,
force the exception to propagate and not be made catchable. This way
the application is still not allowed to proceed but also allowed the
location of the failure to print before exiting. Though the stack itself
may not be of much use since all callbacks except init are called from
the bottom of the call stack.
/tmp/async-test.js:14
throw new Error('pre');
^
Error: pre
at InternalFieldObject.pre (/tmp/async-test.js:14:9)
PR-URL: https://github.com/nodejs/node/pull/5756
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
---
src/async-wrap-inl.h | 15 ++--
src/async-wrap.cc | 20 ++++--
src/node.cc | 39 +++++++++--
src/node_internals.h | 5 ++
.../test-async-wrap-throw-from-callback.js | 68 +++++++++++++++++++
5 files changed, 135 insertions(+), 12 deletions(-)
create mode 100644 test/parallel/test-async-wrap-throw-from-callback.js
diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h
index eeea5973cac57b..cf7024e7e31461 100644
--- a/src/async-wrap-inl.h
+++ b/src/async-wrap-inl.h
@@ -51,11 +51,15 @@ inline AsyncWrap::AsyncWrap(Environment* env,
argv[3] = parent->object();
}
+ v8::TryCatch try_catch(env->isolate());
+
v8::MaybeLocal<v8::Value> ret =
init_fn->Call(env->context(), object, arraysize(argv), argv);
- if (ret.IsEmpty())
- FatalError("node::AsyncWrap::AsyncWrap", "init hook threw");
+ if (ret.IsEmpty()) {
+ ClearFatalExceptionHandlers(env);
+ FatalException(env->isolate(), try_catch);
+ }
bits_ |= 1; // ran_init_callback() is true now.
}
@@ -69,10 +73,13 @@ inline AsyncWrap::~AsyncWrap() {
if (!fn.IsEmpty()) {
v8::HandleScope scope(env()->isolate());
v8::Local<v8::Value> uid = v8::Integer::New(env()->isolate(), get_uid());
+ v8::TryCatch try_catch(env()->isolate());
v8::MaybeLocal<v8::Value> ret =
fn->Call(env()->context(), v8::Null(env()->isolate()), 1, &uid);
- if (ret.IsEmpty())
- FatalError("node::AsyncWrap::~AsyncWrap", "destroy hook threw");
+ if (ret.IsEmpty()) {
+ ClearFatalExceptionHandlers(env());
+ FatalException(env()->isolate(), try_catch);
+ }
}
}
diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index 7338f051cd4aee..8129500a922d97 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -18,6 +18,7 @@ using v8::HeapProfiler;
using v8::Integer;
using v8::Isolate;
using v8::Local;
+using v8::MaybeLocal;
using v8::Object;
using v8::RetainedObjectInfo;
using v8::TryCatch;
@@ -225,8 +226,13 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
}
if (ran_init_callback() && !pre_fn.IsEmpty()) {
- if (pre_fn->Call(context, 1, &uid).IsEmpty())
- FatalError("node::AsyncWrap::MakeCallback", "pre hook threw");
+ TryCatch try_catch(env()->isolate());
+ MaybeLocal<Value> ar = pre_fn->Call(env()->context(), context, 1, &uid);
+ if (ar.IsEmpty()) {
+ ClearFatalExceptionHandlers(env());
+ FatalException(env()->isolate(), try_catch);
+ return Local<Value>();
+ }
}
Local<Value> ret = cb->Call(context, argc, argv);
@@ -234,8 +240,14 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
if (ran_init_callback() && !post_fn.IsEmpty()) {
Local<Value> did_throw = Boolean::New(env()->isolate(), ret.IsEmpty());
Local<Value> vals[] = { uid, did_throw };
- if (post_fn->Call(context, arraysize(vals), vals).IsEmpty())
- FatalError("node::AsyncWrap::MakeCallback", "post hook threw");
+ TryCatch try_catch(env()->isolate());
+ MaybeLocal<Value> ar =
+ post_fn->Call(env()->context(), context, arraysize(vals), vals);
+ if (ar.IsEmpty()) {
+ ClearFatalExceptionHandlers(env());
+ FatalException(env()->isolate(), try_catch);
+ return Local<Value>();
+ }
}
if (ret.IsEmpty()) {
diff --git a/src/node.cc b/src/node.cc
index 0281f31d2bf47b..2f08a1da378eaf 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -115,6 +115,7 @@ using v8::Integer;
using v8::Isolate;
using v8::Local;
using v8::Locker;
+using v8::MaybeLocal;
using v8::Message;
using v8::Number;
using v8::Object;
@@ -1165,8 +1166,13 @@ Local<Value> MakeCallback(Environment* env,
}
if (ran_init_callback && !pre_fn.IsEmpty()) {
- if (pre_fn->Call(object, 0, nullptr).IsEmpty())
- FatalError("node::MakeCallback", "pre hook threw");
+ TryCatch try_catch(env->isolate());
+ MaybeLocal<Value> ar = pre_fn->Call(env->context(), object, 0, nullptr);
+ if (ar.IsEmpty()) {
+ ClearFatalExceptionHandlers(env);
+ FatalException(env->isolate(), try_catch);
+ return Local<Value>();
+ }
}
Local<Value> ret = callback->Call(recv, argc, argv);
@@ -1177,8 +1183,14 @@ Local<Value> MakeCallback(Environment* env,
// This needs to be fixed.
Local<Value> vals[] =
{ Undefined(env->isolate()).As<Value>(), did_throw };
- if (post_fn->Call(object, arraysize(vals), vals).IsEmpty())
- FatalError("node::MakeCallback", "post hook threw");
+ TryCatch try_catch(env->isolate());
+ MaybeLocal<Value> ar =
+ post_fn->Call(env->context(), object, arraysize(vals), vals);
+ if (ar.IsEmpty()) {
+ ClearFatalExceptionHandlers(env);
+ FatalException(env->isolate(), try_catch);
+ return Local<Value>();
+ }
}
if (ret.IsEmpty()) {
@@ -2352,6 +2364,25 @@ void OnMessage(Local<Message> message, Local<Value> error) {
}
+void ClearFatalExceptionHandlers(Environment* env) {
+ Local<Object> process = env->process_object();
+ Local<Value> events =
+ process->Get(env->context(), env->events_string()).ToLocalChecked();
+
+ if (events->IsObject()) {
+ events.As<Object>()->Set(
+ env->context(),
+ OneByteString(env->isolate(), "uncaughtException"),
+ Undefined(env->isolate())).FromJust();
+ }
+
+ process->Set(
+ env->context(),
+ env->domain_string(),
+ Undefined(env->isolate())).FromJust();
+}
+
+
static void Binding(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
diff --git a/src/node_internals.h b/src/node_internals.h
index 6e3f7204777835..33ae25d6062938 100644
--- a/src/node_internals.h
+++ b/src/node_internals.h
@@ -237,6 +237,11 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
Environment* env_;
};
+// Clear any domain and/or uncaughtException handlers to force the error's
+// propagation and shutdown the process. Use this to force the process to exit
+// by clearing all callbacks that could handle the error.
+void ClearFatalExceptionHandlers(Environment* env);
+
enum NodeInstanceType { MAIN, WORKER };
class NodeInstanceData {
diff --git a/test/parallel/test-async-wrap-throw-from-callback.js b/test/parallel/test-async-wrap-throw-from-callback.js
new file mode 100644
index 00000000000000..bfbe32c38b021a
--- /dev/null
+++ b/test/parallel/test-async-wrap-throw-from-callback.js
@@ -0,0 +1,68 @@
+'use strict';
+
+require('../common');
+const async_wrap = process.binding('async_wrap');
+const assert = require('assert');
+const crypto = require('crypto');
+const domain = require('domain');
+const spawn = require('child_process').spawn;
+const callbacks = [ 'init', 'pre', 'post', 'destroy' ];
+const toCall = process.argv[2];
+var msgCalled = 0;
+var msgReceived = 0;
+
+function init() {
+ if (toCall === 'init')
+ throw new Error('init');
+}
+function pre() {
+ if (toCall === 'pre')
+ throw new Error('pre');
+}
+function post() {
+ if (toCall === 'post')
+ throw new Error('post');
+}
+function destroy() {
+ if (toCall === 'destroy')
+ throw new Error('destroy');
+}
+
+if (typeof process.argv[2] === 'string') {
+ async_wrap.setupHooks({ init, pre, post, destroy });
+ async_wrap.enable();
+
+ process.on('uncaughtException', () => assert.ok(0, 'UNREACHABLE'));
+
+ const d = domain.create();
+ d.on('error', () => assert.ok(0, 'UNREACHABLE'));
+ d.run(() => {
+ // Using randomBytes because timers are not yet supported.
+ crypto.randomBytes(0, () => { });
+ });
+
+} else {
+
+ process.on('exit', (code) => {
+ assert.equal(msgCalled, callbacks.length);
+ assert.equal(msgCalled, msgReceived);
+ });
+
+ callbacks.forEach((item) => {
+ msgCalled++;
+
+ const child = spawn(process.execPath, [__filename, item]);
+ var errstring = '';
+
+ child.stderr.on('data', (data) => {
+ errstring += data.toString();
+ });
+
+ child.on('close', (code) => {
+ if (errstring.includes('Error: ' + item))
+ msgReceived++;
+
+ assert.equal(code, 1, `${item} closed with code ${code}`);
+ });
+ });
+}
From 73c0b55d8e23dabfc2842ddb6138ef59912076a9 Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Wed, 1 Jun 2016 12:33:33 -0600
Subject: [PATCH 18/18] async_wrap: pass uid to JS as double
Passing the uid via v8::Integer::New() converts it to a uint32_t. Which
will trim the value early. Instead use v8::Number::New() to convert the
int64_t to a double so that JS can see the full 2^53 range of uid's.
PR-URL: https://github.com/nodejs/node/pull/7096
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
---
src/async-wrap-inl.h | 6 +++---
src/async-wrap.cc | 3 ++-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h
index cf7024e7e31461..9f520de4429622 100644
--- a/src/async-wrap-inl.h
+++ b/src/async-wrap-inl.h
@@ -40,14 +40,14 @@ inline AsyncWrap::AsyncWrap(Environment* env,
v8::HandleScope scope(env->isolate());
v8::Local<v8::Value> argv[] = {
- v8::Integer::New(env->isolate(), get_uid()),
+ v8::Number::New(env->isolate(), get_uid()),
v8::Int32::New(env->isolate(), provider),
Null(env->isolate()),
Null(env->isolate())
};
if (parent != nullptr) {
- argv[2] = v8::Integer::New(env->isolate(), parent->get_uid());
+ argv[2] = v8::Number::New(env->isolate(), parent->get_uid());
argv[3] = parent->object();
}
@@ -72,7 +72,7 @@ inline AsyncWrap::~AsyncWrap() {
v8::Local<v8::Function> fn = env()->async_hooks_destroy_function();
if (!fn.IsEmpty()) {
v8::HandleScope scope(env()->isolate());
- v8::Local<v8::Value> uid = v8::Integer::New(env()->isolate(), get_uid());
+ v8::Local<v8::Value> uid = v8::Number::New(env()->isolate(), get_uid());
v8::TryCatch try_catch(env()->isolate());
v8::MaybeLocal<v8::Value> ret =
fn->Call(env()->context(), v8::Null(env()->isolate()), 1, &uid);
diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index 8129500a922d97..2b6839c05ee461 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -19,6 +19,7 @@ using v8::Integer;
using v8::Isolate;
using v8::Local;
using v8::MaybeLocal;
+using v8::Number;
using v8::Object;
using v8::RetainedObjectInfo;
using v8::TryCatch;
@@ -198,7 +199,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
Local<Function> pre_fn = env()->async_hooks_pre_function();
Local<Function> post_fn = env()->async_hooks_post_function();
- Local<Value> uid = Integer::New(env()->isolate(), get_uid());
+ Local<Value> uid = Number::New(env()->isolate(), get_uid());
Local<Object> context = object();
Local<Object> domain;
bool has_domain = false;