From 5e3538ba1582086e75702b78cc7521ceac09c264 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 1 Aug 2016 12:51:01 +0200 Subject: [PATCH 1/2] src: use RAII for mutexes in node_watchdog.cc --- src/node_watchdog.cc | 62 ++++++++++++++++++-------------------------- src/node_watchdog.h | 5 ++-- 2 files changed, 28 insertions(+), 39 deletions(-) diff --git a/src/node_watchdog.cc b/src/node_watchdog.cc index 9c776973a2d630..af781392e8915e 100644 --- a/src/node_watchdog.cc +++ b/src/node_watchdog.cc @@ -160,7 +160,7 @@ BOOL WINAPI SigintWatchdogHelper::WinCtrlCHandlerRoutine(DWORD dwCtrlType) { bool SigintWatchdogHelper::InformWatchdogsAboutSignal() { - uv_mutex_lock(&instance.list_mutex_); + Mutex::ScopedLock list_lock(instance.list_mutex_); bool is_stopping = false; #ifdef __POSIX__ @@ -176,17 +176,15 @@ bool SigintWatchdogHelper::InformWatchdogsAboutSignal() { for (auto it : instance.watchdogs_) it->HandleSigint(); - uv_mutex_unlock(&instance.list_mutex_); return is_stopping; } int SigintWatchdogHelper::Start() { - int ret = 0; - uv_mutex_lock(&mutex_); + Mutex::ScopedLock lock(mutex_); if (start_stop_count_++ > 0) { - goto dont_start; + return 0; } #ifdef __POSIX__ @@ -197,10 +195,10 @@ int SigintWatchdogHelper::Start() { sigset_t sigmask; sigfillset(&sigmask); CHECK_EQ(0, pthread_sigmask(SIG_SETMASK, &sigmask, &sigmask)); - ret = pthread_create(&thread_, nullptr, RunSigintWatchdog, nullptr); + int ret = pthread_create(&thread_, nullptr, RunSigintWatchdog, nullptr); CHECK_EQ(0, pthread_sigmask(SIG_SETMASK, &sigmask, nullptr)); if (ret != 0) { - goto dont_start; + return ret; } has_running_thread_ = true; @@ -209,34 +207,36 @@ int SigintWatchdogHelper::Start() { SetConsoleCtrlHandler(WinCtrlCHandlerRoutine, TRUE); #endif - dont_start: - uv_mutex_unlock(&mutex_); - return ret; + return 0; } bool SigintWatchdogHelper::Stop() { - uv_mutex_lock(&mutex_); - uv_mutex_lock(&list_mutex_); + bool had_pending_signal; + Mutex::ScopedLock lock(mutex_); - bool had_pending_signal = has_pending_signal_; + { + Mutex::ScopedLock list_lock(list_mutex_); - if (--start_stop_count_ > 0) { - uv_mutex_unlock(&list_mutex_); - goto dont_stop; - } + had_pending_signal = has_pending_signal_; + + if (--start_stop_count_ > 0) { + has_pending_signal_ = false; + return had_pending_signal; + } #ifdef __POSIX__ - // Set stopping now because it's only protected by list_mutex_. - stopping_ = true; + // Set stopping now because it's only protected by list_mutex_. + stopping_ = true; #endif - watchdogs_.clear(); - uv_mutex_unlock(&list_mutex_); + watchdogs_.clear(); + } #ifdef __POSIX__ if (!has_running_thread_) { - goto dont_stop; + has_pending_signal_ = false; + return had_pending_signal; } // Wake up the helper thread. @@ -252,32 +252,26 @@ bool SigintWatchdogHelper::Stop() { #endif had_pending_signal = has_pending_signal_; - dont_stop: - uv_mutex_unlock(&mutex_); - has_pending_signal_ = false; + return had_pending_signal; } void SigintWatchdogHelper::Register(SigintWatchdog* wd) { - uv_mutex_lock(&list_mutex_); + Mutex::ScopedLock lock(list_mutex_); watchdogs_.push_back(wd); - - uv_mutex_unlock(&list_mutex_); } void SigintWatchdogHelper::Unregister(SigintWatchdog* wd) { - uv_mutex_lock(&list_mutex_); + Mutex::ScopedLock lock(list_mutex_); auto it = std::find(watchdogs_.begin(), watchdogs_.end(), wd); CHECK_NE(it, watchdogs_.end()); watchdogs_.erase(it); - - uv_mutex_unlock(&list_mutex_); } @@ -289,9 +283,6 @@ SigintWatchdogHelper::SigintWatchdogHelper() stopping_ = false; CHECK_EQ(0, uv_sem_init(&sem_, 0)); #endif - - CHECK_EQ(0, uv_mutex_init(&mutex_)); - CHECK_EQ(0, uv_mutex_init(&list_mutex_)); } @@ -303,9 +294,6 @@ SigintWatchdogHelper::~SigintWatchdogHelper() { CHECK_EQ(has_running_thread_, false); uv_sem_destroy(&sem_); #endif - - uv_mutex_destroy(&mutex_); - uv_mutex_destroy(&list_mutex_); } SigintWatchdogHelper SigintWatchdogHelper::instance; diff --git a/src/node_watchdog.h b/src/node_watchdog.h index d56b7624de9c12..43842147f9f480 100644 --- a/src/node_watchdog.h +++ b/src/node_watchdog.h @@ -5,6 +5,7 @@ #include "v8.h" #include "uv.h" +#include "node_mutex.h" #include #ifdef __POSIX__ @@ -75,8 +76,8 @@ class SigintWatchdogHelper { int start_stop_count_; - uv_mutex_t mutex_; - uv_mutex_t list_mutex_; + Mutex mutex_; + Mutex list_mutex_; std::vector watchdogs_; bool has_pending_signal_; From 36aa89be0474630e2676c2b078fcdd0f77d861ef Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 1 Aug 2016 12:58:24 +0200 Subject: [PATCH 2/2] test,util: fix flaky test-util-sigint-watchdog Fix parallel/test-util-sigint-watchdog by polling until the signal has definitely been received instead of just using a timeout. Fixes: https://github.com/nodejs/node/issues/7919 --- src/node_util.cc | 8 ++++++++ src/node_watchdog.cc | 7 +++++++ src/node_watchdog.h | 1 + test/parallel/test-util-sigint-watchdog.js | 19 +++++++++++++------ 4 files changed, 29 insertions(+), 6 deletions(-) diff --git a/src/node_util.cc b/src/node_util.cc index e60af80326891a..92eed3a9aec5ec 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -113,6 +113,13 @@ void StopSigintWatchdog(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(had_pending_signals); } + +void WatchdogHasPendingSigint(const FunctionCallbackInfo& args) { + bool ret = SigintWatchdogHelper::GetInstance()->HasPendingSignal(); + args.GetReturnValue().Set(ret); +} + + void Initialize(Local target, Local unused, Local context) { @@ -138,6 +145,7 @@ void Initialize(Local target, env->SetMethod(target, "startSigintWatchdog", StartSigintWatchdog); env->SetMethod(target, "stopSigintWatchdog", StopSigintWatchdog); + env->SetMethod(target, "watchdogHasPendingSigint", WatchdogHasPendingSigint); } } // namespace util diff --git a/src/node_watchdog.cc b/src/node_watchdog.cc index af781392e8915e..756ba64696a913 100644 --- a/src/node_watchdog.cc +++ b/src/node_watchdog.cc @@ -258,6 +258,13 @@ bool SigintWatchdogHelper::Stop() { } +bool SigintWatchdogHelper::HasPendingSignal() { + Mutex::ScopedLock lock(list_mutex_); + + return has_pending_signal_; +} + + void SigintWatchdogHelper::Register(SigintWatchdog* wd) { Mutex::ScopedLock lock(list_mutex_); diff --git a/src/node_watchdog.h b/src/node_watchdog.h index 43842147f9f480..dd97e4e735ccdf 100644 --- a/src/node_watchdog.h +++ b/src/node_watchdog.h @@ -63,6 +63,7 @@ class SigintWatchdogHelper { static SigintWatchdogHelper* GetInstance() { return &instance; } void Register(SigintWatchdog* watchdog); void Unregister(SigintWatchdog* watchdog); + bool HasPendingSignal(); int Start(); bool Stop(); diff --git a/test/parallel/test-util-sigint-watchdog.js b/test/parallel/test-util-sigint-watchdog.js index f9bb3ecd93d39b..42e4048bd74a36 100644 --- a/test/parallel/test-util-sigint-watchdog.js +++ b/test/parallel/test-util-sigint-watchdog.js @@ -20,24 +20,24 @@ if (common.isWindows) { // Test with one call to the watchdog, one signal. binding.startSigintWatchdog(); process.kill(process.pid, 'SIGINT'); - setTimeout(common.mustCall(() => { + waitForPendingSignal(common.mustCall(() => { const hadPendingSignals = binding.stopSigintWatchdog(); assert.strictEqual(hadPendingSignals, true); next(); - }), common.platformTimeout(100)); + })); }, (next) => { // Nested calls are okay. binding.startSigintWatchdog(); binding.startSigintWatchdog(); process.kill(process.pid, 'SIGINT'); - setTimeout(common.mustCall(() => { + waitForPendingSignal(common.mustCall(() => { const hadPendingSignals1 = binding.stopSigintWatchdog(); const hadPendingSignals2 = binding.stopSigintWatchdog(); assert.strictEqual(hadPendingSignals1, true); assert.strictEqual(hadPendingSignals2, false); next(); - }), common.platformTimeout(100)); + })); }, () => { // Signal comes in after first call to stop. @@ -45,9 +45,16 @@ if (common.isWindows) { binding.startSigintWatchdog(); const hadPendingSignals1 = binding.stopSigintWatchdog(); process.kill(process.pid, 'SIGINT'); - setTimeout(common.mustCall(() => { + waitForPendingSignal(common.mustCall(() => { const hadPendingSignals2 = binding.stopSigintWatchdog(); assert.strictEqual(hadPendingSignals1, false); assert.strictEqual(hadPendingSignals2, true); - }), common.platformTimeout(100)); + })); }].reduceRight((a, b) => common.mustCall(b).bind(null, a))(); + +function waitForPendingSignal(cb) { + if (binding.watchdogHasPendingSigint()) + cb(); + else + setTimeout(waitForPendingSignal, 10, cb); +}