From 0bf2b28ef0bb1bb1cd057b67fc6f8d0229907bb4 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Wed, 6 Jun 2018 16:29:30 -0400 Subject: [PATCH 1/5] src: do not persist timer handle in cares_wrap Instead of relying on garbage collection to close the timer handle, manage its state more explicitly. --- src/cares_wrap.cc | 46 +++++++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index cc29ddad5586d9..05ef2b7e12090e 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -151,7 +151,8 @@ class ChannelWrap : public AsyncWrap { void Setup(); void EnsureServers(); - void CleanupTimer(); + void StartTimer(); + void CloseTimer(); void ModifyActivityQueryCount(int count); @@ -313,13 +314,7 @@ void ares_sockstate_cb(void* data, if (read || write) { if (!task) { /* New socket */ - - /* If this is the first socket then start the timer. */ - uv_timer_t* timer_handle = channel->timer_handle(); - if (!uv_is_active(reinterpret_cast(timer_handle))) { - CHECK(channel->task_list()->empty()); - uv_timer_start(timer_handle, ChannelWrap::AresTimeout, 1000, 1000); - } + channel->StartTimer(); task = ares_task_create(channel, sock); if (task == nullptr) { @@ -349,7 +344,7 @@ void ares_sockstate_cb(void* data, channel->env()->CloseHandle(&task->poll_watcher, ares_poll_close_cb); if (channel->task_list()->empty()) { - uv_timer_stop(channel->timer_handle()); + channel->CloseTimer(); } } } @@ -490,15 +485,26 @@ void ChannelWrap::Setup() { } library_inited_ = true; +} - /* Initialize the timeout timer. The timer won't be started until the */ - /* first socket is opened. */ - CleanupTimer(); - timer_handle_ = new uv_timer_t(); - timer_handle_->data = static_cast(this); - uv_timer_init(env()->event_loop(), timer_handle_); +void ChannelWrap::StartTimer() { + if (timer_handle_ == nullptr) { + timer_handle_ = new uv_timer_t(); + timer_handle_->data = static_cast(this); + uv_timer_init(env()->event_loop(), timer_handle_); + } else if (uv_is_active(reinterpret_cast(timer_handle_))) { + return; + } + uv_timer_start(timer_handle_, AresTimeout, 1000, 1000); } +void ChannelWrap::CloseTimer() { + if (timer_handle_ == nullptr) + return; + + env()->CloseHandle(timer_handle_, [](uv_timer_t* handle) { delete handle; }); + timer_handle_ = nullptr; +} ChannelWrap::~ChannelWrap() { if (library_inited_) { @@ -508,17 +514,10 @@ ChannelWrap::~ChannelWrap() { } ares_destroy(channel_); - CleanupTimer(); + CloseTimer(); } -void ChannelWrap::CleanupTimer() { - if (timer_handle_ == nullptr) return; - - env()->CloseHandle(timer_handle_, [](uv_timer_t* handle) { delete handle; }); - timer_handle_ = nullptr; -} - void ChannelWrap::ModifyActivityQueryCount(int count) { active_query_count_ += count; if (active_query_count_ < 0) active_query_count_ = 0; @@ -566,6 +565,7 @@ void ChannelWrap::EnsureServers() { /* destroy channel and reset channel */ ares_destroy(channel_); + CloseTimer(); Setup(); } From c62b30a88a6e5433d16a3b3ce67e194c3308e3fc Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Wed, 6 Jun 2018 16:32:51 -0400 Subject: [PATCH 2/5] src: do not persist fs_poll handle in stat_watcher Instead of relying on garbage collection to close the handle, manage its state more explicitly. --- src/node_stat_watcher.cc | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc index 3f7da197b2a74d..b83db71b3552d6 100644 --- a/src/node_stat_watcher.cc +++ b/src/node_stat_watcher.cc @@ -77,19 +77,14 @@ void StatWatcher::Initialize(Environment* env, Local target) { StatWatcher::StatWatcher(Environment* env, Local wrap, bool use_bigint) : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_STATWATCHER), - watcher_(new uv_fs_poll_t), + watcher_(nullptr), use_bigint_(use_bigint) { MakeWeak(); - uv_fs_poll_init(env->event_loop(), watcher_); - watcher_->data = static_cast(this); } StatWatcher::~StatWatcher() { - if (IsActive()) { - Stop(); - } - env()->CloseHandle(watcher_, [](uv_fs_poll_t* handle) { delete handle; }); + CHECK_EQ(watcher_, nullptr); } @@ -123,7 +118,7 @@ void StatWatcher::New(const FunctionCallbackInfo& args) { } bool StatWatcher::IsActive() { - return uv_is_active(reinterpret_cast(watcher_)) != 0; + return watcher_ != nullptr; } void StatWatcher::IsActive(const v8::FunctionCallbackInfo& args) { @@ -157,6 +152,9 @@ void StatWatcher::Start(const FunctionCallbackInfo& args) { const uint32_t interval = args[2].As()->Value(); // Safe, uv_ref/uv_unref are idempotent. + wrap->watcher_ = new uv_fs_poll_t(); + uv_fs_poll_init(wrap->env()->event_loop(), wrap->watcher_); + wrap->watcher_->data = static_cast(wrap); if (persistent) uv_ref(reinterpret_cast(wrap->watcher_)); else @@ -187,7 +185,8 @@ void StatWatcher::Stop(const FunctionCallbackInfo& args) { void StatWatcher::Stop() { - uv_fs_poll_stop(watcher_); + env()->CloseHandle(watcher_, [](uv_fs_poll_t* handle) { delete handle; }); + watcher_ = nullptr; MakeWeak(); } From 2897c15dd953e3d0c086ff5de1d32a73fe395050 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Wed, 6 Jun 2018 16:06:51 -0400 Subject: [PATCH 3/5] test: check gc does not resurrect the loop --- test/sequential/test-async-wrap-getasyncid.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/sequential/test-async-wrap-getasyncid.js b/test/sequential/test-async-wrap-getasyncid.js index c16624a79e6c83..60309ecb2d49b8 100644 --- a/test/sequential/test-async-wrap-getasyncid.js +++ b/test/sequential/test-async-wrap-getasyncid.js @@ -1,4 +1,5 @@ 'use strict'; +// Flags: --expose-gc const common = require('../common'); const assert = require('assert'); @@ -22,6 +23,11 @@ common.crashOnUnhandledRejection(); }, }).enable(); process.on('beforeExit', common.mustCall(() => { + // This garbage collection call verifies that the wraps being garbage + // collected doesn't resurrect the process again due to weirdly timed + // uv_close calls and other similar instruments in destructors. + global.gc(); + process.removeAllListeners('uncaughtException'); hooks.disable(); delete providers.NONE; // Should never be used. From 95fc1912c7224012a45731a624ce7bb772c252af Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Wed, 6 Jun 2018 17:32:30 -0400 Subject: [PATCH 4/5] fixup: addaleax feedback --- src/node_stat_watcher.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc index b83db71b3552d6..c922b0b8737d90 100644 --- a/src/node_stat_watcher.cc +++ b/src/node_stat_watcher.cc @@ -84,7 +84,8 @@ StatWatcher::StatWatcher(Environment* env, Local wrap, bool use_bigint) StatWatcher::~StatWatcher() { - CHECK_EQ(watcher_, nullptr); + if (IsActive()) + Stop(); } From 1156ee232949ac01542008a0843c6818bd4a7477 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Thu, 7 Jun 2018 08:17:32 -0400 Subject: [PATCH 5/5] fixup: bnoordhuis feedback --- src/node_stat_watcher.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc index c922b0b8737d90..0409cfbfb5fac1 100644 --- a/src/node_stat_watcher.cc +++ b/src/node_stat_watcher.cc @@ -152,10 +152,10 @@ void StatWatcher::Start(const FunctionCallbackInfo& args) { CHECK(args[2]->IsUint32()); const uint32_t interval = args[2].As()->Value(); - // Safe, uv_ref/uv_unref are idempotent. wrap->watcher_ = new uv_fs_poll_t(); - uv_fs_poll_init(wrap->env()->event_loop(), wrap->watcher_); + CHECK_EQ(0, uv_fs_poll_init(wrap->env()->event_loop(), wrap->watcher_)); wrap->watcher_->data = static_cast(wrap); + // Safe, uv_ref/uv_unref are idempotent. if (persistent) uv_ref(reinterpret_cast(wrap->watcher_)); else