From 5379b9da11bf8baeafc7074dc64bcc144db89e64 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 22 Nov 2016 22:35:10 -0700 Subject: [PATCH] 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 }); } });