From f088c61033a9df385dcd1961f44af97e26ccbc9b Mon Sep 17 00:00:00 2001 From: clemahieu Date: Tue, 4 May 2021 11:39:09 +0100 Subject: [PATCH 1/4] Rewriting election_scheduler::flush so it will terminate if there is no vacancy left in active_transactions or if it is stopped. Renaming election_scheduler::observe to ::notify as that's what it's doing. --- nano/core_test/election_scheduler.cpp | 27 ++++++++++++++++++++++++++- nano/node/election_scheduler.cpp | 23 +++++++++++++---------- nano/node/election_scheduler.hpp | 3 ++- nano/node/node.cpp | 2 +- 4 files changed, 42 insertions(+), 13 deletions(-) diff --git a/nano/core_test/election_scheduler.cpp b/nano/core_test/election_scheduler.cpp index 31d1d21d95..9ad6686590 100644 --- a/nano/core_test/election_scheduler.cpp +++ b/nano/core_test/election_scheduler.cpp @@ -79,8 +79,11 @@ TEST (election_scheduler, no_vacancy) .work (*system.work.generate (key.pub)) .build_shared (); ASSERT_EQ (nano::process_result::progress, node.process (*send).code); + nano::blocks_confirm (node, { send }, true); + ASSERT_TIMELY (1s, node.active.empty ()); ASSERT_EQ (nano::process_result::progress, node.process (*receive).code); - nano::blocks_confirm (node, { send, receive }, true); + nano::blocks_confirm (node, { receive }, true); + ASSERT_TIMELY (1s, node.active.empty ()); // Second, process two eligble transactions auto block0 = builder.make_block () @@ -118,3 +121,25 @@ TEST (election_scheduler, no_vacancy) auto election4 = node.active.election (block1->qualified_root ()); ASSERT_NE (nullptr, election4); } + +TEST (election_scheduler, flush_vacancy) +{ + nano::system system; + nano::node_config config{ nano::get_available_port (), system.logging }; + config.active_elections_size = 0; + auto & node = *system.add_node (config); + nano::state_block_builder builder; + nano::keypair key; + + // Activating accounts depends on confirmed dependencies. First, prepare 2 accounts + auto send = builder.make_block () + .account (nano::dev_genesis_key.pub) + .previous (nano::genesis_hash) + .representative (nano::dev_genesis_key.pub) + .link (key.pub) + .balance (nano::genesis_amount - nano::Gxrb_ratio) + .sign (nano::dev_genesis_key.prv, nano::dev_genesis_key.pub) + .work (*system.work.generate (nano::genesis_hash)) + .build_shared (); + node.scheduler.manual (send); +} diff --git a/nano/node/election_scheduler.cpp b/nano/node/election_scheduler.cpp index 039ea9257a..71a9991ad7 100644 --- a/nano/node/election_scheduler.cpp +++ b/nano/node/election_scheduler.cpp @@ -18,7 +18,7 @@ void nano::election_scheduler::manual (std::shared_ptr const & bloc { nano::lock_guard lock{ mutex }; manual_queue.push_back (std::make_tuple (block_a, previous_balance_a, election_behavior_a, confirmation_action_a)); - observe (); + notify (); } void nano::election_scheduler::activate (nano::account const & account_a, nano::transaction const & transaction) @@ -39,7 +39,7 @@ void nano::election_scheduler::activate (nano::account const & account_a, nano:: { nano::lock_guard lock{ mutex }; priority.push (account_info.modified, block); - observe (); + notify (); } } } @@ -49,20 +49,18 @@ void nano::election_scheduler::stop () { nano::unique_lock lock{ mutex }; stopped = true; - observe (); + notify (); } void nano::election_scheduler::flush () { nano::unique_lock lock{ mutex }; - auto priority_target = priority_queued + priority.size (); - auto manual_target = manual_queued + manual_queue.size (); - condition.wait (lock, [this, &priority_target, &manual_target] () { - return priority_queued >= priority_target && manual_queued >= manual_target; + condition.wait (lock, [this] () { + return stopped || empty_locked () || node.active.vacancy () <= 0; }); } -void nano::election_scheduler::observe () +void nano::election_scheduler::notify () { condition.notify_all (); } @@ -73,10 +71,15 @@ size_t nano::election_scheduler::size () const return priority.size () + manual_queue.size (); } +bool nano::election_scheduler::empty_locked () const +{ + return priority.empty () && manual_queue.empty (); +} + bool nano::election_scheduler::empty () const { nano::lock_guard lock{ mutex }; - return priority.empty () && manual_queue.empty (); + return empty_locked (); } size_t nano::election_scheduler::priority_queue_size () const @@ -120,7 +123,7 @@ void nano::election_scheduler::run () manual_queue.pop_front (); ++manual_queued; } - observe (); + notify (); } } } diff --git a/nano/node/election_scheduler.hpp b/nano/node/election_scheduler.hpp index 02544bfc55..873d8f76a2 100644 --- a/nano/node/election_scheduler.hpp +++ b/nano/node/election_scheduler.hpp @@ -27,13 +27,14 @@ class election_scheduler final void activate (nano::account const &, nano::transaction const &); void stop (); void flush (); - void observe (); + void notify (); size_t size () const; bool empty () const; size_t priority_queue_size () const; private: void run (); + bool empty_locked () const; nano::prioritization priority; uint64_t priority_queued{ 0 }; std::deque, boost::optional, nano::election_behavior, std::function)>>> manual_queue; diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 7aa8ad8aa0..d7f4808b37 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -127,7 +127,7 @@ nano::node::node (boost::asio::io_context & io_ctx_a, boost::filesystem::path co { telemetry->start (); - active.vacancy_update = [this] () { scheduler.observe (); }; + active.vacancy_update = [this] () { scheduler.notify (); }; if (config.websocket_config.enabled) { From 46a6edcbf48129d6995e336632af75eeb998aa7a Mon Sep 17 00:00:00 2001 From: clemahieu Date: Thu, 6 May 2021 15:27:48 +0100 Subject: [PATCH 2/4] Adding comments and directly calling flush to test it does not hang. --- nano/core_test/election_scheduler.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/nano/core_test/election_scheduler.cpp b/nano/core_test/election_scheduler.cpp index 9ad6686590..0b3fbbb3a3 100644 --- a/nano/core_test/election_scheduler.cpp +++ b/nano/core_test/election_scheduler.cpp @@ -122,16 +122,17 @@ TEST (election_scheduler, no_vacancy) ASSERT_NE (nullptr, election4); } +// Ensure that election_scheduler::flush terminates even if no elections can currently be queued e.g. shutdown or no active_transactions vacancy TEST (election_scheduler, flush_vacancy) { nano::system system; nano::node_config config{ nano::get_available_port (), system.logging }; + // No elections can be activated config.active_elections_size = 0; auto & node = *system.add_node (config); nano::state_block_builder builder; nano::keypair key; - // Activating accounts depends on confirmed dependencies. First, prepare 2 accounts auto send = builder.make_block () .account (nano::dev_genesis_key.pub) .previous (nano::genesis_hash) @@ -142,4 +143,6 @@ TEST (election_scheduler, flush_vacancy) .work (*system.work.generate (nano::genesis_hash)) .build_shared (); node.scheduler.manual (send); + // Ensure this call does not block, even though no elections can be activated. + node.scheduler.flush (); } From 6ec763758dcb09a4ba005867e755944ae68a3451 Mon Sep 17 00:00:00 2001 From: clemahieu Date: Thu, 6 May 2021 16:32:28 +0100 Subject: [PATCH 3/4] Adding expected state checks at the end of the test. --- nano/core_test/election_scheduler.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/nano/core_test/election_scheduler.cpp b/nano/core_test/election_scheduler.cpp index 0b3fbbb3a3..98ba24993b 100644 --- a/nano/core_test/election_scheduler.cpp +++ b/nano/core_test/election_scheduler.cpp @@ -145,4 +145,6 @@ TEST (election_scheduler, flush_vacancy) node.scheduler.manual (send); // Ensure this call does not block, even though no elections can be activated. node.scheduler.flush (); + ASSERT_EQ (0, node.active.size ()); + ASSERT_EQ (1, node.scheduler.size ()); } From bd280296f1b475ba186b67208792cb46d4a6c777 Mon Sep 17 00:00:00 2001 From: clemahieu Date: Thu, 6 May 2021 16:36:48 +0100 Subject: [PATCH 4/4] Adding election_scheduler::flush comment. --- nano/node/election_scheduler.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/nano/node/election_scheduler.hpp b/nano/node/election_scheduler.hpp index 873d8f76a2..8baf7a7e10 100644 --- a/nano/node/election_scheduler.hpp +++ b/nano/node/election_scheduler.hpp @@ -26,6 +26,7 @@ class election_scheduler final // Activates the first unconfirmed block of \p account_a void activate (nano::account const &, nano::transaction const &); void stop (); + // Blocks until no more elections can be activated or there are no more elections to activate void flush (); void notify (); size_t size () const;