From 1e44fd960fe38b1191ed9ceb034eade3054b5531 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 1 Jun 2017 05:56:29 -0600 Subject: [PATCH 1/2] async_wrap: run destroy in uv_timer_t Calling the destroy callbacks in a uv_idle_t causes a timing issue where if a handle or request is closed then the class isn't deleted until uv_close() callbacks are called (which happens after the poll phase). This results in some destroy callbacks not being called just before the application exits. So instead switch the destroy callbacks to be called in a uv_timer_t with the timeout set to zero. When uv_run() is called with UV_RUN_ONCE the final operation of the event loop is to process all remaining timers. By setting the timeout to zero it results in the destroy callbacks being processed after uv_close() but before uv_run() returned. Processing the destroyed ids that were previously missed. Also, process the destroy_ids_list() in a do {} while() loop that makes sure the vector is empty before returning. Which also makes running clear() unnecessary. Fixes: https://github.com/nodejs/node/issues/13262 PR-URL: https://github.com/nodejs/node/pull/13369 Reviewed-By: Anna Henningsen Reviewed-By: Andreas Madsen --- src/async-wrap.cc | 40 ++++++++++++++---------------- src/env-inl.h | 10 ++++---- src/env.cc | 13 +--------- src/env.h | 6 ++--- test/async-hooks/test-writewrap.js | 2 +- 5 files changed, 29 insertions(+), 42 deletions(-) diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 0ea6c64a8be582..b5d3d959bdbd6c 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -138,10 +138,8 @@ RetainedObjectInfo* WrapperInfo(uint16_t class_id, Local wrapper) { // end RetainedAsyncInfo -static void DestroyIdsCb(uv_idle_t* handle) { - uv_idle_stop(handle); - - Environment* env = Environment::from_destroy_ids_idle_handle(handle); +static void DestroyIdsCb(uv_timer_t* handle) { + Environment* env = Environment::from_destroy_ids_timer_handle(handle); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); @@ -149,23 +147,23 @@ static void DestroyIdsCb(uv_idle_t* handle) { TryCatch try_catch(env->isolate()); - std::vector destroy_ids_list; - destroy_ids_list.swap(*env->destroy_ids_list()); - for (auto current_id : 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); + do { + std::vector destroy_ids_list; + destroy_ids_list.swap(*env->destroy_ids_list()); + for (auto current_id : 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); + } } - } - - env->destroy_ids_list()->clear(); + } while (!env->destroy_ids_list()->empty()); } @@ -174,7 +172,7 @@ static void PushBackDestroyId(Environment* env, double id) { return; if (env->destroy_ids_list()->empty()) - uv_idle_start(env->destroy_ids_idle_handle(), DestroyIdsCb); + uv_timer_start(env->destroy_ids_timer_handle(), DestroyIdsCb, 0, 0); env->destroy_ids_list()->push_back(id); } diff --git a/src/env-inl.h b/src/env-inl.h index e2a58761fd026d..f73f9687b0a4f3 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -356,13 +356,13 @@ 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 Environment* Environment::from_destroy_ids_timer_handle( + uv_timer_t* handle) { + return ContainerOf(&Environment::destroy_ids_timer_handle_, handle); } -inline uv_idle_t* Environment::destroy_ids_idle_handle() { - return &destroy_ids_idle_handle_; +inline uv_timer_t* Environment::destroy_ids_timer_handle() { + return &destroy_ids_timer_handle_; } inline void Environment::RegisterHandleCleanup(uv_handle_t* handle, diff --git a/src/env.cc b/src/env.cc index 034625b375446c..0bf4b573aafd69 100644 --- a/src/env.cc +++ b/src/env.cc @@ -49,8 +49,7 @@ 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())); + uv_timer_init(event_loop(), destroy_ids_timer_handle()); auto close_and_finish = [](Environment* env, uv_handle_t* handle, void* arg) { handle->data = env; @@ -102,16 +101,6 @@ void Environment::CleanupHandles() { while (handle_cleanup_waiting_ != 0) uv_run(event_loop(), UV_RUN_ONCE); - // Closing the destroy_ids_idle_handle_ within the handle cleanup queue - // prevents the async wrap destroy hook from being called. - uv_handle_t* handle = - reinterpret_cast(&destroy_ids_idle_handle_); - handle->data = this; - handle_cleanup_waiting_ = 1; - uv_close(handle, [](uv_handle_t* handle) { - static_cast(handle->data)->FinishHandleCleanup(handle); - }); - while (handle_cleanup_waiting_ != 0) uv_run(event_loop(), UV_RUN_ONCE); } diff --git a/src/env.h b/src/env.h index c8c8232cc07fd4..7179a6081cc417 100644 --- a/src/env.h +++ b/src/env.h @@ -526,10 +526,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); + static inline Environment* from_destroy_ids_timer_handle(uv_timer_t* handle); inline uv_check_t* immediate_check_handle(); inline uv_idle_t* immediate_idle_handle(); - inline uv_idle_t* destroy_ids_idle_handle(); + inline uv_timer_t* destroy_ids_timer_handle(); // Register clean-up cb to be called on environment destruction. inline void RegisterHandleCleanup(uv_handle_t* handle, @@ -662,7 +662,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_timer_t destroy_ids_timer_handle_; uv_prepare_t idle_prepare_handle_; uv_check_t idle_check_handle_; AsyncHooks async_hooks_; diff --git a/test/async-hooks/test-writewrap.js b/test/async-hooks/test-writewrap.js index fecceaf13c5cad..89e5cfbb13889a 100644 --- a/test/async-hooks/test-writewrap.js +++ b/test/async-hooks/test-writewrap.js @@ -53,7 +53,7 @@ function checkDestroyedWriteWraps(n, stage) { assert.strictEqual(typeof w.uid, 'number', 'uid is a number'); assert.strictEqual(typeof w.triggerId, 'number', 'triggerId is a number'); - checkInvocations(w, { init: 1, destroy: 1 }, 'when ' + stage); + checkInvocations(w, { init: 1 }, 'when ' + stage); } as.forEach(checkValidWriteWrap); } From 78b135806f10c39c3bbd00edf206ddc9f505597e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 1 Jun 2017 06:13:58 -0600 Subject: [PATCH 2/2] test: check destroy hooks are called before exit Verify that the destroy callback for a TCP server is called before exit if it is closed in another destroy callback. Fixes: https://github.com/nodejs/node/issues/13262 PR-URL: https://github.com/nodejs/node/pull/13369 Reviewed-By: Trevor Norris Reviewed-By: Andreas Madsen --- .../test-async-hooks-close-during-destroy.js | 37 +++++++++++++++++++ ...st-async-hooks-top-level-clearimmediate.js | 27 ++++++++++++++ 2 files changed, 64 insertions(+) create mode 100644 test/parallel/test-async-hooks-close-during-destroy.js create mode 100644 test/parallel/test-async-hooks-top-level-clearimmediate.js diff --git a/test/parallel/test-async-hooks-close-during-destroy.js b/test/parallel/test-async-hooks-close-during-destroy.js new file mode 100644 index 00000000000000..98e12e77fbcce1 --- /dev/null +++ b/test/parallel/test-async-hooks-close-during-destroy.js @@ -0,0 +1,37 @@ +'use strict'; +// Test that async ids that are added to the destroy queue while running a +// `destroy` callback are handled correctly. + +const common = require('../common'); +const assert = require('assert'); +const async_hooks = require('async_hooks'); + +const initCalls = new Set(); +let destroyResCallCount = 0; +let res2; + +async_hooks.createHook({ + init: common.mustCallAtLeast((id, provider, triggerId) => { + if (provider === 'foobar') + initCalls.add(id); + }, 2), + destroy: common.mustCallAtLeast((id) => { + if (!initCalls.has(id)) return; + + switch (destroyResCallCount++) { + case 0: + // Trigger the second `destroy` call. + res2.emitDestroy(); + break; + case 2: + assert.fail('More than 2 destroy() invocations'); + break; + } + }, 2) +}).enable(); + +const res1 = new async_hooks.AsyncResource('foobar'); +res2 = new async_hooks.AsyncResource('foobar'); +res1.emitDestroy(); + +process.on('exit', () => assert.strictEqual(destroyResCallCount, 2)); diff --git a/test/parallel/test-async-hooks-top-level-clearimmediate.js b/test/parallel/test-async-hooks-top-level-clearimmediate.js new file mode 100644 index 00000000000000..f667c3ca337816 --- /dev/null +++ b/test/parallel/test-async-hooks-top-level-clearimmediate.js @@ -0,0 +1,27 @@ +'use strict'; + +// Regression test for https://github.com/nodejs/node/issues/13262 + +const common = require('../common'); +const assert = require('assert'); +const async_hooks = require('async_hooks'); + +let seenId, seenResource; + +async_hooks.createHook({ + init: common.mustCall((id, provider, triggerId, resource) => { + seenId = id; + seenResource = resource; + assert.strictEqual(provider, 'Immediate'); + assert.strictEqual(triggerId, 1); + }), + before: common.mustNotCall(), + after: common.mustNotCall(), + destroy: common.mustCall((id) => { + assert.strictEqual(seenId, id); + }) +}).enable(); + +const immediate = setImmediate(common.mustNotCall()); +assert.strictEqual(immediate, seenResource); +clearImmediate(immediate);