From 6af28bb0f9a01fcb77739dc942f3b0ccfe808e5c Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Mon, 18 Sep 2023 15:48:56 -0500 Subject: [PATCH 1/4] GH-1639 Fix producer_plugin shutdown of read only threads to prevent SEGFAULT and deadlock. --- .../include/eosio/chain/application.hpp | 6 ++++++ .../include/eosio/chain/exec_pri_queue.hpp | 6 ++++++ plugins/producer_plugin/producer_plugin.cpp | 19 ++++++------------- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/libraries/custom_appbase/include/eosio/chain/application.hpp b/libraries/custom_appbase/include/eosio/chain/application.hpp index d0858ea358..ef46cc0934 100644 --- a/libraries/custom_appbase/include/eosio/chain/application.hpp +++ b/libraries/custom_appbase/include/eosio/chain/application.hpp @@ -79,6 +79,12 @@ class two_queue_executor { else return read_only_queue_.wrap( priority, --order_, std::forward( func)); } + + void stop() { + read_only_queue_.stop(); + read_write_queue_.stop(); + read_exclusive_queue_.stop(); + } void clear() { read_only_queue_.clear(); diff --git a/libraries/custom_appbase/include/eosio/chain/exec_pri_queue.hpp b/libraries/custom_appbase/include/eosio/chain/exec_pri_queue.hpp index 984adc8d60..06c3208d7b 100644 --- a/libraries/custom_appbase/include/eosio/chain/exec_pri_queue.hpp +++ b/libraries/custom_appbase/include/eosio/chain/exec_pri_queue.hpp @@ -13,6 +13,12 @@ class exec_pri_queue : public boost::asio::execution_context { public: + void stop() { + std::lock_guard g( mtx_ ); + exiting_blocking_ = true; + cond_.notify_all(); + } + void enable_locking(uint32_t num_threads, std::function should_exit) { assert(num_threads > 0 && num_waiting_ == 0); lock_enabled_ = true; diff --git a/plugins/producer_plugin/producer_plugin.cpp b/plugins/producer_plugin/producer_plugin.cpp index 1f0a12667c..94d5e22d30 100644 --- a/plugins/producer_plugin/producer_plugin.cpp +++ b/plugins/producer_plugin/producer_plugin.cpp @@ -1389,20 +1389,13 @@ void producer_plugin::plugin_startup() } FC_CAPTURE_AND_RETHROW() } void producer_plugin::plugin_shutdown() { - try { - my->_timer.cancel(); - } catch ( const std::bad_alloc& ) { - chain_plugin::handle_bad_alloc(); - } catch ( const boost::interprocess::bad_alloc& ) { - chain_plugin::handle_bad_alloc(); - } catch(const fc::exception& e) { - edump((e.to_detail_string())); - } catch(const std::exception& e) { - edump((fc::std_exception_wrapper::from_current_exception(e).to_detail_string())); - } - + boost::system::error_code ec; + my->_timer.cancel(ec); + boost::system::error_code ro_ec; + my->_ro_timer.cancel(ro_ec); + app().executor().stop(); + my->_ro_thread_pool.stop(); my->_thread_pool.stop(); - my->_unapplied_transactions.clear(); app().executor().post( 0, [me = my](){} ); // keep my pointer alive until queue is drained From 38545553ec19f2707a972f448bf8a064827d746f Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Wed, 4 Oct 2023 14:23:21 -0500 Subject: [PATCH 2/4] GH-1716 pop before execute as the executed lambda can switch to read-only mode and spawn threads that access the queue. --- .../include/eosio/chain/exec_pri_queue.hpp | 24 ++++++++++--------- .../tests/custom_appbase_tests.cpp | 4 ++-- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/libraries/custom_appbase/include/eosio/chain/exec_pri_queue.hpp b/libraries/custom_appbase/include/eosio/chain/exec_pri_queue.hpp index 06c3208d7b..1f06f3db9e 100644 --- a/libraries/custom_appbase/include/eosio/chain/exec_pri_queue.hpp +++ b/libraries/custom_appbase/include/eosio/chain/exec_pri_queue.hpp @@ -53,17 +53,6 @@ class exec_pri_queue : public boost::asio::execution_context handlers_ = prio_queue(); } - // only call when no lock required - bool execute_highest() - { - if( !handlers_.empty() ) { - handlers_.top()->execute(); - handlers_.pop(); - } - - return !handlers_.empty(); - } - private: // has to be defined before use, auto return type auto pop() { @@ -74,6 +63,19 @@ class exec_pri_queue : public boost::asio::execution_context public: + // only call when no lock required + bool execute_highest() + { + if( !handlers_.empty() ) { + auto t = pop(); + bool empty = handlers_.empty(); + t->execute(); + return !empty; + } + + return false; + } + bool execute_highest_locked(bool should_block) { std::unique_lock g(mtx_); if (should_block) { diff --git a/libraries/custom_appbase/tests/custom_appbase_tests.cpp b/libraries/custom_appbase/tests/custom_appbase_tests.cpp index 75ccfeacac..945c77f8a6 100644 --- a/libraries/custom_appbase/tests/custom_appbase_tests.cpp +++ b/libraries/custom_appbase/tests/custom_appbase_tests.cpp @@ -40,7 +40,7 @@ BOOST_AUTO_TEST_CASE( default_exec_window ) { app->executor().post( priority::lowest, exec_queue::read_only, [&]() { // read_only_queue should only contain the current lambda function, // and read_write_queue should have executed all its functions - BOOST_REQUIRE_EQUAL( app->executor().read_only_queue().size(), 1); // pop()s after execute + BOOST_REQUIRE_EQUAL( app->executor().read_only_queue().size(), 0); // pop()s before execute BOOST_REQUIRE_EQUAL( app->executor().read_write_queue().size(), 0 ); app->quit(); } ); @@ -175,7 +175,7 @@ BOOST_AUTO_TEST_CASE( execute_from_both_queues ) { // stop application. Use lowest at the end to make sure this executes the last app->executor().post( priority::lowest, exec_queue::read_only, [&]() { // read_queue should have current function and write_queue's functions are all executed - BOOST_REQUIRE_EQUAL( app->executor().read_only_queue().size(), 1); // pop()s after execute + BOOST_REQUIRE_EQUAL( app->executor().read_only_queue().size(), 0); // pop()s before execute BOOST_REQUIRE_EQUAL( app->executor().read_write_queue().size(), 0 ); app->quit(); } ); From c94682fad27490e0580a2ca781f53d90169e7e21 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Wed, 4 Oct 2023 17:26:30 -0500 Subject: [PATCH 3/4] Fix merge issue --- libraries/custom_appbase/include/eosio/chain/application.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/libraries/custom_appbase/include/eosio/chain/application.hpp b/libraries/custom_appbase/include/eosio/chain/application.hpp index ef46cc0934..b86209b398 100644 --- a/libraries/custom_appbase/include/eosio/chain/application.hpp +++ b/libraries/custom_appbase/include/eosio/chain/application.hpp @@ -83,7 +83,6 @@ class two_queue_executor { void stop() { read_only_queue_.stop(); read_write_queue_.stop(); - read_exclusive_queue_.stop(); } void clear() { From adc24ae6e6cb92687a9e9f4682174cbf8d58aeef Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Thu, 5 Oct 2023 07:31:47 -0500 Subject: [PATCH 4/4] Reuse same ec --- plugins/producer_plugin/producer_plugin.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/plugins/producer_plugin/producer_plugin.cpp b/plugins/producer_plugin/producer_plugin.cpp index 94d5e22d30..4812e2ab08 100644 --- a/plugins/producer_plugin/producer_plugin.cpp +++ b/plugins/producer_plugin/producer_plugin.cpp @@ -1391,8 +1391,7 @@ void producer_plugin::plugin_startup() void producer_plugin::plugin_shutdown() { boost::system::error_code ec; my->_timer.cancel(ec); - boost::system::error_code ro_ec; - my->_ro_timer.cancel(ro_ec); + my->_ro_timer.cancel(ec); app().executor().stop(); my->_ro_thread_pool.stop(); my->_thread_pool.stop();