From 78d12342ba7bdea25b9c4da41679f668788044da Mon Sep 17 00:00:00 2001 From: Guilherme Lawless Date: Fri, 28 Feb 2020 09:31:45 +0000 Subject: [PATCH] Lock before stopping when it is necessary to notify other threads (#2608) This prevents having all threads waiting and the system freezing. Mostly important for tests, especially with sanitizers, but could happen on stopping the node normally. Portmapping requires an atomic bool, and the database queue doesn't after this change. Other small related bootstrap changes included in this PR per Serg's suggestion. --- nano/node/bootstrap/bootstrap_attempt.cpp | 8 ++++-- nano/node/bootstrap/bootstrap_connections.cpp | 28 +++++++++++-------- nano/node/bootstrap/bootstrap_lazy.cpp | 2 +- nano/node/confirmation_height_processor.cpp | 5 +++- nano/node/portmapping.hpp | 2 +- nano/node/write_database_queue.cpp | 5 +++- nano/node/write_database_queue.hpp | 3 +- 7 files changed, 33 insertions(+), 20 deletions(-) diff --git a/nano/node/bootstrap/bootstrap_attempt.cpp b/nano/node/bootstrap/bootstrap_attempt.cpp index d0914c25c..32c9b0b6d 100644 --- a/nano/node/bootstrap/bootstrap_attempt.cpp +++ b/nano/node/bootstrap/bootstrap_attempt.cpp @@ -202,9 +202,11 @@ bool nano::bootstrap_attempt_legacy::consume_future (std::future & future_ void nano::bootstrap_attempt_legacy::stop () { - stopped = true; - condition.notify_all (); nano::unique_lock lock (mutex); + stopped = true; + lock.unlock (); + condition.notify_all (); + lock.lock (); if (auto i = frontiers.lock ()) { try @@ -478,7 +480,7 @@ bool nano::bootstrap_attempt_legacy::request_frontier (nano::unique_lockbootstrap_initiator.connections->connection (shared_from_this (), first_attempt)); lock_a.lock (); - if (connection_l) + if (connection_l && !stopped) { endpoint_frontier_request = connection_l->channel->get_tcp_endpoint (); std::future future; diff --git a/nano/node/bootstrap/bootstrap_connections.cpp b/nano/node/bootstrap/bootstrap_connections.cpp index 93e66ac55..f1ab2200f 100644 --- a/nano/node/bootstrap/bootstrap_connections.cpp +++ b/nano/node/bootstrap/bootstrap_connections.cpp @@ -76,7 +76,8 @@ std::shared_ptr nano::bootstrap_connections::connection if (result == nullptr && connections_count == 0 && new_connections_empty && attempt_a != nullptr) { node.logger.try_log (boost::str (boost::format ("Bootstrap attempt stopped because there are no peers"))); - attempt_a->stopped = true; + lock.unlock (); + attempt_a->stop (); } return result; } @@ -440,19 +441,22 @@ void nano::bootstrap_connections::requeue_pull (nano::pull_info const & pull_a, void nano::bootstrap_connections::clear_pulls (uint64_t bootstrap_id_a) { - nano::lock_guard lock (mutex); - auto i (pulls.begin ()); - while (i != pulls.end ()) { - if (i->bootstrap_id == bootstrap_id_a) + nano::lock_guard lock (mutex); + auto i (pulls.begin ()); + while (i != pulls.end ()) { - i = pulls.erase (i); - } - else - { - ++i; + if (i->bootstrap_id == bootstrap_id_a) + { + i = pulls.erase (i); + } + else + { + ++i; + } } } + condition.notify_all (); } void nano::bootstrap_connections::run () @@ -477,9 +481,11 @@ void nano::bootstrap_connections::run () void nano::bootstrap_connections::stop () { + nano::unique_lock lock (mutex); stopped = true; + lock.unlock (); condition.notify_all (); - nano::lock_guard lock (mutex); + lock.lock (); for (auto i : clients) { if (auto client = i.lock ()) diff --git a/nano/node/bootstrap/bootstrap_lazy.cpp b/nano/node/bootstrap/bootstrap_lazy.cpp index b48f3021a..ef1dc82e5 100644 --- a/nano/node/bootstrap/bootstrap_lazy.cpp +++ b/nano/node/bootstrap/bootstrap_lazy.cpp @@ -519,7 +519,7 @@ void nano::bootstrap_attempt_wallet::request_pending (nano::unique_lockbootstrap_initiator.connections->connection (shared_from_this ())); lock_a.lock (); - if (connection_l) + if (connection_l && !stopped) { auto account (wallet_accounts.front ()); wallet_accounts.pop_front (); diff --git a/nano/node/confirmation_height_processor.cpp b/nano/node/confirmation_height_processor.cpp index 42d6b85ec..f7ede3759 100644 --- a/nano/node/confirmation_height_processor.cpp +++ b/nano/node/confirmation_height_processor.cpp @@ -34,7 +34,10 @@ nano::confirmation_height_processor::~confirmation_height_processor () void nano::confirmation_height_processor::stop () { - stopped = true; + { + nano::lock_guard guard (mutex); + stopped = true; + } condition.notify_one (); if (thread.joinable ()) { diff --git a/nano/node/portmapping.hpp b/nano/node/portmapping.hpp index ccf754d45..3828785bf 100644 --- a/nano/node/portmapping.hpp +++ b/nano/node/portmapping.hpp @@ -59,7 +59,7 @@ private: boost::asio::ip::address_v4 address; std::array protocols; uint64_t check_count{ 0 }; - bool on{ false }; + std::atomic on{ false }; std::mutex mutex; }; } diff --git a/nano/node/write_database_queue.cpp b/nano/node/write_database_queue.cpp index 3378f0a78..c350f049a 100644 --- a/nano/node/write_database_queue.cpp +++ b/nano/node/write_database_queue.cpp @@ -77,6 +77,9 @@ nano::write_guard nano::write_database_queue::pop () void nano::write_database_queue::stop () { - stopped = true; + { + nano::lock_guard guard (mutex); + stopped = true; + } cv.notify_all (); } diff --git a/nano/node/write_database_queue.hpp b/nano/node/write_database_queue.hpp index b2657c00b..e18803482 100644 --- a/nano/node/write_database_queue.hpp +++ b/nano/node/write_database_queue.hpp @@ -2,7 +2,6 @@ #include -#include #include #include #include @@ -53,6 +52,6 @@ private: std::mutex mutex; nano::condition_variable cv; std::function guard_finish_callback; - std::atomic stopped{ false }; + bool stopped{ false }; }; }