From 256534c3c25f6ab992e46d1c96c7e392384611d9 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. 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 b420cb020df04e39e978ba86724904596606d20d 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. 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 340aee7354c475c5e841708e3cc76f3f9adc4b48 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. 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.h | 8 +++ src/node.cc | 3 ++ .../test-async-wrap-throw-from-callback.js | 8 +-- test/parallel/test-async-wrap-uid.js | 8 +-- 7 files changed, 69 insertions(+), 25 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 607ac445e3d085..0981a09aeb4bed 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -245,6 +245,8 @@ inline Environment::Environment(v8::Local context, RB_INIT(&cares_task_list_); handle_cleanup_waiting_ = 0; + + destroy_ids_list_.reserve(512); } inline Environment::~Environment() { @@ -305,6 +307,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 Environment* Environment::from_idle_prepare_handle( uv_prepare_t* handle) { return ContainerOf(&Environment::idle_prepare_handle_, handle); @@ -381,6 +392,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.h b/src/env.h index b2e4e9d0ee72f9..cf881a4196bea2 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 @@ -413,8 +414,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(); static inline Environment* from_idle_prepare_handle(uv_prepare_t* handle); inline uv_prepare_t* idle_prepare_handle(); @@ -451,6 +454,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); @@ -543,6 +549,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_; @@ -558,6 +565,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/src/node.cc b/src/node.cc index 5207da0a1bfdc0..ce39cb45583ebf 100644 --- a/src/node.cc +++ b/src/node.cc @@ -4511,6 +4511,9 @@ Environment* CreateEnvironment(Isolate* isolate, uv_unref(reinterpret_cast(env->idle_prepare_handle())); uv_unref(reinterpret_cast(env->idle_check_handle())); + uv_idle_init(env->event_loop(), env->destroy_ids_idle_handle()); + uv_unref(reinterpret_cast(env->destroy_ids_idle_handle())); + // Register handle cleanups env->RegisterHandleCleanup( reinterpret_cast(env->immediate_check_handle()), diff --git a/test/parallel/test-async-wrap-throw-from-callback.js b/test/parallel/test-async-wrap-throw-from-callback.js index bfbe32c38b021a..e2e01e97ad6db9 100644 --- a/test/parallel/test-async-wrap-throw-from-callback.js +++ b/test/parallel/test-async-wrap-throw-from-callback.js @@ -6,7 +6,7 @@ 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 callbacks = [ 'init', 'pre', 'post' ]; const toCall = process.argv[2]; var msgCalled = 0; var msgReceived = 0; @@ -23,13 +23,9 @@ 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.setupHooks({ init, pre, post }); async_wrap.enable(); process.on('uncaughtException', () => assert.ok(0, 'UNREACHABLE')); 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 }); } });