From 517e3a642552679c855595baeff46ff81720fae4 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 22 Nov 2016 17:01:02 -0700 Subject: [PATCH 1/3] async_wrap: mode constructor/destructor to .cc The constructor and destructor shouldn't have been placed in the -inl.h file from the beginning. PR-URL: https://github.com/nodejs/node/pull/9753 Reviewed-By: Anna Henningsen Reviewed-By: Ben Noordhuis --- src/async-wrap-inl.h | 71 -------------------------------------------- src/async-wrap.cc | 71 ++++++++++++++++++++++++++++++++++++++++++++ src/async-wrap.h | 10 +++---- 3 files changed, 76 insertions(+), 76 deletions(-) diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h index 85e31b1ed09d27..64b5f091612368 100644 --- a/src/async-wrap-inl.h +++ b/src/async-wrap-inl.h @@ -15,77 +15,6 @@ namespace node { -inline AsyncWrap::AsyncWrap(Environment* env, - v8::Local object, - ProviderType provider, - AsyncWrap* parent) - : BaseObject(env, object), bits_(static_cast(provider) << 1), - uid_(env->get_async_wrap_uid()) { - CHECK_NE(provider, PROVIDER_NONE); - CHECK_GE(object->InternalFieldCount(), 1); - - // Shift provider value over to prevent id collision. - persistent().SetWrapperClassId(NODE_ASYNC_ID_OFFSET + provider); - - v8::Local init_fn = env->async_hooks_init_function(); - - // No init callback exists, no reason to go on. - if (init_fn.IsEmpty()) - return; - - // If async wrap callbacks are disabled and no parent was passed that has - // run the init callback then return. - if (!env->async_wrap_callbacks_enabled() && - (parent == nullptr || !parent->ran_init_callback())) - return; - - v8::HandleScope scope(env->isolate()); - - v8::Local argv[] = { - 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::Number::New(env->isolate(), parent->get_uid()); - argv[3] = parent->object(); - } - - v8::TryCatch try_catch(env->isolate()); - - v8::MaybeLocal ret = - init_fn->Call(env->context(), object, arraysize(argv), argv); - - if (ret.IsEmpty()) { - ClearFatalExceptionHandlers(env); - FatalException(env->isolate(), try_catch); - } - - bits_ |= 1; // ran_init_callback() is true now. -} - - -inline AsyncWrap::~AsyncWrap() { - if (!ran_init_callback()) - return; - - v8::Local fn = env()->async_hooks_destroy_function(); - if (!fn.IsEmpty()) { - v8::HandleScope scope(env()->isolate()); - v8::Local uid = v8::Number::New(env()->isolate(), get_uid()); - v8::TryCatch try_catch(env()->isolate()); - v8::MaybeLocal ret = - fn->Call(env()->context(), v8::Null(env()->isolate()), 1, &uid); - if (ret.IsEmpty()) { - ClearFatalExceptionHandlers(env()); - FatalException(env()->isolate(), try_catch); - } - } -} - - inline bool AsyncWrap::ran_init_callback() const { return static_cast(bits_ & 1); } diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 60124e47ad8833..af634a165a56c3 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -191,6 +191,77 @@ void LoadAsyncWrapperInfo(Environment* env) { } +AsyncWrap::AsyncWrap(Environment* env, + Local object, + ProviderType provider, + AsyncWrap* parent) + : BaseObject(env,object), bits_(static_cast(provider) << 1), + uid_(env->get_async_wrap_uid()) { + CHECK_NE(provider, PROVIDER_NONE); + CHECK_GE(object->InternalFieldCount(), 1); + + // Shift provider value over to prevent id collision. + persistent().SetWrapperClassId(NODE_ASYNC_ID_OFFSET + provider); + + Local init_fn = env->async_hooks_init_function(); + + // No init callback exists, no reason to go on. + if (init_fn.IsEmpty()) + return; + + // If async wrap callbacks are disabled and no parent was passed that has + // run the init callback then return. + if (!env->async_wrap_callbacks_enabled() && + (parent == nullptr || !parent->ran_init_callback())) + return; + + HandleScope scope(env->isolate()); + + Local argv[] = { + Number::New(env->isolate(), get_uid()), + Int32::New(env->isolate(), provider), + Null(env->isolate()), + Null(env->isolate()) + }; + + if (parent != nullptr) { + argv[2] = Number::New(env->isolate(), parent->get_uid()); + argv[3] = parent->object(); + } + + TryCatch try_catch(env->isolate()); + + MaybeLocal ret = + init_fn->Call(env->context(), object, arraysize(argv), argv); + + if (ret.IsEmpty()) { + ClearFatalExceptionHandlers(env); + FatalException(env->isolate(), try_catch); + } + + bits_ |= 1; // ran_init_callback() is true now. +} + + +AsyncWrap::~AsyncWrap() { + if (!ran_init_callback()) + return; + + Local fn = env()->async_hooks_destroy_function(); + if (!fn.IsEmpty()) { + HandleScope scope(env()->isolate()); + Local uid = Number::New(env()->isolate(), get_uid()); + TryCatch try_catch(env()->isolate()); + MaybeLocal ret = + fn->Call(env()->context(), Null(env()->isolate()), 1, &uid); + if (ret.IsEmpty()) { + ClearFatalExceptionHandlers(env()); + FatalException(env()->isolate(), try_catch); + } + } +} + + Local AsyncWrap::MakeCallback(const Local cb, int argc, Local* argv) { diff --git a/src/async-wrap.h b/src/async-wrap.h index e1ea383d9f1a09..bdd6883f211c2f 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -49,12 +49,12 @@ class AsyncWrap : public BaseObject { #undef V }; - inline AsyncWrap(Environment* env, - v8::Local object, - ProviderType provider, - AsyncWrap* parent = nullptr); + AsyncWrap(Environment* env, + v8::Local object, + ProviderType provider, + AsyncWrap* parent = nullptr); - inline virtual ~AsyncWrap(); + virtual ~AsyncWrap(); inline ProviderType provider_type() const; From cf5f4b85f5ef7befa592d83b69be646f7efb236a Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 22 Nov 2016 17:05:14 -0700 Subject: [PATCH 2/3] async_wrap: make Initialize a static class member This is how it's done everywhere else in core. Make it follow suit. PR-URL: https://github.com/nodejs/node/pull/9753 Reviewed-By: Anna Henningsen Reviewed-By: Ben Noordhuis --- src/async-wrap.cc | 11 ++++++----- src/async-wrap.h | 4 ++++ 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index af634a165a56c3..f1f5a09dc0796e 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -14,6 +14,7 @@ using v8::Function; using v8::FunctionCallbackInfo; using v8::HandleScope; using v8::HeapProfiler; +using v8::Int32; using v8::Integer; using v8::Isolate; using v8::Local; @@ -155,9 +156,9 @@ static void SetupHooks(const FunctionCallbackInfo& args) { } -static void Initialize(Local target, - Local unused, - Local context) { +void AsyncWrap::Initialize(Local target, + Local unused, + Local context) { Environment* env = Environment::GetCurrent(context); Isolate* isolate = env->isolate(); HandleScope scope(isolate); @@ -195,7 +196,7 @@ AsyncWrap::AsyncWrap(Environment* env, Local object, ProviderType provider, AsyncWrap* parent) - : BaseObject(env,object), bits_(static_cast(provider) << 1), + : BaseObject(env, object), bits_(static_cast(provider) << 1), uid_(env->get_async_wrap_uid()) { CHECK_NE(provider, PROVIDER_NONE); CHECK_GE(object->InternalFieldCount(), 1); @@ -361,4 +362,4 @@ Local AsyncWrap::MakeCallback(const Local cb, } // namespace node -NODE_MODULE_CONTEXT_AWARE_BUILTIN(async_wrap, node::Initialize) +NODE_MODULE_CONTEXT_AWARE_BUILTIN(async_wrap, node::AsyncWrap::Initialize) diff --git a/src/async-wrap.h b/src/async-wrap.h index bdd6883f211c2f..5ffd67dba9d15c 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -56,6 +56,10 @@ class AsyncWrap : public BaseObject { virtual ~AsyncWrap(); + static void Initialize(v8::Local target, + v8::Local unused, + v8::Local context); + inline ProviderType provider_type() const; inline int64_t get_uid() const; From b49b496a92118bf786176bc3c763a292b9bdff5d Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 22 Nov 2016 22:35:10 -0700 Subject: [PATCH 3/3] async_wrap: call destroy() callback in uv_idle_t Calling JS during GC is a no-no. So intead create a queue of all ids that need to have their destroy() callback called and call them later. Removed checking destroy() in test-async-wrap-uid because destroy() can be called after the 'exit' callback. Missing a reliable test to reproduce the issue that caused the FATAL_ERROR. PR-URL: https://github.com/nodejs/node/pull/9753 Fixes: https://github.com/nodejs/node/issues/8216 Fixes: https://github.com/nodejs/node/issues/9465 Reviewed-By: Anna Henningsen Reviewed-By: Ben Noordhuis --- src/async-wrap.cc | 49 +++++++++++++++++++++------- src/async-wrap.h | 3 ++ src/env-inl.h | 15 +++++++++ src/env.cc | 3 ++ src/env.h | 8 +++++ test/parallel/test-async-wrap-uid.js | 8 +---- 6 files changed, 67 insertions(+), 19 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index f1f5a09dc0796e..42463bd22b31f4 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -5,6 +5,7 @@ #include "util.h" #include "util-inl.h" +#include "uv.h" #include "v8.h" #include "v8-profiler.h" @@ -182,6 +183,38 @@ void AsyncWrap::Initialize(Local target, } +void AsyncWrap::DestroyIdsCb(uv_idle_t* handle) { + uv_idle_stop(handle); + + Environment* env = Environment::from_destroy_ids_idle_handle(handle); + // None of the V8 calls done outside the HandleScope leak a handle. If this + // changes in the future then the SealHandleScope wrapping the uv_run() + // will catch this can cause the process to abort. + HandleScope handle_scope(env->isolate()); + Context::Scope context_scope(env->context()); + Local fn = env->async_hooks_destroy_function(); + + if (fn.IsEmpty()) + return env->destroy_ids_list()->clear(); + + TryCatch try_catch(env->isolate()); + + for (auto current_id : *env->destroy_ids_list()) { + // Want each callback to be cleaned up after itself, instead of cleaning + // them all up after the while() loop completes. + HandleScope scope(env->isolate()); + Local argv = Number::New(env->isolate(), current_id); + MaybeLocal ret = fn->Call( + env->context(), Undefined(env->isolate()), 1, &argv); + + if (ret.IsEmpty()) { + ClearFatalExceptionHandlers(env); + FatalException(env->isolate(), try_catch); + } + } +} + + void LoadAsyncWrapperInfo(Environment* env) { HeapProfiler* heap_profiler = env->isolate()->GetHeapProfiler(); #define V(PROVIDER) \ @@ -248,18 +281,10 @@ AsyncWrap::~AsyncWrap() { if (!ran_init_callback()) return; - Local fn = env()->async_hooks_destroy_function(); - if (!fn.IsEmpty()) { - HandleScope scope(env()->isolate()); - Local uid = Number::New(env()->isolate(), get_uid()); - TryCatch try_catch(env()->isolate()); - MaybeLocal ret = - fn->Call(env()->context(), Null(env()->isolate()), 1, &uid); - if (ret.IsEmpty()) { - ClearFatalExceptionHandlers(env()); - FatalException(env()->isolate(), try_catch); - } - } + if (env()->destroy_ids_list()->empty()) + uv_idle_start(env()->destroy_ids_idle_handle(), DestroyIdsCb); + + env()->destroy_ids_list()->push_back(get_uid()); } diff --git a/src/async-wrap.h b/src/async-wrap.h index 5ffd67dba9d15c..d01c6ce9f9b724 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -4,6 +4,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include "base-object.h" +#include "uv.h" #include "v8.h" #include @@ -60,6 +61,8 @@ class AsyncWrap : public BaseObject { v8::Local unused, v8::Local context); + static void DestroyIdsCb(uv_idle_t* handle); + inline ProviderType provider_type() const; inline int64_t get_uid() const; diff --git a/src/env-inl.h b/src/env-inl.h index 74e427e40353d9..83db3d33b6d18f 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -194,6 +194,8 @@ inline Environment::Environment(IsolateData* isolate_data, RB_INIT(&cares_task_list_); AssignToContext(context); + + destroy_ids_list_.reserve(512); } inline Environment::~Environment() { @@ -247,6 +249,15 @@ inline uv_idle_t* Environment::immediate_idle_handle() { return &immediate_idle_handle_; } +inline Environment* Environment::from_destroy_ids_idle_handle( + uv_idle_t* handle) { + return ContainerOf(&Environment::destroy_ids_idle_handle_, handle); +} + +inline uv_idle_t* Environment::destroy_ids_idle_handle() { + return &destroy_ids_idle_handle_; +} + inline void Environment::RegisterHandleCleanup(uv_handle_t* handle, HandleCleanupCb cb, void *arg) { @@ -301,6 +312,10 @@ inline int64_t Environment::get_async_wrap_uid() { return ++async_wrap_uid_; } +inline std::vector* Environment::destroy_ids_list() { + return &destroy_ids_list_; +} + inline uint32_t* Environment::heap_statistics_buffer() const { CHECK_NE(heap_statistics_buffer_, nullptr); return heap_statistics_buffer_; diff --git a/src/env.cc b/src/env.cc index efa2d53f0435b2..40f0c9ebd66a07 100644 --- a/src/env.cc +++ b/src/env.cc @@ -49,6 +49,9 @@ void Environment::Start(int argc, uv_unref(reinterpret_cast(&idle_prepare_handle_)); uv_unref(reinterpret_cast(&idle_check_handle_)); + uv_idle_init(event_loop(), destroy_ids_idle_handle()); + uv_unref(reinterpret_cast(destroy_ids_idle_handle())); + auto close_and_finish = [](Environment* env, uv_handle_t* handle, void* arg) { handle->data = env; diff --git a/src/env.h b/src/env.h index 51049ed2bea473..b99bb45f819e59 100644 --- a/src/env.h +++ b/src/env.h @@ -16,6 +16,7 @@ #include "v8.h" #include +#include // Caveat emptor: we're going slightly crazy with macros here but the end // hopefully justifies the means. We have a lot of per-context properties @@ -431,8 +432,10 @@ class Environment { inline uint32_t watched_providers() const; static inline Environment* from_immediate_check_handle(uv_check_t* handle); + static inline Environment* from_destroy_ids_idle_handle(uv_idle_t* handle); inline uv_check_t* immediate_check_handle(); inline uv_idle_t* immediate_idle_handle(); + inline uv_idle_t* destroy_ids_idle_handle(); // Register clean-up cb to be called on environment destruction. inline void RegisterHandleCleanup(uv_handle_t* handle, @@ -463,6 +466,9 @@ class Environment { inline int64_t get_async_wrap_uid(); + // List of id's that have been destroyed and need the destroy() cb called. + inline std::vector* destroy_ids_list(); + inline uint32_t* heap_statistics_buffer() const; inline void set_heap_statistics_buffer(uint32_t* pointer); @@ -548,6 +554,7 @@ class Environment { IsolateData* const isolate_data_; uv_check_t immediate_check_handle_; uv_idle_t immediate_idle_handle_; + uv_idle_t destroy_ids_idle_handle_; uv_prepare_t idle_prepare_handle_; uv_check_t idle_check_handle_; AsyncHooks async_hooks_; @@ -562,6 +569,7 @@ class Environment { bool trace_sync_io_; size_t makecallback_cntr_; int64_t async_wrap_uid_; + std::vector destroy_ids_list_; debugger::Agent debugger_agent_; #if HAVE_INSPECTOR inspector::Agent inspector_agent_; diff --git a/test/parallel/test-async-wrap-uid.js b/test/parallel/test-async-wrap-uid.js index 5bf3a8856e0e3f..3497c3b0768ddd 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 }); async_wrap.enable(); function init(uid) { @@ -14,7 +14,6 @@ function init(uid) { init: true, pre: false, post: false, - destroy: false }); } @@ -26,10 +25,6 @@ function post(uid) { storage.get(uid).post = true; } -function destroy(uid) { - storage.get(uid).destroy = true; -} - fs.access(__filename, function(err) { assert.ifError(err); }); @@ -51,7 +46,6 @@ process.once('exit', function() { init: true, pre: true, post: true, - destroy: true }); } });