From 022d3324e2c4f0bcd487a0adf0f6e31f5d229f7a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 10 Sep 2017 05:06:34 +0200 Subject: [PATCH 1/9] async_wrap: update utility methods for flexibility Original PR: https://github.com/ayojs/ayo/pull/82 > This makes some of the internal `AsyncWrap::MakeCallback()` > utility wrappers throw rather than crash the process when > the requested method is not present on the `this` object. > Doing so makes it easier for future code to expose C++ > objects directly to userland, where JS code can overwrite > or delete such methods. > PR-URL: https://github.com/ayojs/ayo/pull/82 > Reviewed-By: Stephen Belanger --- src/async-wrap-inl.h | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h index dd947dbd446cac..2d10a6d5f75fa5 100644 --- a/src/async-wrap-inl.h +++ b/src/async-wrap-inl.h @@ -50,8 +50,13 @@ inline v8::MaybeLocal AsyncWrap::MakeCallback( const v8::Local symbol, int argc, v8::Local* argv) { - v8::Local cb_v = object()->Get(symbol); - CHECK(cb_v->IsFunction()); + v8::Local cb_v; + if (!object()->Get(object()->CreationContext(), symbol).ToLocal(&cb_v)) + return v8::MaybeLocal(); + if (!cb_v->IsFunction()) { + env()->ThrowError("callback must be a function"); + return v8::MaybeLocal(); + } return MakeCallback(cb_v.As(), argc, argv); } @@ -60,8 +65,13 @@ inline v8::MaybeLocal AsyncWrap::MakeCallback( uint32_t index, int argc, v8::Local* argv) { - v8::Local cb_v = object()->Get(index); - CHECK(cb_v->IsFunction()); + v8::Local cb_v; + if (!object()->Get(object()->CreationContext(), index).ToLocal(&cb_v)) + return v8::MaybeLocal(); + if (!cb_v->IsFunction()) { + env()->ThrowError("callback must be a function"); + return v8::MaybeLocal(); + } return MakeCallback(cb_v.As(), argc, argv); } From a4e8e9e4354f55b29e23323b3f22abd7184a810c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 20 Sep 2017 14:55:57 +0200 Subject: [PATCH 2/9] src: fix `fs_stats_field_array` memory leak Original PR: https://github.com/ayojs/ayo/pull/82 > Previously, the `Environment` destructor did not release its > `fs_stats_field_array` memory. The memory is allocated when the > `fs` module is initialized and calls `GetStatValues()`. > PR-URL: https://github.com/ayojs/ayo/pull/82 > Reviewed-By: Stephen Belanger --- src/env-inl.h | 1 + src/env.h | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/env-inl.h b/src/env-inl.h index 0297e6697714b4..fe77f72d3e4ff5 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -334,6 +334,7 @@ inline Environment::~Environment() { delete[] heap_space_statistics_buffer_; delete[] http_parser_buffer_; delete http2_state_; + delete[] fs_stats_field_array_; free(performance_state_); } diff --git a/src/env.h b/src/env.h index 786f87c4374f05..7ee61dde379f8a 100644 --- a/src/env.h +++ b/src/env.h @@ -723,7 +723,7 @@ class Environment { char* http_parser_buffer_; http2::http2_state* http2_state_ = nullptr; - double* fs_stats_field_array_; + double* fs_stats_field_array_ = nullptr; struct AtExitCallback { void (*cb_)(void* arg); From 53d646588fd83286b3d3ff047dbcb12cf734be0d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 5 Sep 2017 14:09:39 +0200 Subject: [PATCH 3/9] src: protect global state with mutexes Original PR: https://github.com/ayojs/ayo/pull/82 > PR-URL: https://github.com/ayojs/ayo/pull/82 > Reviewed-By: Stephen Belanger --- node.gyp | 2 -- src/node.cc | 13 +++++++++++++ src/node_crypto.cc | 3 +++ src/node_http2.cc | 2 +- src/node_http2_core.h | 2 +- src/string_search.cc | 11 ----------- src/string_search.h | 6 +++--- 7 files changed, 21 insertions(+), 18 deletions(-) delete mode 100644 src/string_search.cc diff --git a/node.gyp b/node.gyp index 338b7f324e4f6a..1dec5acbec16d9 100644 --- a/node.gyp +++ b/node.gyp @@ -223,7 +223,6 @@ 'src/signal_wrap.cc', 'src/spawn_sync.cc', 'src/string_bytes.cc', - 'src/string_search.cc', 'src/stream_base.cc', 'src/stream_wrap.cc', 'src/tcp_wrap.cc', @@ -685,7 +684,6 @@ '<(OBJ_PATH)<(OBJ_SEPARATOR)node_url.<(OBJ_SUFFIX)', '<(OBJ_PATH)<(OBJ_SEPARATOR)util.<(OBJ_SUFFIX)', '<(OBJ_PATH)<(OBJ_SEPARATOR)string_bytes.<(OBJ_SUFFIX)', - '<(OBJ_PATH)<(OBJ_SEPARATOR)string_search.<(OBJ_SUFFIX)', '<(OBJ_PATH)<(OBJ_SEPARATOR)stream_base.<(OBJ_SUFFIX)', '<(OBJ_PATH)<(OBJ_SEPARATOR)node_constants.<(OBJ_SUFFIX)', '<(OBJ_TRACING_PATH)<(OBJ_SEPARATOR)agent.<(OBJ_SUFFIX)', diff --git a/src/node.cc b/src/node.cc index 0c69ece87ad366..64a90942915c8f 100644 --- a/src/node.cc +++ b/src/node.cc @@ -167,6 +167,9 @@ using v8::Value; using AsyncHooks = node::Environment::AsyncHooks; +static Mutex process_mutex; +static Mutex environ_mutex; + static bool print_eval = false; static bool force_repl = false; static bool syntax_check_only = false; @@ -1789,6 +1792,7 @@ void AppendExceptionLine(Environment* env, if (!can_set_arrow || (mode == FATAL_ERROR && !err_obj->IsNativeError())) { if (env->printed_error()) return; + Mutex::ScopedLock lock(process_mutex); env->set_printed_error(true); uv_tty_reset_mode(); @@ -2952,6 +2956,7 @@ static void LinkedBinding(const FunctionCallbackInfo& args) { static void ProcessTitleGetter(Local property, const PropertyCallbackInfo& info) { + Mutex::ScopedLock lock(process_mutex); char buffer[512]; uv_get_process_title(buffer, sizeof(buffer)); info.GetReturnValue().Set(String::NewFromUtf8(info.GetIsolate(), buffer)); @@ -2961,6 +2966,7 @@ static void ProcessTitleGetter(Local property, static void ProcessTitleSetter(Local property, Local value, const PropertyCallbackInfo& info) { + Mutex::ScopedLock lock(process_mutex); node::Utf8Value title(info.GetIsolate(), value); // TODO(piscisaureus): protect with a lock uv_set_process_title(*title); @@ -2969,6 +2975,7 @@ static void ProcessTitleSetter(Local property, static void EnvGetter(Local property, const PropertyCallbackInfo& info) { + Mutex::ScopedLock lock(environ_mutex); Isolate* isolate = info.GetIsolate(); if (property->IsSymbol()) { return info.GetReturnValue().SetUndefined(); @@ -3001,6 +3008,7 @@ static void EnvGetter(Local property, static void EnvSetter(Local property, Local value, const PropertyCallbackInfo& info) { + Mutex::ScopedLock lock(environ_mutex); #ifdef __POSIX__ node::Utf8Value key(info.GetIsolate(), property); node::Utf8Value val(info.GetIsolate(), value); @@ -3021,6 +3029,7 @@ static void EnvSetter(Local property, static void EnvQuery(Local property, const PropertyCallbackInfo& info) { + Mutex::ScopedLock lock(environ_mutex); int32_t rc = -1; // Not found unless proven otherwise. if (property->IsString()) { #ifdef __POSIX__ @@ -3049,6 +3058,7 @@ static void EnvQuery(Local property, static void EnvDeleter(Local property, const PropertyCallbackInfo& info) { + Mutex::ScopedLock lock(environ_mutex); if (property->IsString()) { #ifdef __POSIX__ node::Utf8Value key(info.GetIsolate(), property); @@ -3067,6 +3077,7 @@ static void EnvDeleter(Local property, static void EnvEnumerator(const PropertyCallbackInfo& info) { + Mutex::ScopedLock lock(environ_mutex); Environment* env = Environment::GetCurrent(info); Isolate* isolate = env->isolate(); Local ctx = env->context(); @@ -3190,6 +3201,7 @@ static Local GetFeatures(Environment* env) { static void DebugPortGetter(Local property, const PropertyCallbackInfo& info) { + Mutex::ScopedLock lock(process_mutex); int port = debug_options.port(); #if HAVE_INSPECTOR if (port == 0) { @@ -3205,6 +3217,7 @@ static void DebugPortGetter(Local property, static void DebugPortSetter(Local property, Local value, const PropertyCallbackInfo& info) { + Mutex::ScopedLock lock(process_mutex); debug_options.set_port(value->Int32Value()); } diff --git a/src/node_crypto.cc b/src/node_crypto.cc index a1d601adf4b7cf..d161a8670683f2 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -788,6 +788,9 @@ static int X509_up_ref(X509* cert) { static X509_STORE* NewRootCertStore() { static std::vector root_certs_vector; + static Mutex root_certs_vector_mutex; + Mutex::ScopedLock lock(root_certs_vector_mutex); + if (root_certs_vector.empty()) { for (size_t i = 0; i < arraysize(root_certs); i++) { BIO* bp = NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i])); diff --git a/src/node_http2.cc b/src/node_http2.cc index c1e178f7175935..6f726a93390b55 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -28,7 +28,7 @@ Freelist header_free_list; Freelist data_chunks_free_list; -Nghttp2Session::Callbacks Nghttp2Session::callback_struct_saved[2] = { +const Nghttp2Session::Callbacks Nghttp2Session::callback_struct_saved[2] = { Callbacks(false), Callbacks(true)}; diff --git a/src/node_http2_core.h b/src/node_http2_core.h index 69b9891d5871cb..8def84cc18f184 100644 --- a/src/node_http2_core.h +++ b/src/node_http2_core.h @@ -279,7 +279,7 @@ class Nghttp2Session { }; /* Use callback_struct_saved[kHasGetPaddingCallback ? 1 : 0] */ - static Callbacks callback_struct_saved[2]; + static const Callbacks callback_struct_saved[2]; nghttp2_session* session_; uv_loop_t* loop_; diff --git a/src/string_search.cc b/src/string_search.cc deleted file mode 100644 index 326fba7c4abf05..00000000000000 --- a/src/string_search.cc +++ /dev/null @@ -1,11 +0,0 @@ -#include "string_search.h" - -namespace node { -namespace stringsearch { - -int StringSearchBase::kBadCharShiftTable[kUC16AlphabetSize]; -int StringSearchBase::kGoodSuffixShiftTable[kBMMaxShift + 1]; -int StringSearchBase::kSuffixTable[kBMMaxShift + 1]; - -} // namespace stringsearch -} // namespace node diff --git a/src/string_search.h b/src/string_search.h index 73e90f5873f767..f2fed07e2e48d6 100644 --- a/src/string_search.h +++ b/src/string_search.h @@ -85,12 +85,12 @@ class StringSearchBase { static const int kBMMinPatternLength = 8; // Store for the BoyerMoore(Horspool) bad char shift table. - static int kBadCharShiftTable[kUC16AlphabetSize]; + int kBadCharShiftTable[kUC16AlphabetSize]; // Store for the BoyerMoore good suffix shift table. - static int kGoodSuffixShiftTable[kBMMaxShift + 1]; + int kGoodSuffixShiftTable[kBMMaxShift + 1]; // Table used temporarily while building the BoyerMoore good suffix // shift table. - static int kSuffixTable[kBMMaxShift + 1]; + int kSuffixTable[kBMMaxShift + 1]; }; template From 5cfcba2bf6b6a0b14dc9f366eeda9055c23bdd57 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 9 Sep 2017 22:28:02 +0200 Subject: [PATCH 4/9] src: add environment cleanup hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original PR: https://github.com/ayojs/ayo/pull/82 > This adds pairs of methods to the `Environment` class and to public APIs > which can add and remove cleanup handlers. > Unlike `AtExit`, this API targets addon developers rather than > embedders, giving them (and Node’s internals) the ability to register > per-`Environment` cleanup work. > PR-URL: https://github.com/ayojs/ayo/pull/82 > Reviewed-By: Stephen Belanger --- src/env-inl.h | 20 ++++++++++++++++++++ src/env.cc | 20 ++++++++++++++++++++ src/env.h | 13 +++++++++++++ src/node.cc | 18 ++++++++++++++++++ src/node.h | 13 +++++++++++++ src/node_api.cc | 22 ++++++++++++++++++++++ src/node_api.h | 7 +++++++ 7 files changed, 113 insertions(+) diff --git a/src/env-inl.h b/src/env-inl.h index fe77f72d3e4ff5..dbb0c6ba5e8d56 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -599,6 +599,26 @@ inline void Environment::SetTemplateMethod(v8::Local that, t->SetClassName(name_string); // NODE_SET_METHOD() compatibility. } +void Environment::AddCleanupHook(void (*fn)(void*), void* arg) { + cleanup_hooks_[arg].push_back( + CleanupHookCallback { fn, arg, cleanup_hook_counter_++ }); +} + +void Environment::RemoveCleanupHook(void (*fn)(void*), void* arg) { + auto map_it = cleanup_hooks_.find(arg); + if (map_it == cleanup_hooks_.end()) + return; + + for (auto it = map_it->second.begin(); it != map_it->second.end(); ++it) { + if (it->fun_ == fn && it->arg_ == arg) { + map_it->second.erase(it); + if (map_it->second.empty()) + cleanup_hooks_.erase(arg); + return; + } + } +} + #define VP(PropertyName, StringValue) V(v8::Private, PropertyName) #define VS(PropertyName, StringValue) V(v8::String, PropertyName) #define V(TypeName, PropertyName) \ diff --git a/src/env.cc b/src/env.cc index a0d82986c9e231..d276ebc0ecf311 100644 --- a/src/env.cc +++ b/src/env.cc @@ -166,6 +166,26 @@ void Environment::PrintSyncTrace() const { fflush(stderr); } +void Environment::RunCleanup() { + while (!cleanup_hooks_.empty()) { + std::vector callbacks; + // Concatenate all vectors in cleanup_hooks_ + for (const auto& pair : cleanup_hooks_) + callbacks.insert(callbacks.end(), pair.second.begin(), pair.second.end()); + cleanup_hooks_.clear(); + std::sort(callbacks.begin(), callbacks.end(), + [](const CleanupHookCallback& a, const CleanupHookCallback& b) { + // Sort in descending order so that the last-inserted callbacks get run + // first. + return a.insertion_order_counter_ > b.insertion_order_counter_; + }); + + for (const CleanupHookCallback& cb : callbacks) { + cb.fun_(cb.arg_); + } + } +} + void Environment::RunAtExitCallbacks() { for (AtExitCallback at_exit : at_exit_functions_) { at_exit.cb_(at_exit.arg_); diff --git a/src/env.h b/src/env.h index 7ee61dde379f8a..b0cbbf38612aeb 100644 --- a/src/env.h +++ b/src/env.h @@ -680,6 +680,10 @@ class Environment { bool RemovePromiseHook(promise_hook_func fn, void* arg); bool EmitNapiWarning(); + inline void AddCleanupHook(void (*fn)(void*), void* arg); + inline void RemoveCleanupHook(void (*fn)(void*), void* arg); + void RunCleanup(); + private: inline void ThrowError(v8::Local (*fun)(v8::Local), const char* errmsg); @@ -738,6 +742,15 @@ class Environment { }; std::vector promise_hooks_; + struct CleanupHookCallback { + void (*fun_)(void*); + void* arg_; + int64_t insertion_order_counter_; + }; + + std::unordered_map> cleanup_hooks_; + int64_t cleanup_hook_counter_ = 0; + static void EnvPromiseHook(v8::PromiseHookType type, v8::Local promise, v8::Local parent); diff --git a/src/node.cc b/src/node.cc index 64a90942915c8f..6b95a96d319236 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1339,6 +1339,22 @@ void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg) { env->AddPromiseHook(fn, arg); } +void AddEnvironmentCleanupHook(v8::Isolate* isolate, + void (*fun)(void* arg), + void* arg) { + Environment* env = Environment::GetCurrent(isolate); + env->AddCleanupHook(fun, arg); +} + + +void RemoveEnvironmentCleanupHook(v8::Isolate* isolate, + void (*fun)(void* arg), + void* arg) { + Environment* env = Environment::GetCurrent(isolate); + env->RemoveCleanupHook(fun, arg); +} + + CallbackScope::CallbackScope(Isolate* isolate, Local object, async_context asyncContext) @@ -4747,6 +4763,8 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, env.set_trace_sync_io(false); const int exit_code = EmitExit(&env); + + env.RunCleanup(); RunAtExit(&env); uv_key_delete(&thread_local_env); diff --git a/src/node.h b/src/node.h index 287823b17ba3a3..1ec5dd8ee97613 100644 --- a/src/node.h +++ b/src/node.h @@ -546,6 +546,19 @@ NODE_EXTERN void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg); +/* This is a lot like node::AtExit, except that the hooks added via this + * function are run before the AtExit ones and will always be registered + * for the current Environment instance. + * These functions are safe to use in an addon supporting multiple + * threads/isolates. */ +NODE_EXTERN void AddEnvironmentCleanupHook(v8::Isolate* isolate, + void (*fun)(void* arg), + void* arg); + +NODE_EXTERN void RemoveEnvironmentCleanupHook(v8::Isolate* isolate, + void (*fun)(void* arg), + void* arg); + /* Returns the id of the current execution context. If the return value is * zero then no execution has been set. This will happen if the user handles * I/O from native code. */ diff --git a/src/node_api.cc b/src/node_api.cc index 94e4e57bb483d3..a9b631d08f3f4c 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -868,6 +868,28 @@ void napi_module_register(napi_module* mod) { node::node_module_register(nm); } +napi_status napi_add_env_cleanup_hook(napi_env env, + void (*fun)(void* arg), + void* arg) { + CHECK_ENV(env); + CHECK_ARG(env, fun); + + node::AddEnvironmentCleanupHook(env->isolate, fun, arg); + + return napi_ok; +} + +napi_status napi_remove_env_cleanup_hook(napi_env env, + void (*fun)(void* arg), + void* arg) { + CHECK_ENV(env); + CHECK_ARG(env, fun); + + node::RemoveEnvironmentCleanupHook(env->isolate, fun, arg); + + return napi_ok; +} + // Warning: Keep in-sync with napi_status enum const char* error_messages[] = {nullptr, "Invalid argument", diff --git a/src/node_api.h b/src/node_api.h index a3a07a64673366..29070c3ec85d9b 100644 --- a/src/node_api.h +++ b/src/node_api.h @@ -106,6 +106,13 @@ EXTERN_C_START NAPI_EXTERN void napi_module_register(napi_module* mod); +NAPI_EXTERN napi_status napi_add_env_cleanup_hook(napi_env env, + void (*fun)(void* arg), + void* arg); +NAPI_EXTERN napi_status napi_remove_env_cleanup_hook(napi_env env, + void (*fun)(void* arg), + void* arg); + NAPI_EXTERN napi_status napi_get_last_error_info(napi_env env, const napi_extended_error_info** result); From ab573aefba51bb8fb46a4d3d95b5e9c2a74c7ee1 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 20 Sep 2017 14:43:19 +0200 Subject: [PATCH 5/9] src: add can_call_into_js flag Original PR: https://github.com/ayojs/ayo/pull/82 > PR-URL: https://github.com/ayojs/ayo/pull/82 > Reviewed-By: Stephen Belanger --- src/async-wrap.cc | 4 ++++ src/env-inl.h | 8 ++++++++ src/env.h | 5 +++++ src/node.cc | 9 +++++++++ src/node_contextify.cc | 2 ++ 5 files changed, 28 insertions(+) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index a16d93888925c6..2a65549e962222 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -152,6 +152,7 @@ static void DestroyAsyncIdsCallback(uv_timer_t* handle) { do { std::vector destroy_async_id_list; destroy_async_id_list.swap(*env->destroy_async_id_list()); + if (!env->can_call_into_js()) return; for (auto async_id : destroy_async_id_list) { // Want each callback to be cleaned up after itself, instead of cleaning // them all up after the while() loop completes. @@ -174,6 +175,9 @@ static void PushBackDestroyAsyncId(Environment* env, double id) { if (env->async_hooks()->fields()[AsyncHooks::kDestroy] == 0) return; + if (!env->can_call_into_js()) + return; + if (env->destroy_async_id_list()->empty()) uv_timer_start(env->destroy_async_ids_timer_handle(), DestroyAsyncIdsCallback, 0, 0); diff --git a/src/env-inl.h b/src/env-inl.h index dbb0c6ba5e8d56..1142b536698c9a 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -506,6 +506,14 @@ inline void Environment::set_fs_stats_field_array(double* fields) { fs_stats_field_array_ = fields; } +inline bool Environment::can_call_into_js() const { + return can_call_into_js_; +} + +inline void Environment::set_can_call_into_js(bool can_call_into_js) { + can_call_into_js_ = can_call_into_js; +} + inline performance::performance_state* Environment::performance_state() { return performance_state_; } diff --git a/src/env.h b/src/env.h index b0cbbf38612aeb..c0ff34d3afc399 100644 --- a/src/env.h +++ b/src/env.h @@ -611,6 +611,9 @@ class Environment { inline performance::performance_state* performance_state(); inline std::map* performance_marks(); + inline bool can_call_into_js() const; + inline void set_can_call_into_js(bool can_call_into_js); + inline void ThrowError(const char* errmsg); inline void ThrowTypeError(const char* errmsg); inline void ThrowRangeError(const char* errmsg); @@ -708,6 +711,8 @@ class Environment { size_t makecallback_cntr_; std::vector destroy_async_id_list_; + bool can_call_into_js_ = true; + performance::performance_state* performance_state_ = nullptr; std::map performance_marks_; diff --git a/src/node.cc b/src/node.cc index 6b95a96d319236..d52dbcb7e1b471 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1383,6 +1383,11 @@ InternalCallbackScope::InternalCallbackScope(Environment* env, CHECK(!object.IsEmpty()); } + if (!env->can_call_into_js()) { + failed_ = true; + return; + } + HandleScope handle_scope(env->isolate()); // If you hit this assertion, you forgot to enter the v8::Context first. CHECK_EQ(env->context(), env->isolate()->GetCurrentContext()); @@ -1430,6 +1435,7 @@ void InternalCallbackScope::Close() { Environment::TickInfo* tick_info = env_->tick_info(); + if (!env_->can_call_into_js()) return; if (tick_info->length() == 0) { env_->isolate()->RunMicrotasks(); } @@ -1449,6 +1455,8 @@ void InternalCallbackScope::Close() { CHECK_EQ(env_->execution_async_id(), 0); CHECK_EQ(env_->trigger_async_id(), 0); + if (!env_->can_call_into_js()) return; + if (env_->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) { failed_ = true; } @@ -4764,6 +4772,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, const int exit_code = EmitExit(&env); + env.set_can_call_into_js(false); env.RunCleanup(); RunAtExit(&env); uv_key_delete(&thread_local_env); diff --git a/src/node_contextify.cc b/src/node_contextify.cc index c73db420f18b4e..c8830b45f30889 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -986,6 +986,8 @@ class ContextifyScript : public BaseObject { const bool break_on_sigint, const FunctionCallbackInfo& args, TryCatch* try_catch) { + if (!env->can_call_into_js()) + return false; if (!ContextifyScript::InstanceOf(env, args.Holder())) { env->ThrowTypeError( "Script methods can only be called on script instances."); From 91848f199eb34662ab73f715e401bb5ba798dd7d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 4 Sep 2017 22:31:20 +0200 Subject: [PATCH 6/9] src: use lock for c-ares library init/cleanup Original PR: https://github.com/ayojs/ayo/pull/82 > PR-URL: https://github.com/ayojs/ayo/pull/82 > Reviewed-By: Stephen Belanger --- src/cares_wrap.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index e800e0f2fee260..c26d18bd8e1fb0 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -70,6 +70,8 @@ using v8::Value; namespace { +Mutex ares_library_mutex; + inline uint16_t cares_get_16bit(const unsigned char* p) { return static_cast(p[0] << 8U) | (static_cast(p[1])); } @@ -496,6 +498,7 @@ void ChannelWrap::Setup() { int r; if (!library_inited_) { + Mutex::ScopedLock lock(ares_library_mutex); // Multiple calls to ares_library_init() increase a reference counter, // so this is a no-op except for the first call to it. r = ares_library_init(ARES_LIB_INIT_ALL); @@ -509,6 +512,7 @@ void ChannelWrap::Setup() { ARES_OPT_FLAGS | ARES_OPT_SOCK_STATE_CB); if (r != ARES_SUCCESS) { + Mutex::ScopedLock lock(ares_library_mutex); ares_library_cleanup(); return env()->ThrowError(ToErrorCodeString(r)); } @@ -526,6 +530,7 @@ void ChannelWrap::Setup() { ChannelWrap::~ChannelWrap() { if (library_inited_) { + Mutex::ScopedLock lock(ares_library_mutex); // This decreases the reference counter increased by ares_library_init(). ares_library_cleanup(); } From f254a3768c3e794fa0d20050a695007a9d3cbf42 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 4 Sep 2017 22:02:55 +0200 Subject: [PATCH 7/9] src: make CleanupHandles() tear down handles/reqs Original PR: https://github.com/ayojs/ayo/pull/85 > Previously, handles would not be closed when the current `Environment` > stopped, which is acceptable in a single-`Environment`-per-process > situation, but would otherwise create memory and file descriptor > leaks. > PR-URL: https://github.com/ayojs/ayo/pull/85 > Reviewed-By: Stephen Belanger --- node.gyp | 1 + src/cares_wrap.cc | 18 ++++++------------ src/connection_wrap.h | 2 +- src/env-inl.h | 23 +++++++++++++++++++++-- src/env.cc | 19 ++++++++++++------- src/env.h | 4 +++- src/fs_event_wrap.cc | 6 ++++-- src/handle_wrap.cc | 38 +++++++++++++++++++++++++------------- src/handle_wrap.h | 8 +++++++- src/node_perf.cc | 3 +-- src/node_stat_watcher.cc | 7 +------ src/process_wrap.cc | 2 ++ src/req-wrap-inl.h | 6 ++++++ src/req-wrap.h | 1 + src/tty_wrap.cc | 2 ++ 15 files changed, 93 insertions(+), 47 deletions(-) diff --git a/node.gyp b/node.gyp index 1dec5acbec16d9..90c4b807e22d91 100644 --- a/node.gyp +++ b/node.gyp @@ -677,6 +677,7 @@ '<(OBJ_PATH)<(OBJ_SEPARATOR)node_debug_options.<(OBJ_SUFFIX)', '<(OBJ_PATH)<(OBJ_SEPARATOR)async-wrap.<(OBJ_SUFFIX)', '<(OBJ_PATH)<(OBJ_SEPARATOR)env.<(OBJ_SUFFIX)', + '<(OBJ_PATH)<(OBJ_SEPARATOR)handle_wrap.<(OBJ_SUFFIX)', '<(OBJ_PATH)<(OBJ_SEPARATOR)node.<(OBJ_SUFFIX)', '<(OBJ_PATH)<(OBJ_SEPARATOR)node_buffer.<(OBJ_SUFFIX)', '<(OBJ_PATH)<(OBJ_SEPARATOR)node_i18n.<(OBJ_SUFFIX)', diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index c26d18bd8e1fb0..5d6911107575c1 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -273,9 +273,8 @@ void ares_poll_cb(uv_poll_t* watcher, int status, int events) { } -void ares_poll_close_cb(uv_handle_t* watcher) { - node_ares_task* task = ContainerOf(&node_ares_task::poll_watcher, - reinterpret_cast(watcher)); +void ares_poll_close_cb(uv_poll_t* watcher) { + node_ares_task* task = ContainerOf(&node_ares_task::poll_watcher, watcher); free(task); } @@ -353,8 +352,7 @@ void ares_sockstate_cb(void* data, "When an ares socket is closed we should have a handle for it"); channel->task_list()->erase(it); - uv_close(reinterpret_cast(&task->poll_watcher), - ares_poll_close_cb); + channel->env()->CloseHandle(&task->poll_watcher, ares_poll_close_cb); if (channel->task_list()->empty()) { uv_timer_stop(channel->timer_handle()); @@ -543,10 +541,7 @@ ChannelWrap::~ChannelWrap() { void ChannelWrap::CleanupTimer() { if (timer_handle_ == nullptr) return; - uv_close(reinterpret_cast(timer_handle_), - [](uv_handle_t* handle) { - delete reinterpret_cast(handle); - }); + env()->CloseHandle(timer_handle_, [](uv_timer_t* handle){ delete handle; }); timer_handle_ = nullptr; } @@ -641,8 +636,7 @@ class QueryWrap : public AsyncWrap { static_cast(this)); } - static void CaresAsyncClose(uv_handle_t* handle) { - uv_async_t* async = reinterpret_cast(handle); + static void CaresAsyncClose(uv_async_t* async) { auto data = static_cast(async->data); delete data->wrap; delete data; @@ -667,7 +661,7 @@ class QueryWrap : public AsyncWrap { free(host); } - uv_close(reinterpret_cast(handle), CaresAsyncClose); + wrap->env()->CloseHandle(handle, CaresAsyncClose); } static void Callback(void *arg, int status, int timeouts, diff --git a/src/connection_wrap.h b/src/connection_wrap.h index 99fe5697ed91fa..638f7f9b568e1e 100644 --- a/src/connection_wrap.h +++ b/src/connection_wrap.h @@ -23,7 +23,7 @@ class ConnectionWrap : public StreamWrap { ConnectionWrap(Environment* env, v8::Local object, ProviderType provider); - ~ConnectionWrap() { + virtual ~ConnectionWrap() { } UVType handle_; diff --git a/src/env-inl.h b/src/env-inl.h index 1142b536698c9a..b0dbda1a57144e 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -376,8 +376,27 @@ inline void Environment::RegisterHandleCleanup(uv_handle_t* handle, handle_cleanup_queue_.PushBack(new HandleCleanup(handle, cb, arg)); } -inline void Environment::FinishHandleCleanup(uv_handle_t* handle) { - handle_cleanup_waiting_--; +template +inline void Environment::CloseHandle(T* handle, OnCloseCallback callback) { + handle_cleanup_waiting_++; + static_assert(sizeof(T) >= sizeof(uv_handle_t), "T is a libuv handle"); + static_assert(offsetof(T, data) == offsetof(uv_handle_t, data), + "T is a libuv handle"); + static_assert(offsetof(T, close_cb) == offsetof(uv_handle_t, close_cb), + "T is a libuv handle"); + struct CloseData { + Environment* env; + OnCloseCallback callback; + void* original_data; + }; + handle->data = new CloseData { this, callback, handle->data }; + uv_close(reinterpret_cast(handle), [](uv_handle_t* handle) { + CloseData* data = static_cast(handle->data); + data->env->handle_cleanup_waiting_--; + handle->data = data->original_data; + data->callback(reinterpret_cast(handle)); + delete data; + }); } inline uv_loop_t* Environment::event_loop() const { diff --git a/src/env.cc b/src/env.cc index d276ebc0ecf311..e28c5dfb831f27 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1,6 +1,7 @@ #include "node_internals.h" #include "async-wrap.h" #include "v8-profiler.h" +#include "req-wrap-inl.h" #if defined(_MSC_VER) #define getpid GetCurrentProcessId @@ -51,11 +52,7 @@ void Environment::Start(int argc, uv_timer_init(event_loop(), destroy_async_ids_timer_handle()); auto close_and_finish = [](Environment* env, uv_handle_t* handle, void* arg) { - handle->data = env; - - uv_close(handle, [](uv_handle_t* handle) { - static_cast(handle->data)->FinishHandleCleanup(handle); - }); + env->CloseHandle(handle, [](uv_handle_t* handle) {}); }; RegisterHandleCleanup( @@ -95,13 +92,18 @@ void Environment::Start(int argc, } void Environment::CleanupHandles() { + for (auto r : req_wrap_queue_) + r->Cancel(); + + for (auto w : handle_wrap_queue_) + w->Close(); + while (HandleCleanup* hc = handle_cleanup_queue_.PopFront()) { - handle_cleanup_waiting_++; hc->cb_(this, hc->handle_, hc->arg_); delete hc; } - while (handle_cleanup_waiting_ != 0) + while (handle_cleanup_waiting_ != 0 || !handle_wrap_queue_.IsEmpty()) uv_run(event_loop(), UV_RUN_ONCE); } @@ -167,6 +169,8 @@ void Environment::PrintSyncTrace() const { } void Environment::RunCleanup() { + CleanupHandles(); + while (!cleanup_hooks_.empty()) { std::vector callbacks; // Concatenate all vectors in cleanup_hooks_ @@ -182,6 +186,7 @@ void Environment::RunCleanup() { for (const CleanupHookCallback& cb : callbacks) { cb.fun_(cb.arg_); + CleanupHandles(); } } } diff --git a/src/env.h b/src/env.h index c0ff34d3afc399..353bc0b8979c00 100644 --- a/src/env.h +++ b/src/env.h @@ -560,7 +560,9 @@ class Environment { inline void RegisterHandleCleanup(uv_handle_t* handle, HandleCleanupCb cb, void *arg); - inline void FinishHandleCleanup(uv_handle_t* handle); + + template + inline void CloseHandle(T* handle, OnCloseCallback callback); inline AsyncHooks* async_hooks(); inline DomainFlag* domain_flag(); diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc index 8ec8dd6dcfbd76..0ebd9625496dd4 100644 --- a/src/fs_event_wrap.cc +++ b/src/fs_event_wrap.cc @@ -76,11 +76,12 @@ FSEventWrap::FSEventWrap(Environment* env, Local object) : HandleWrap(env, object, reinterpret_cast(&handle_), - AsyncWrap::PROVIDER_FSEVENTWRAP) {} + AsyncWrap::PROVIDER_FSEVENTWRAP) { + MarkAsUninitialized(); +} FSEventWrap::~FSEventWrap() { - CHECK_EQ(initialized_, false); } @@ -132,6 +133,7 @@ void FSEventWrap::Start(const FunctionCallbackInfo& args) { int err = uv_fs_event_init(wrap->env()->event_loop(), &wrap->handle_); if (err == 0) { + wrap->MarkAsInitialized(); wrap->initialized_ = true; err = uv_fs_event_start(&wrap->handle_, OnEvent, *path, flags); diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index 7d0925e2fd6354..561927505b9db6 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -64,29 +64,40 @@ void HandleWrap::HasRef(const FunctionCallbackInfo& args) { void HandleWrap::Close(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - HandleWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - // Guard against uninitialized handle or double close. - if (!IsAlive(wrap)) - return; + wrap->Close(args[0]); +} - if (wrap->state_ != kInitialized) +void HandleWrap::Close(v8::Local close_callback) { + if (state_ != kInitialized) return; - CHECK_EQ(false, wrap->persistent().IsEmpty()); - uv_close(wrap->handle_, OnClose); - wrap->state_ = kClosing; + CHECK_EQ(false, persistent().IsEmpty()); + uv_close(handle_, OnClose); + state_ = kClosing; - if (args[0]->IsFunction()) { - wrap->object()->Set(env->onclose_string(), args[0]); - wrap->state_ = kClosingWithCallback; + if (!close_callback.IsEmpty() && close_callback->IsFunction()) { + object()->Set(env()->context(), env()->onclose_string(), close_callback) + .FromJust(); + state_ = kClosingWithCallback; } } +void HandleWrap::MarkAsInitialized() { + env()->handle_wrap_queue()->PushBack(this); + state_ = kInitialized; +} + + +void HandleWrap::MarkAsUninitialized() { + handle_wrap_queue_.Remove(); + state_ = kClosed; +} + + HandleWrap::HandleWrap(Environment* env, Local object, uv_handle_t* handle, @@ -102,7 +113,6 @@ HandleWrap::HandleWrap(Environment* env, HandleWrap::~HandleWrap() { - CHECK(persistent().IsEmpty()); } @@ -119,6 +129,8 @@ void HandleWrap::OnClose(uv_handle_t* handle) { const bool have_close_callback = (wrap->state_ == kClosingWithCallback); wrap->state_ = kClosed; + wrap->OnClose(); + if (have_close_callback) wrap->MakeCallback(env->onclose_string(), 0, nullptr); diff --git a/src/handle_wrap.h b/src/handle_wrap.h index f8be356e1a730c..547ed945d675df 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -70,12 +70,18 @@ class HandleWrap : public AsyncWrap { inline uv_handle_t* GetHandle() const { return handle_; } + void Close(v8::Local close_callback = v8::Local()); + protected: HandleWrap(Environment* env, v8::Local object, uv_handle_t* handle, AsyncWrap::ProviderType provider); - ~HandleWrap() override; + virtual ~HandleWrap(); + virtual void OnClose() {} + + void MarkAsInitialized(); + void MarkAsUninitialized(); private: friend class Environment; diff --git a/src/node_perf.cc b/src/node_perf.cc index f5aafbab63a781..b1af546c70e579 100644 --- a/src/node_perf.cc +++ b/src/node_perf.cc @@ -196,8 +196,7 @@ void PerformanceGCCallback(uv_async_t* handle) { cleanup: delete data; - auto closeCB = [](uv_handle_t* handle) { delete handle; }; - uv_close(reinterpret_cast(handle), closeCB); + env->CloseHandle(handle, [](uv_async_t* handle) { delete handle; }); } void MarkGarbageCollectionStart(Isolate* isolate, diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc index 4b2e4db7bc418c..2e5f793defcce1 100644 --- a/src/node_stat_watcher.cc +++ b/src/node_stat_watcher.cc @@ -60,11 +60,6 @@ void StatWatcher::Initialize(Environment* env, Local target) { } -static void Delete(uv_handle_t* handle) { - delete reinterpret_cast(handle); -} - - StatWatcher::StatWatcher(Environment* env, Local wrap) : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_STATWATCHER), watcher_(new uv_fs_poll_t) { @@ -77,7 +72,7 @@ StatWatcher::StatWatcher(Environment* env, Local wrap) StatWatcher::~StatWatcher() { Stop(); - uv_close(reinterpret_cast(watcher_), Delete); + env()->CloseHandle(watcher_, [](uv_fs_poll_t* handle) { delete handle; }); } diff --git a/src/process_wrap.cc b/src/process_wrap.cc index 7accc8c129ecbc..ff785538cfa2c2 100644 --- a/src/process_wrap.cc +++ b/src/process_wrap.cc @@ -88,6 +88,7 @@ class ProcessWrap : public HandleWrap { object, reinterpret_cast(&process_), AsyncWrap::PROVIDER_PROCESSWRAP) { + MarkAsUninitialized(); } static void ParseStdioOptions(Environment* env, @@ -225,6 +226,7 @@ class ProcessWrap : public HandleWrap { } int err = uv_spawn(env->event_loop(), &wrap->process_, &options); + wrap->MarkAsInitialized(); if (err == 0) { CHECK_EQ(wrap->process_.data, wrap); diff --git a/src/req-wrap-inl.h b/src/req-wrap-inl.h index e21fb1bdad9363..07c380fe28cf18 100644 --- a/src/req-wrap-inl.h +++ b/src/req-wrap-inl.h @@ -10,6 +10,7 @@ #include "env-inl.h" #include "util.h" #include "util-inl.h" +#include "uv.h" namespace node { @@ -38,6 +39,11 @@ void ReqWrap::Dispatched() { req_.data = this; } +template +void ReqWrap::Cancel() { + uv_cancel(reinterpret_cast(&req_)); +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/req-wrap.h b/src/req-wrap.h index 0fddae67460d6f..5fe713b6b87608 100644 --- a/src/req-wrap.h +++ b/src/req-wrap.h @@ -19,6 +19,7 @@ class ReqWrap : public AsyncWrap { inline ~ReqWrap() override; inline void Dispatched(); // Call this after the req has been dispatched. T* req() { return &req_; } + inline void Cancel(); private: friend class Environment; diff --git a/src/tty_wrap.cc b/src/tty_wrap.cc index ee793056c845d8..5e7f7dd17f3b89 100644 --- a/src/tty_wrap.cc +++ b/src/tty_wrap.cc @@ -174,6 +174,8 @@ TTYWrap::TTYWrap(Environment* env, reinterpret_cast(&handle_), AsyncWrap::PROVIDER_TTYWRAP) { *init_err = uv_tty_init(env->event_loop(), &handle_, fd, readable); + if (*init_err != 0) + MarkAsUninitialized(); } } // namespace node From 78f22380de52c112e2e4ade8317a2ab09eb81237 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 25 Sep 2017 21:16:02 +0200 Subject: [PATCH 8/9] src: unify ReqWrap libuv calling Original PR: https://github.com/ayojs/ayo/pull/85 > This allows easier tracking of whether there are active `ReqWrap`s. > PR-URL: https://github.com/ayojs/ayo/pull/85 > Reviewed-By: Stephen Belanger --- src/cares_wrap.cc | 22 ++++------ src/node_file.cc | 19 +++------ src/pipe_wrap.cc | 9 ++-- src/req-wrap-inl.h | 104 +++++++++++++++++++++++++++++++++++++++++++++ src/req-wrap.h | 13 +++++- src/tcp_wrap.cc | 18 ++++---- src/udp_wrap.cc | 13 +++--- 7 files changed, 150 insertions(+), 48 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 5d6911107575c1..a4ed032fbd0464 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -1969,13 +1969,11 @@ void GetAddrInfo(const FunctionCallbackInfo& args) { hints.ai_socktype = SOCK_STREAM; hints.ai_flags = flags; - int err = uv_getaddrinfo(env->event_loop(), - req_wrap->req(), - AfterGetAddrInfo, - *hostname, - nullptr, - &hints); - req_wrap->Dispatched(); + int err = req_wrap->Dispatch(uv_getaddrinfo, + AfterGetAddrInfo, + *hostname, + nullptr, + &hints); if (err) delete req_wrap; @@ -1999,12 +1997,10 @@ void GetNameInfo(const FunctionCallbackInfo& args) { GetNameInfoReqWrap* req_wrap = new GetNameInfoReqWrap(env, req_wrap_obj); - int err = uv_getnameinfo(env->event_loop(), - req_wrap->req(), - AfterGetNameInfo, - (struct sockaddr*)&addr, - NI_NAMEREQD); - req_wrap->Dispatched(); + int err = req_wrap->Dispatch(uv_getnameinfo, + AfterGetNameInfo, + (struct sockaddr*)&addr, + NI_NAMEREQD); if (err) delete req_wrap; diff --git a/src/node_file.cc b/src/node_file.cc index b9b3d34f346e56..aed16bf2887efc 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -355,11 +355,7 @@ class fs_req_wrap { CHECK(request->IsObject()); \ FSReqWrap* req_wrap = FSReqWrap::New(env, request.As(), \ #func, dest, encoding); \ - int err = uv_fs_ ## func(env->event_loop(), \ - req_wrap->req(), \ - __VA_ARGS__, \ - After); \ - req_wrap->Dispatched(); \ + int err = req_wrap->Dispatch(uv_fs_ ## func, __VA_ARGS__, After); \ if (err < 0) { \ uv_fs_t* uv_req = req_wrap->req(); \ uv_req->result = err; \ @@ -1123,13 +1119,12 @@ static void WriteString(const FunctionCallbackInfo& args) { FSReqWrap* req_wrap = FSReqWrap::New(env, req.As(), "write", buf, UTF8, ownership); - int err = uv_fs_write(env->event_loop(), - req_wrap->req(), - fd, - &uvbuf, - 1, - pos, - After); + int err = req_wrap->Dispatch(uv_fs_write, + fd, + &uvbuf, + 1, + pos, + After); req_wrap->Dispatched(); if (err < 0) { uv_fs_t* uv_req = req_wrap->req(); diff --git a/src/pipe_wrap.cc b/src/pipe_wrap.cc index bf1ddbdc0da717..11fd830ced410b 100644 --- a/src/pipe_wrap.cc +++ b/src/pipe_wrap.cc @@ -194,11 +194,10 @@ void PipeWrap::Connect(const FunctionCallbackInfo& args) { ConnectWrap* req_wrap = new ConnectWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_PIPECONNECTWRAP); - uv_pipe_connect(req_wrap->req(), - &wrap->handle_, - *name, - AfterConnect); - req_wrap->Dispatched(); + req_wrap->Dispatch(uv_pipe_connect, + &wrap->handle_, + *name, + AfterConnect); args.GetReturnValue().Set(0); // uv_pipe_connect() doesn't return errors. } diff --git a/src/req-wrap-inl.h b/src/req-wrap-inl.h index 07c380fe28cf18..5745ec2692914e 100644 --- a/src/req-wrap-inl.h +++ b/src/req-wrap-inl.h @@ -44,6 +44,110 @@ void ReqWrap::Cancel() { uv_cancel(reinterpret_cast(&req_)); } +/* Below is dark template magic designed to invoke libuv functions that + * initialize uv_req_t instances in a unified fashion, to allow easier + * tracking of active/inactive requests. */ + +// Invoke a generic libuv function that initializes uv_req_t instances. +// This is, unfortunately, necessary since they come in three different +// variants that can not all be invoked in the same way: +// - int uv_foo(uv_loop_t* loop, uv_req_t* request, ...); +// - int uv_foo(uv_req_t* request, ...); +// - void uv_foo(uv_req_t* request, ...); +template +struct CallLibuvFunction; + +// Detect `int uv_foo(uv_loop_t* loop, uv_req_t* request, ...);`. +template +struct CallLibuvFunction { + typedef int(*T)(uv_loop_t*, ReqT*, Args...); + template + static int Call(T fn, uv_loop_t* loop, ReqT* req, PassedArgs... args) { + return fn(loop, req, args...); + } +}; + +// Detect `int uv_foo(uv_req_t* request, ...);`. +template +struct CallLibuvFunction { + typedef int(*T)(ReqT*, Args...); + template + static int Call(T fn, uv_loop_t* loop, ReqT* req, PassedArgs... args) { + return fn(req, args...); + } +}; + +// Detect `void uv_foo(uv_req_t* request, ...);`. +template +struct CallLibuvFunction { + typedef void(*T)(ReqT*, Args...); + template + static int Call(T fn, uv_loop_t* loop, ReqT* req, PassedArgs... args) { + fn(req, args...); + return 0; + } +}; + +// This is slightly darker magic: This template is 'applied' to each parameter +// passed to the libuv function. If the parameter type (aka `T`) is a +// function type, it is assumed that this it is the request callback, and a +// wrapper that calls the original callback is created. +// If not, the parameter is passed through verbatim. +template +struct MakeLibuvRequestCallback { + static T For(ReqWrap* req_wrap, T v) { + static_assert(!std::is_function::value, + "MakeLibuvRequestCallback missed a callback"); + return v; + } +}; + +// Match the `void callback(uv_req_t*, ...);` signature that all libuv +// callbacks use. +template +struct MakeLibuvRequestCallback { + typedef void(*T)(ReqT* req, Args... args); + + static void Wrapper(ReqT* req, Args... args) { + ReqWrap* req_wrap = ContainerOf(&ReqWrap::req_, req); + T original_callback = reinterpret_cast(req_wrap->original_callback_); + original_callback(req, args...); + } + + static T For(ReqWrap* req_wrap, T v) { + CHECK_EQ(req_wrap->original_callback_, nullptr); + req_wrap->original_callback_ = + reinterpret_cast::callback_t>(v); + return Wrapper; + } +}; + +template +template +int ReqWrap::Dispatch(LibuvFunction fn, Args... args) { + Dispatched(); + + // This expands as: + // + // return fn(env()->event_loop(), req(), arg1, arg2, Wrapper, arg3, ...) + // ^ ^ ^ + // | | | + // \-- Omitted if `fn` has no | | + // first `uv_loop_t*` argument | | + // | | + // A function callback whose first argument | | + // matches the libuv request type is replaced ---/ | + // by the `Wrapper` method defined above | + // | + // Other (non-function) arguments are passed -----/ + // through verbatim + return CallLibuvFunction::Call( + fn, + env()->event_loop(), + req(), + MakeLibuvRequestCallback::For(this, args)...); +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/req-wrap.h b/src/req-wrap.h index 5fe713b6b87608..6d342bb6be70b2 100644 --- a/src/req-wrap.h +++ b/src/req-wrap.h @@ -17,14 +17,25 @@ class ReqWrap : public AsyncWrap { v8::Local object, AsyncWrap::ProviderType provider); inline ~ReqWrap() override; - inline void Dispatched(); // Call this after the req has been dispatched. + // Call this after the req has been dispatched, if that did not already + // happen by using Dispatch(). + inline void Dispatched(); T* req() { return &req_; } inline void Cancel(); + template + inline int Dispatch(LibuvFunction fn, Args... args); + private: friend class Environment; + template + friend struct MakeLibuvRequestCallback; + ListNode req_wrap_queue_; + typedef void (*callback_t)(); + callback_t original_callback_ = nullptr; + protected: // req_wrap_queue_ needs to be at a fixed offset from the start of the class // because it is used by ContainerOf to calculate the address of the embedding diff --git a/src/tcp_wrap.cc b/src/tcp_wrap.cc index 2bbc350dd5573c..7bda85f622419f 100644 --- a/src/tcp_wrap.cc +++ b/src/tcp_wrap.cc @@ -271,11 +271,10 @@ void TCPWrap::Connect(const FunctionCallbackInfo& args) { env->set_init_trigger_async_id(wrap->get_async_id()); ConnectWrap* req_wrap = new ConnectWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_TCPCONNECTWRAP); - err = uv_tcp_connect(req_wrap->req(), - &wrap->handle_, - reinterpret_cast(&addr), - AfterConnect); - req_wrap->Dispatched(); + err = req_wrap->Dispatch(uv_tcp_connect, + &wrap->handle_, + reinterpret_cast(&addr), + AfterConnect); if (err) delete req_wrap; } @@ -307,11 +306,10 @@ void TCPWrap::Connect6(const FunctionCallbackInfo& args) { env->set_init_trigger_async_id(wrap->get_async_id()); ConnectWrap* req_wrap = new ConnectWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_TCPCONNECTWRAP); - err = uv_tcp_connect(req_wrap->req(), - &wrap->handle_, - reinterpret_cast(&addr), - AfterConnect); - req_wrap->Dispatched(); + err = req_wrap->Dispatch(uv_tcp_connect, + &wrap->handle_, + reinterpret_cast(&addr), + AfterConnect); if (err) delete req_wrap; } diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index 98399155a84fef..efe6059084b0de 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -384,15 +384,14 @@ void UDPWrap::DoSend(const FunctionCallbackInfo& args, int family) { } if (err == 0) { - err = uv_udp_send(req_wrap->req(), - &wrap->handle_, - *bufs, - count, - reinterpret_cast(&addr), - OnSend); + err = req_wrap->Dispatch(uv_udp_send, + &wrap->handle_, + *bufs, + count, + reinterpret_cast(&addr), + OnSend); } - req_wrap->Dispatched(); if (err) delete req_wrap; From 7a3bb9fcb6f81ac6c97e7b73864e8a459c1af3dd Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 25 Sep 2017 22:53:17 +0200 Subject: [PATCH 9/9] src: keep track of open requests Original PR: https://github.com/ayojs/ayo/pull/85 > Workers cannot shut down while requests are open, so keep a counter > that is increased whenever libuv requests are made and decreased > whenever their callback is called. > PR-URL: https://github.com/ayojs/ayo/pull/85 > Reviewed-By: Stephen Belanger --- src/env-inl.h | 10 +++++++++- src/env.cc | 5 ++++- src/env.h | 6 +++++- src/node_api.cc | 10 ++++++---- src/node_crypto.cc | 17 +++++++++++++++-- src/node_zlib.cc | 13 ++++++++++--- src/req-wrap-inl.h | 30 +++++++++++++++++------------- 7 files changed, 66 insertions(+), 25 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index b0dbda1a57144e..bed897298a214c 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -295,7 +295,6 @@ inline Environment::Environment(IsolateData* isolate_data, #if HAVE_INSPECTOR inspector_agent_(this), #endif - handle_cleanup_waiting_(0), http_parser_buffer_(nullptr), fs_stats_field_array_(nullptr), context_(context->GetIsolate(), context) { @@ -399,6 +398,15 @@ inline void Environment::CloseHandle(T* handle, OnCloseCallback callback) { }); } +void Environment::IncreaseWaitingRequestCounter() { + request_waiting_++; +} + +void Environment::DecreaseWaitingRequestCounter() { + request_waiting_--; + CHECK_GE(request_waiting_, 0); +} + inline uv_loop_t* Environment::event_loop() const { return isolate_data()->event_loop(); } diff --git a/src/env.cc b/src/env.cc index e28c5dfb831f27..5a6a69704db50f 100644 --- a/src/env.cc +++ b/src/env.cc @@ -103,8 +103,11 @@ void Environment::CleanupHandles() { delete hc; } - while (handle_cleanup_waiting_ != 0 || !handle_wrap_queue_.IsEmpty()) + while (handle_cleanup_waiting_ != 0 || + request_waiting_ != 0 || + !handle_wrap_queue_.IsEmpty()) { uv_run(event_loop(), UV_RUN_ONCE); + } } void Environment::StartProfilerIdleNotifier() { diff --git a/src/env.h b/src/env.h index 353bc0b8979c00..2672ab7fb74040 100644 --- a/src/env.h +++ b/src/env.h @@ -564,6 +564,9 @@ class Environment { template inline void CloseHandle(T* handle, OnCloseCallback callback); + inline void IncreaseWaitingRequestCounter(); + inline void DecreaseWaitingRequestCounter(); + inline AsyncHooks* async_hooks(); inline DomainFlag* domain_flag(); inline TickInfo* tick_info(); @@ -726,7 +729,8 @@ class Environment { ReqWrapQueue req_wrap_queue_; ListHead handle_cleanup_queue_; - int handle_cleanup_waiting_; + int handle_cleanup_waiting_ = 0; + int request_waiting_ = 0; double* heap_statistics_buffer_ = nullptr; double* heap_space_statistics_buffer_ = nullptr; diff --git a/src/node_api.cc b/src/node_api.cc index a9b631d08f3f4c..20bec452e77e43 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -3397,6 +3397,9 @@ class Work : public node::AsyncResource { // Establish a handle scope here so that every callback doesn't have to. // Also it is needed for the exception-handling below. v8::HandleScope scope(env->isolate); + auto env_ = node::Environment::GetCurrent(env->isolate); + env_->DecreaseWaitingRequestCounter(); + CallbackScope callback_scope(work); work->_complete(env, ConvertUVErrorCode(status), work->_data); @@ -3485,13 +3488,12 @@ napi_status napi_queue_async_work(napi_env env, napi_async_work work) { CHECK_ARG(env, work); // Consider: Encapsulate the uv_loop_t into an opaque pointer parameter. - // Currently the environment event loop is the same as the UV default loop. - // Someday (if node ever supports multiple isolates), it may be better to get - // the loop from node::Environment::GetCurrent(env->isolate)->event_loop(); - uv_loop_t* event_loop = uv_default_loop(); + auto env_ = node::Environment::GetCurrent(env->isolate); + uv_loop_t* event_loop = env_->event_loop(); uvimpl::Work* w = reinterpret_cast(work); + env_->IncreaseWaitingRequestCounter(); CALL_UV(env, uv_queue_work(event_loop, w->Request(), uvimpl::Work::ExecuteCallback, diff --git a/src/node_crypto.cc b/src/node_crypto.cc index d161a8670683f2..fd14370fe8f628 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5322,8 +5322,13 @@ void PBKDF2Request::After() { void PBKDF2Request::After(uv_work_t* work_req, int status) { - CHECK_EQ(status, 0); PBKDF2Request* req = ContainerOf(&PBKDF2Request::work_req_, work_req); + req->env()->DecreaseWaitingRequestCounter(); + if (status == UV_ECANCELED) { + delete req; + return; + } + CHECK_EQ(status, 0); req->After(); delete req; } @@ -5430,6 +5435,7 @@ void PBKDF2(const FunctionCallbackInfo& args) { .FromJust(); } + env->IncreaseWaitingRequestCounter(); uv_queue_work(env->event_loop(), req->work_req(), PBKDF2Request::Work, @@ -5582,10 +5588,15 @@ void RandomBytesCheck(RandomBytesRequest* req, Local (*argv)[2]) { void RandomBytesAfter(uv_work_t* work_req, int status) { - CHECK_EQ(status, 0); RandomBytesRequest* req = ContainerOf(&RandomBytesRequest::work_req_, work_req); Environment* env = req->env(); + env->DecreaseWaitingRequestCounter(); + if (status == UV_ECANCELED) { + delete req; + return; + } + CHECK_EQ(status, 0); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); Local argv[2]; @@ -5639,6 +5650,7 @@ void RandomBytes(const FunctionCallbackInfo& args) { .FromJust(); } + env->IncreaseWaitingRequestCounter(); uv_queue_work(env->event_loop(), req->work_req(), RandomBytesWork, @@ -5685,6 +5697,7 @@ void RandomBytesBuffer(const FunctionCallbackInfo& args) { .FromJust(); } + env->IncreaseWaitingRequestCounter(); uv_queue_work(env->event_loop(), req->work_req(), RandomBytesWork, diff --git a/src/node_zlib.cc b/src/node_zlib.cc index fdfd314222664c..b7283c56397a4d 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -219,6 +219,7 @@ class ZCtx : public AsyncWrap { } // async version + env->IncreaseWaitingRequestCounter(); uv_queue_work(env->event_loop(), work_req, ZCtx::Process, ZCtx::After); } @@ -366,10 +367,17 @@ class ZCtx : public AsyncWrap { // v8 land! static void After(uv_work_t* work_req, int status) { - CHECK_EQ(status, 0); - ZCtx* ctx = ContainerOf(&ZCtx::work_req_, work_req); Environment* env = ctx->env(); + ctx->write_in_progress_ = false; + + env->DecreaseWaitingRequestCounter(); + if (status == UV_ECANCELED) { + ctx->Close(); + return; + } + + CHECK_EQ(status, 0); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); @@ -379,7 +387,6 @@ class ZCtx : public AsyncWrap { ctx->write_result_[0] = ctx->strm_.avail_out; ctx->write_result_[1] = ctx->strm_.avail_in; - ctx->write_in_progress_ = false; // call the write() cb Local cb = PersistentToLocal(env->isolate(), diff --git a/src/req-wrap-inl.h b/src/req-wrap-inl.h index 5745ec2692914e..c181bc4f09be30 100644 --- a/src/req-wrap-inl.h +++ b/src/req-wrap-inl.h @@ -110,6 +110,7 @@ struct MakeLibuvRequestCallback { static void Wrapper(ReqT* req, Args... args) { ReqWrap* req_wrap = ContainerOf(&ReqWrap::req_, req); + req_wrap->env()->DecreaseWaitingRequestCounter(); T original_callback = reinterpret_cast(req_wrap->original_callback_); original_callback(req, args...); } @@ -129,23 +130,26 @@ int ReqWrap::Dispatch(LibuvFunction fn, Args... args) { // This expands as: // - // return fn(env()->event_loop(), req(), arg1, arg2, Wrapper, arg3, ...) - // ^ ^ ^ - // | | | - // \-- Omitted if `fn` has no | | - // first `uv_loop_t*` argument | | - // | | - // A function callback whose first argument | | - // matches the libuv request type is replaced ---/ | - // by the `Wrapper` method defined above | - // | - // Other (non-function) arguments are passed -----/ - // through verbatim - return CallLibuvFunction::Call( + // int err = fn(env()->event_loop(), req(), arg1, arg2, Wrapper, arg3, ...) + // ^ ^ ^ + // | | | + // \-- Omitted if `fn` has no | | + // first `uv_loop_t*` argument | | + // | | + // A function callback whose first argument | | + // matches the libuv request type is replaced ---/ | + // by the `Wrapper` method defined above | + // | + // Other (non-function) arguments are passed -----/ + // through verbatim + int err = CallLibuvFunction::Call( fn, env()->event_loop(), req(), MakeLibuvRequestCallback::For(this, args)...); + if (err >= 0) + env()->IncreaseWaitingRequestCounter(); + return err; } } // namespace node