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 }); } });