From ee4e4b0db6b0b5759bdc36be6d99a4c61ad6e8a2 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Thu, 25 May 2023 11:13:37 -0400 Subject: [PATCH 01/15] use appbase branch which calls `plugin_shutdown()` on exception. --- libraries/appbase | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/appbase b/libraries/appbase index 54cc7fb4f9..be400afa16 160000 --- a/libraries/appbase +++ b/libraries/appbase @@ -1 +1 @@ -Subproject commit 54cc7fb4f9a3dbbe2c71bdf23f9342c9c01b9673 +Subproject commit be400afa16bebc27a633cd7f9896cb8ad442294c From ee29899e1af8ce7b7bab39edc2c2dbb3c67a64da Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Thu, 25 May 2023 14:26:28 -0400 Subject: [PATCH 02/15] update appbase to new tip --- libraries/appbase | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/appbase b/libraries/appbase index be400afa16..65bb05604e 160000 --- a/libraries/appbase +++ b/libraries/appbase @@ -1 +1 @@ -Subproject commit be400afa16bebc27a633cd7f9896cb8ad442294c +Subproject commit 65bb05604e90bbf62d9defb23f73bebb41903688 From 3812d5b026ece05f082dcb398e7797a3defce45c Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Fri, 26 May 2023 15:43:46 -0400 Subject: [PATCH 03/15] Do not destroy `chain::controller` on `plugin_shutdown()`. as other plugins could possibly not be shutdown yet. --- plugins/chain_plugin/chain_plugin.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/plugins/chain_plugin/chain_plugin.cpp b/plugins/chain_plugin/chain_plugin.cpp index f073454d30..32287b53d2 100644 --- a/plugins/chain_plugin/chain_plugin.cpp +++ b/plugins/chain_plugin/chain_plugin.cpp @@ -1129,7 +1129,6 @@ void chain_plugin::plugin_shutdown() { my->block_start_connection.reset(); if(app().is_quiting()) my->chain->get_wasm_interface().indicate_shutting_down(); - my->chain.reset(); } void chain_plugin::handle_sighup() { From 3210dfe48b12223f73eb96cc6b4d0dbebb11afb5 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Fri, 26 May 2023 15:45:54 -0400 Subject: [PATCH 04/15] No need to catch exceptions to make sure `plugin_shutdown()` is executed. This is now ensured with the current version of appbase. --- plugins/net_plugin/net_plugin.cpp | 8 -------- 1 file changed, 8 deletions(-) diff --git a/plugins/net_plugin/net_plugin.cpp b/plugins/net_plugin/net_plugin.cpp index dfa1e47d85..9113957297 100644 --- a/plugins/net_plugin/net_plugin.cpp +++ b/plugins/net_plugin/net_plugin.cpp @@ -3819,8 +3819,6 @@ namespace eosio { } void net_plugin::plugin_startup() { - try { - fc_ilog( logger, "my node_id is ${id}", ("id", my->node_id )); my->producer_plug = app().find_plugin(); @@ -3900,12 +3898,6 @@ namespace eosio { my->update_chain_info(); my->connections.connect_supplied_peers(); }); - - } catch( ... ) { - // always want plugin_shutdown even on exception - plugin_shutdown(); - throw; - } } void net_plugin::handle_sighup() { From 81e5a810743d4d5b8a37db056dee1c9617ec7ea7 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Fri, 26 May 2023 15:46:47 -0400 Subject: [PATCH 05/15] Whitespace fix --- plugins/trace_api_plugin/trace_api_plugin.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/trace_api_plugin/trace_api_plugin.cpp b/plugins/trace_api_plugin/trace_api_plugin.cpp index 6eee7a93f4..d2b664eeee 100644 --- a/plugins/trace_api_plugin/trace_api_plugin.cpp +++ b/plugins/trace_api_plugin/trace_api_plugin.cpp @@ -151,7 +151,7 @@ struct trace_api_common_impl { } void plugin_startup() { - store->start_maintenance_thread([](const std::string& msg ){ + store->start_maintenance_thread([](const std::string& msg) { fc_dlog( _log, msg ); }); } From a2157c4d7967a368f2e76b7a2efff1f1835cf936 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Fri, 26 May 2023 15:47:10 -0400 Subject: [PATCH 06/15] In `plugin_startup()`, in case of error, better `throw` than call `app().quit()` --- .../state_history_plugin.cpp | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/plugins/state_history_plugin/state_history_plugin.cpp b/plugins/state_history_plugin/state_history_plugin.cpp index f783693193..bda0b895e6 100644 --- a/plugins/state_history_plugin/state_history_plugin.cpp +++ b/plugins/state_history_plugin/state_history_plugin.cpp @@ -381,23 +381,19 @@ void state_history_plugin::plugin_initialize(const variables_map& options) { } // state_history_plugin::plugin_initialize void state_history_plugin::plugin_startup() { - try { - auto bsp = my->chain_plug->chain().head_block_state(); - if( bsp && my->chain_state_log && my->chain_state_log->empty() ) { - fc_ilog( _log, "Storing initial state on startup, this can take a considerable amount of time" ); - my->store_chain_state( bsp ); - fc_ilog( _log, "Done storing initial state on startup" ); - } - my->listen(); - // use of executor assumes only one thread - my->thread_pool.start( 1, [](const fc::exception& e) { - fc_elog( _log, "Exception in SHiP thread pool, exiting: ${e}", ("e", e.to_detail_string()) ); - app().quit(); - }); - my->plugin_started = true; - } catch (std::exception& ex) { - appbase::app().quit(); + auto bsp = my->chain_plug->chain().head_block_state(); + if( bsp && my->chain_state_log && my->chain_state_log->empty() ) { + fc_ilog( _log, "Storing initial state on startup, this can take a considerable amount of time" ); + my->store_chain_state( bsp ); + fc_ilog( _log, "Done storing initial state on startup" ); } + my->listen(); + // use of executor assumes only one thread + my->thread_pool.start( 1, [](const fc::exception& e) { + fc_elog( _log, "Exception in SHiP thread pool, exiting: ${e}", ("e", e.to_detail_string()) ); + app().quit(); + }); + my->plugin_started = true; } void state_history_plugin::plugin_shutdown() { From dc72da4e58d885a8d50f096fab8f4c70d90b6c1e Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Thu, 1 Jun 2023 11:30:18 -0400 Subject: [PATCH 07/15] Remove unnecessary (now) try/catch block calling `plugin_shutdown()` --- plugins/producer_plugin/producer_plugin.cpp | 139 ++++++++++---------- 1 file changed, 66 insertions(+), 73 deletions(-) diff --git a/plugins/producer_plugin/producer_plugin.cpp b/plugins/producer_plugin/producer_plugin.cpp index 81267e8c2f..6aaa9648f8 100644 --- a/plugins/producer_plugin/producer_plugin.cpp +++ b/plugins/producer_plugin/producer_plugin.cpp @@ -1024,9 +1024,9 @@ void producer_plugin_impl::plugin_initialize(const boost::program_options::varia chain_plug = app().find_plugin(); EOS_ASSERT(chain_plug, plugin_config_exception, "chain_plugin not found" ); _options = &options; - LOAD_VALUE_SET(options, "producer-name", _producers) + LOAD_VALUE_SET(options, "producer-name", _producers); - chain::controller& chain = chain_plug->chain(); + chain::controller& chain = chain_plug->chain(); if (options.count("signature-provider")) { const std::vector key_spec_pairs = options["signature-provider"].as>(); @@ -1277,93 +1277,86 @@ void producer_plugin::plugin_initialize(const boost::program_options::variables_ using namespace std::chrono_literals; void producer_plugin_impl::plugin_startup() { try { - try { - ilog("producer plugin: plugin_startup() begin"); + ilog("producer plugin: plugin_startup() begin"); - _thread_pool.start(_thread_pool_size, [](const fc::exception& e) { - fc_elog(_log, "Exception in producer thread pool, exiting: ${e}", ("e", e.to_detail_string())); - app().quit(); - }); + _thread_pool.start(_thread_pool_size, [](const fc::exception& e) { + fc_elog(_log, "Exception in producer thread pool, exiting: ${e}", ("e", e.to_detail_string())); + app().quit(); + }); - chain::controller& chain = chain_plug->chain(); - EOS_ASSERT(_producers.empty() || chain.get_read_mode() != chain::db_read_mode::IRREVERSIBLE, plugin_config_exception, - "node cannot have any producer-name configured because block production is impossible when read_mode is \"irreversible\""); + chain::controller& chain = chain_plug->chain(); + EOS_ASSERT(_producers.empty() || chain.get_read_mode() != chain::db_read_mode::IRREVERSIBLE, plugin_config_exception, + "node cannot have any producer-name configured because block production is impossible when read_mode is \"irreversible\""); - EOS_ASSERT(_producers.empty() || chain.get_validation_mode() == chain::validation_mode::FULL, plugin_config_exception, - "node cannot have any producer-name configured because block production is not safe when validation_mode is not \"full\""); + EOS_ASSERT(_producers.empty() || chain.get_validation_mode() == chain::validation_mode::FULL, plugin_config_exception, + "node cannot have any producer-name configured because block production is not safe when validation_mode is not \"full\""); - EOS_ASSERT(_producers.empty() || chain_plug->accept_transactions(), plugin_config_exception, - "node cannot have any producer-name configured because no block production is possible with no [api|p2p]-accepted-transactions"); + EOS_ASSERT(_producers.empty() || chain_plug->accept_transactions(), plugin_config_exception, + "node cannot have any producer-name configured because no block production is possible with no [api|p2p]-accepted-transactions"); - _accepted_block_connection.emplace(chain.accepted_block.connect([this](const auto& bsp) { on_block(bsp); })); - _accepted_block_header_connection.emplace(chain.accepted_block_header.connect([this](const auto& bsp) { on_block_header(bsp); })); - _irreversible_block_connection.emplace( - chain.irreversible_block.connect([this](const auto& bsp) { on_irreversible_block(bsp->block); })); + _accepted_block_connection.emplace(chain.accepted_block.connect([this](const auto& bsp) { on_block(bsp); })); + _accepted_block_header_connection.emplace(chain.accepted_block_header.connect([this](const auto& bsp) { on_block_header(bsp); })); + _irreversible_block_connection.emplace(chain.irreversible_block.connect([this](const auto& bsp) { on_irreversible_block(bsp->block); })); - _block_start_connection.emplace(chain.block_start.connect([this, &chain](uint32_t bs) { - try { - _snapshot_scheduler.on_start_block(bs, chain); - } catch (const snapshot_execution_exception& e) { - fc_elog(_log, "Exception during snapshot execution: ${e}", ("e", e.to_detail_string())); - app().quit(); - } - })); - - const auto lib_num = chain.last_irreversible_block_num(); - const auto lib = chain.fetch_block_by_number(lib_num); - if (lib) { - on_irreversible_block(lib); - } else { - _irreversible_block_time = fc::time_point::maximum(); + _block_start_connection.emplace(chain.block_start.connect([this, &chain](uint32_t bs) { + try { + _snapshot_scheduler.on_start_block(bs, chain); + } catch (const snapshot_execution_exception& e) { + fc_elog(_log, "Exception during snapshot execution: ${e}", ("e", e.to_detail_string())); + app().quit(); } + })); - if (!_producers.empty()) { - ilog("Launching block production for ${n} producers at ${time}.", ("n", _producers.size())("time", fc::time_point::now())); + const auto lib_num = chain.last_irreversible_block_num(); + const auto lib = chain.fetch_block_by_number(lib_num); + if (lib) { + on_irreversible_block(lib); + } else { + _irreversible_block_time = fc::time_point::maximum(); + } - if (_production_enabled) { - if (chain.head_block_num() == 0) { - new_chain_banner(chain); - } - } - } + if (!_producers.empty()) { + ilog("Launching block production for ${n} producers at ${time}.", ("n", _producers.size())("time", fc::time_point::now())); - if (_ro_thread_pool_size > 0) { - std::atomic num_threads_started = 0; - _ro_thread_pool.start( - _ro_thread_pool_size, - [](const fc::exception& e) { - fc_elog(_log, "Exception in read-only thread pool, exiting: ${e}", ("e", e.to_detail_string())); - app().quit(); - }, - [&]() { - chain.init_thread_local_data(); - ++num_threads_started; - }); - - // This will be changed with std::latch or std::atomic<>::wait - // when C++20 is used. - auto time_slept_ms = 0; - constexpr auto max_time_slept_ms = 1000; - while (num_threads_started.load() < _ro_thread_pool_size && time_slept_ms < max_time_slept_ms) { - std::this_thread::sleep_for(1ms); - ++time_slept_ms; + if (_production_enabled) { + if (chain.head_block_num() == 0) { + new_chain_banner(chain); } - EOS_ASSERT(num_threads_started.load() == _ro_thread_pool_size, producer_exception, - "read-only threads failed to start. num_threads_started: ${n}, time_slept_ms: ${t}ms", - ("n", num_threads_started.load())("t", time_slept_ms)); - - start_write_window(); } + } - schedule_production_loop(); + if (_ro_thread_pool_size > 0) { + std::atomic num_threads_started = 0; + _ro_thread_pool.start( + _ro_thread_pool_size, + [](const fc::exception& e) { + fc_elog(_log, "Exception in read-only thread pool, exiting: ${e}", ("e", e.to_detail_string())); + app().quit(); + }, + [&]() { + chain.init_thread_local_data(); + ++num_threads_started; + }); + + // This will be changed with std::latch or std::atomic<>::wait + // when C++20 is used. + auto time_slept_ms = 0; + constexpr auto max_time_slept_ms = 1000; + while (num_threads_started.load() < _ro_thread_pool_size && time_slept_ms < max_time_slept_ms) { + std::this_thread::sleep_for(1ms); + ++time_slept_ms; + } + EOS_ASSERT(num_threads_started.load() == _ro_thread_pool_size, producer_exception, + "read-only threads failed to start. num_threads_started: ${n}, time_slept_ms: ${t}ms", + ("n", num_threads_started.load())("t", time_slept_ms)); - ilog("producer plugin: plugin_startup() end"); - } catch (...) { - // always call plugin_shutdown, even on exception - plugin_shutdown(); - throw; + start_write_window(); } + + schedule_production_loop(); + + ilog("producer plugin: plugin_startup() end"); } FC_CAPTURE_AND_RETHROW() } From a4f54a29cd7274df6738bf8c993fae83e94548a3 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Mon, 5 Jun 2023 13:50:27 -0400 Subject: [PATCH 08/15] Add `usage_patterns.md` file --- plugins/usage_pattern.md | 196 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 196 insertions(+) create mode 100644 plugins/usage_pattern.md diff --git a/plugins/usage_pattern.md b/plugins/usage_pattern.md new file mode 100644 index 0000000000..7ec56e6afb --- /dev/null +++ b/plugins/usage_pattern.md @@ -0,0 +1,196 @@ +## Rules and considerations for writing an appbase application + +### Appbase application lifetime + +#### Plugin registration + +* Register plugins by calling `appbase::application::register_plugin();`. This needs to be done only once per executable. +* These plugins, and their dependents, will get registered when `initialize` is called on `appbase::scoped_app` +* plugin registration is required for every plugin we want to use. + +#### Plugin initialization + +* Precondition: plugins must have been registered +* Happens when `appbase::scoped_app::initialize(argc, argv)` is called +* Plugins to be initialized are those passed as options (using the `--plugin` command line option) or as template parameters to the `initialize()` call, as well as their dependents. +* if we try to initialize an unregistered plugin, we get an exception (`std::exception::what: unable to find plugin`). +* initialization order: + - for each plugin specified in command line, initialize its dependents (depth first) and then itself + - for each plugin specified as template parameter, initialize its dependents (depth first) and then itself +* Plugin state is updated to `initialized` *before* its descendents and itself are initialized. + So if there was a dependency cycle, the same plugin would not be initialized twice. +* After the plugin initialization, `app().plugin_initialized(this);` is called which allows appbase to add it to its list of initialized plugins. + + +#### Application initialization + +Currently, `appbase::initialize()` can: + +1. either return `false` + - if a specified option like `--help` or `--version` is specified, which does not require to continue the program execution + - if a configuration error is detected (for ex `--logconf` specified dir missing, `--config` file missing) +2. or throw in case of failure + - if any plugin throws during `plugin_initialize`, `appbase::initialize()` logs the error and rethrows the exception +3. or return `true` is initialization was successful and the application should proceed. + + +So an application should exit cleanly in cases 1 and 2 (without calling `app->startup();` or `app->exec();`), and continue in case 3. + +Unfortunately, when `appbase::initialize()` returns false, it is not clear whether it indicates a normal exit or an error, so for example in `nodeos` we do: + + +```c++ + try { + ... + if(!app->initialize(argc, argv, initialize_logging)) { + const auto& opts = app->get_options(); + if( opts.count("help") || opts.count("version") || opts.count("full-version") || opts.count("print-default-config") ) { + return SUCCESS; + } + return INITIALIZE_FAIL; + } + ... + app->startup(); + app->exec(); + } + // other catch blocks + } catch( ... ) { + elog("unknown exception"); + return OTHER_FAIL; + } +``` + +> Note that in the code above, we check the return value of `app->initialize()` (as we should) and we have a try/catch block in case `app->initialize()` (or any of the plugins) throws an exception + +#### Application Startup + +This is when `appbase::scoped_app::startup()` is called. Besides starting a thread for catching signals, the main task is to call `startup() on all initialized plugins. + + +```c++ + try { + for( auto plugin : initialized_plugins ) { + if( is_quiting() ) break; + plugin->startup(); + } + + } catch( ... ) { + clean_up_signal_thread(); + shutdown(); + throw; + } +``` + +If any plugin throws during plugin_startup, the startup process stops and an orderly shutdown is initiated, where all started plugins (including the one that threw) will be invoked with plugin_shutdown. After the orderly shutdown of all started plugins, the exception is rethrown, and it is expected that this exception will cause the application to terminate without calling `app->exec()`. + +If a plugin calls `app().quit()` during plugin_startup (not recommended), the appbase framework will detect it and throw an exception, so we fall back to the same processing as in the above paragraph. + +> We note that plugins are also started in a similar way as C++ objects are constructed, where the dependent plugins are started first, in a depth first fashion. Just like C++, shutdown proceeds in the reverse order. + + +## Rules and considerations for writing plugins + + +### Initialize + +#### Typical actions in `plugin_initialize` + +- get references (using `app().find_plugin()`) to other plugins you depend on, and maybe store a reference to them. + Those dependencies should have been specified in the plugin class definition using `APPBASE_PLUGIN_REQUIRES` +- use the provided program options to configure the plugin +- possibly read additional configuration from files +- create new objects, directories, files, etc... + critically any deallocation/cleanup of these (or other objects created during the plugin execution) should be done in the plugin destructor, not in `plugin_shutdown()`. +- connect to signals from other plugins (ex: `chain.applied_transaction.connect([this](...) { ... });`) +- register provider methods (that others can connect to via signals). + Ex: `app().get_method().register_provider([this](...) { ... });` +- relay signals to channels. + Ex: `pre_accepted_block_connection = chain->pre_accepted_block.connect([this](...) { ...; pre_accepted_block_channel.publish(...); });` + +#### Rules to follow in `plugin_initialize` + +- in case of irrecoverable error, appropriate action is to throw an exception (possibly after logging the error). + This will cause the application to terminate. +- `io_context`s should not be running, and threads should not be started that would run on any `io_context` before `plugin_startup`. +- plugins should not post any action involving callbacks (timer, io, etc), either on the main appbbase thread or on plugin-specific thread pools, during `plugin_initialize`. + +#### Guarantees provided by appbase to `plugin_initialize` + +- dependent plugins (as specified by `APPBASE_PLUGIN_REQUIRES`) are always initialized before the plugins that depends on them. +- if any plugin throws during `plugin_initialize`, the plugin initialization process stops immediately, an error is logged, and the exception is rethrown. `plugin_shutdown` is not called for any plugin, the expectation being that any object or resource that was allocated during `plugin_initialize` will be freed when the plugin destructor is executed. + + + +### Startup + +`plugin_startup()` is intended to allow the plugins to activate their `io_context`s, thread pools, timers, establish connections with dependent plugins, and start the intended processing of the plugin, in order to make sure that all the resources necessary are available. If critical resources required for the application correct functioning are not available, the plugin should throw an exception. + +Ideally, all resource allocation is completed before `plugin_startup` returns. + +If the plugin was to start a thread, which allocates resources upon startup, this could trigger an exception while another plugin is executing `plugin_startup`, making the log harder to decipher as it would intermingle messages from different sources. + +> in some case, a delayed startup is desired, because we would like other plugins to finish their startup before we start our own services (for example the http_plugin may want to start listening for API requests only after the blockchain replay is completed). One way to achieve this is to post a lambda implementing the plugin startup on the appbase queue, as in: `app().executor().post(appbase::priority::high, [this] () { ... });` + +#### Typical actions in `plugin_startup` + +Example of actions in `plugin_startup`: + +- an `io_context`, task queue and a main thread is provided by appbase, and plugins can schedule tasks to be executed on it using `app().executor().post()`. However, if necessary, plugins can create their own `io_context` and thread pools, and `plugin_startup()` is an appropriate time to activate these. +- if the plugin starts its own thread pool, and these threads allocate resources upon startup, it is recommented for the plugin to wait till all the threads are ready to accept work (using a synchronization primitive such as a `condition_variable`) before returning from `plugin_startup`. +- a plugin which communicates with other nodes may want to open network connections, or start listening for incoming connections. + + +#### Rules to follow in `plugin_startup` + +- in case of an error during `plugin_startup()`, the plugin should throw, not call `app().quit()` +- any code in `plugin_shutdown()` should not depend on `plugin_startup` having fully executed. + +#### Guarantees provided by appbase to `plugin_startup` + +- `plugin_startup()` is called in a depth-first fashion, according to the stated dependency graph specified by `APPBASE_PLUGIN_REQUIRES`. +- `plugin_startup()` is called within a try/catch block. +- If an exception is thrown while `plugin_startup()` is executed on any plugin, `plugin_shutdown()` is called on it immediately, and an application shutdown is triggerred. +- but if the plugin does call `app().quit()` during `plugin_startup()`, the framework will throw, ensuring no other plugin is started and application terminates cleanly. +- Before `plugin_startup()` is called, the plugin is added to the list of running plugins, which ensures that `plugin_shutdown()` will be called on it (either at application shutdown, or in case of an exception occuring before that) +- because `plugin_shutdown()` will be executed regardless of whether `plugin_startup()` fully (or at all) executed, it is critical that `plugin_shutdown()`'s actions do not require or expect anything from `plugin_startup()`. For example, calling `cancel()` on a timer that has not been `async_wait`'ed on is fine. + + + +### Shutdown + +#### Typical actions in `plugin_shutdown` + +`plugin_shutdown()` is intended to allow the plugins to terminate its processing, for example: + +- cancel timers (typically `boost::asio::steady_timer` or `boost::asio::deadline_timer` +- stop thread pools (stop the `io_context`, join all threads) +- disconnect from other plugins if needed (this is probably not needed). + +#### Rules to follow in `plugin_shutdown` + +- plugin created objects, or the plugin itself, should *not* be destroyed during `plugin_shutdown()`. + This must be done in the plugin destructor. + +#### Guarantees provided by appbase to `plugin_shutdown` + +- `plugin_shutdown()` is called within a try/catch block for each running plugin, starting from the last registered first. + This means that `plugin_shutdown()` is called on the inverse order of `plugin_startup()`, +- If an exception is thrown during `plugin_shutdown()`, it is logged on `std::cerr`, but doesn't prevent `plugin_shutdown()` from being called on the remaining running plugins. +- The appbase framework will destruct the running plugins in a second phase of shutdown (last one added to the running list is destructed first). + + +## Action items + +1. If a plugin (or one of its dependents) throws during `startup()`, `plugin_shutdown()` should be called on it. + => update producer_plugin and other plugins which may (needlessly) call `plugin_shutdown()` when there is an exception in `plugin_startup()` + **done** [appbase 65bb056] ensure plugin_shutdown called when plugin_shutdown throws. + [leap/gh-672 3210dfe48] net_plugin: no need to catch exceptions to make sure `plugin_shutdown()` is executed. + [leap/gh-672 dc72da4e5] producer_plugin: remove unnecessary try/catch block calling `plugin_shutdown()` + +2. many appbase apps do not check the return value of `app->initialize()` or catch exceptions. + +3. have `app->initialize()` return `enum class result { all_done, success, failure }` + + + + From 4224df058801db4d018b3254d550efe127a0b1f2 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Mon, 5 Jun 2023 16:26:19 -0400 Subject: [PATCH 09/15] Update `usage_pattern.md` --- plugins/usage_pattern.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/plugins/usage_pattern.md b/plugins/usage_pattern.md index 7ec56e6afb..f46983e415 100644 --- a/plugins/usage_pattern.md +++ b/plugins/usage_pattern.md @@ -183,13 +183,16 @@ Example of actions in `plugin_startup`: 1. If a plugin (or one of its dependents) throws during `startup()`, `plugin_shutdown()` should be called on it. => update producer_plugin and other plugins which may (needlessly) call `plugin_shutdown()` when there is an exception in `plugin_startup()` + +``` **done** [appbase 65bb056] ensure plugin_shutdown called when plugin_shutdown throws. [leap/gh-672 3210dfe48] net_plugin: no need to catch exceptions to make sure `plugin_shutdown()` is executed. [leap/gh-672 dc72da4e5] producer_plugin: remove unnecessary try/catch block calling `plugin_shutdown()` - +``` + 2. many appbase apps do not check the return value of `app->initialize()` or catch exceptions. -3. have `app->initialize()` return `enum class result { all_done, success, failure }` +3. have `app->initialize()` return `enum class result { all_done, success, failure }`, so an application knows whether to just return or contunue with `app->startup();`. From 48be9385ba380d219a5adad589c2d31340949ceb Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Mon, 5 Jun 2023 16:46:32 -0400 Subject: [PATCH 10/15] fix typo --- plugins/usage_pattern.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/usage_pattern.md b/plugins/usage_pattern.md index f46983e415..a6ffa5c87f 100644 --- a/plugins/usage_pattern.md +++ b/plugins/usage_pattern.md @@ -192,7 +192,7 @@ Example of actions in `plugin_startup`: 2. many appbase apps do not check the return value of `app->initialize()` or catch exceptions. -3. have `app->initialize()` return `enum class result { all_done, success, failure }`, so an application knows whether to just return or contunue with `app->startup();`. +3. have `app->initialize()` return `enum class result { all_done, success, failure }`, so an application knows whether to just return or continue with `app->startup();`. From 727175d5125d721b497691d42489553e78e71d9c Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Thu, 8 Jun 2023 16:05:48 -0400 Subject: [PATCH 11/15] Fix merge issue --- plugins/producer_plugin/producer_plugin.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/plugins/producer_plugin/producer_plugin.cpp b/plugins/producer_plugin/producer_plugin.cpp index 1ac3381ac8..1ac2ee7fda 100644 --- a/plugins/producer_plugin/producer_plugin.cpp +++ b/plugins/producer_plugin/producer_plugin.cpp @@ -1490,7 +1490,18 @@ void producer_plugin::create_snapshot(producer_plugin::next_functionchain_plug->chain(); + const auto head_block_num = chain.head_block_num(); + + // missing start/end is set to head block num, missing end to UINT32_MAX + chain::snapshot_scheduler::snapshot_request_information sri = { + .block_spacing = srp.block_spacing ? *srp.block_spacing : 0, + .start_block_num = srp.start_block_num ? *srp.start_block_num : head_block_num + 1, + .end_block_num = srp.end_block_num ? *srp.end_block_num : std::numeric_limits::max(), + .snapshot_description = srp.snapshot_description ? *srp.snapshot_description : "" + }; + return my->_snapshot_scheduler.schedule_snapshot(sri); } From 52911085733fb5206b40a3ace03ab13246ab66ac Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Mon, 12 Jun 2023 16:50:32 -0400 Subject: [PATCH 12/15] Fix incorrect merge --- plugins/producer_plugin/producer_plugin.cpp | 103 ++++++++++---------- 1 file changed, 52 insertions(+), 51 deletions(-) diff --git a/plugins/producer_plugin/producer_plugin.cpp b/plugins/producer_plugin/producer_plugin.cpp index cd59c35422..dfffe9a37a 100644 --- a/plugins/producer_plugin/producer_plugin.cpp +++ b/plugins/producer_plugin/producer_plugin.cpp @@ -1277,72 +1277,73 @@ void producer_plugin::plugin_initialize(const boost::program_options::variables_ using namespace std::chrono_literals; void producer_plugin_impl::plugin_startup() { try { - ilog("producer plugin: plugin_startup() begin"); + ilog("producer plugin: plugin_startup() begin"); - _thread_pool.start(_thread_pool_size, [](const fc::exception& e) { - fc_elog(_log, "Exception in producer thread pool, exiting: ${e}", ("e", e.to_detail_string())); - app().quit(); - }); + _thread_pool.start(_thread_pool_size, [](const fc::exception& e) { + fc_elog(_log, "Exception in producer thread pool, exiting: ${e}", ("e", e.to_detail_string())); + app().quit(); + }); - chain::controller& chain = chain_plug->chain(); - EOS_ASSERT(_producers.empty() || chain.get_read_mode() != chain::db_read_mode::IRREVERSIBLE, plugin_config_exception, - "node cannot have any producer-name configured because block production is impossible when read_mode is \"irreversible\""); + chain::controller& chain = chain_plug->chain(); + EOS_ASSERT(_producers.empty() || chain.get_read_mode() != chain::db_read_mode::IRREVERSIBLE, plugin_config_exception, + "node cannot have any producer-name configured because block production is impossible when read_mode is \"irreversible\""); - EOS_ASSERT(_producers.empty() || chain.get_validation_mode() == chain::validation_mode::FULL, plugin_config_exception, - "node cannot have any producer-name configured because block production is not safe when validation_mode is not \"full\""); + EOS_ASSERT(_producers.empty() || chain.get_validation_mode() == chain::validation_mode::FULL, plugin_config_exception, + "node cannot have any producer-name configured because block production is not safe when validation_mode is not \"full\""); - EOS_ASSERT(_producers.empty() || chain_plug->accept_transactions(), plugin_config_exception, - "node cannot have any producer-name configured because no block production is possible with no [api|p2p]-accepted-transactions"); + EOS_ASSERT(_producers.empty() || chain_plug->accept_transactions(), plugin_config_exception, + "node cannot have any producer-name configured because no block production is possible with no [api|p2p]-accepted-transactions"); - _accepted_block_connection.emplace(chain.accepted_block.connect([this](const auto& bsp) { on_block(bsp); })); - _accepted_block_header_connection.emplace(chain.accepted_block_header.connect([this](const auto& bsp) { on_block_header(bsp); })); - _irreversible_block_connection.emplace(chain.irreversible_block.connect([this](const auto& bsp) { on_irreversible_block(bsp->block); })); + _accepted_block_connection.emplace(chain.accepted_block.connect([this](const auto& bsp) { on_block(bsp); })); + _accepted_block_header_connection.emplace(chain.accepted_block_header.connect([this](const auto& bsp) { on_block_header(bsp); })); + _irreversible_block_connection.emplace( + chain.irreversible_block.connect([this](const auto& bsp) { on_irreversible_block(bsp->block); })); - _block_start_connection.emplace(chain.block_start.connect([this, &chain](uint32_t bs) { - try { - _snapshot_scheduler.on_start_block(bs, chain); - } catch (const snapshot_execution_exception& e) { - fc_elog(_log, "Exception during snapshot execution: ${e}", ("e", e.to_detail_string())); - app().quit(); - } - })); + _block_start_connection.emplace(chain.block_start.connect([this, &chain](uint32_t bs) { + try { + _snapshot_scheduler.on_start_block(bs, chain); + } catch (const snapshot_execution_exception& e) { + fc_elog(_log, "Exception during snapshot execution: ${e}", ("e", e.to_detail_string())); + app().quit(); + } + })); - const auto lib_num = chain.last_irreversible_block_num(); - const auto lib = chain.fetch_block_by_number(lib_num); - if (lib) { - on_irreversible_block(lib); - } else { - _irreversible_block_time = fc::time_point::maximum(); - } + const auto lib_num = chain.last_irreversible_block_num(); + const auto lib = chain.fetch_block_by_number(lib_num); + if (lib) { + on_irreversible_block(lib); + } else { + _irreversible_block_time = fc::time_point::maximum(); + } - if (!_producers.empty()) { - ilog("Launching block production for ${n} producers at ${time}.", ("n", _producers.size())("time", fc::time_point::now())); + if (!_producers.empty()) { + ilog("Launching block production for ${n} producers at ${time}.", ("n", _producers.size())("time", fc::time_point::now())); - if (_production_enabled) { - if (chain.head_block_num() == 0) { - new_chain_banner(chain); + if (_production_enabled) { + if (chain.head_block_num() == 0) { + new_chain_banner(chain); + } } } - } - if (_ro_thread_pool_size > 0) { - _ro_thread_pool.start( - _ro_thread_pool_size, - [](const fc::exception& e) { - fc_elog(_log, "Exception in read-only thread pool, exiting: ${e}", ("e", e.to_detail_string())); - app().quit(); - }, - [&]() { - chain.init_thread_local_data(); - }); - - start_write_window(); - } + if (_ro_thread_pool_size > 0) { + _ro_thread_pool.start( + _ro_thread_pool_size, + [](const fc::exception& e) { + fc_elog(_log, "Exception in read-only thread pool, exiting: ${e}", ("e", e.to_detail_string())); + app().quit(); + }, + [&]() { + chain.init_thread_local_data(); + }); + + start_write_window(); + } - schedule_production_loop(); + schedule_production_loop(); - ilog("producer plugin: plugin_startup() end"); + ilog("producer plugin: plugin_startup() end"); } FC_CAPTURE_AND_RETHROW() } From 64ef9ea9dd6db5430b328f8ddf366e7b823f5d19 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Tue, 13 Jun 2023 10:54:47 -0400 Subject: [PATCH 13/15] Remove unneeded semicolumn --- plugins/producer_plugin/producer_plugin.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/producer_plugin/producer_plugin.cpp b/plugins/producer_plugin/producer_plugin.cpp index dfffe9a37a..5a778bae31 100644 --- a/plugins/producer_plugin/producer_plugin.cpp +++ b/plugins/producer_plugin/producer_plugin.cpp @@ -1024,7 +1024,7 @@ void producer_plugin_impl::plugin_initialize(const boost::program_options::varia chain_plug = app().find_plugin(); EOS_ASSERT(chain_plug, plugin_config_exception, "chain_plugin not found" ); _options = &options; - LOAD_VALUE_SET(options, "producer-name", _producers); + LOAD_VALUE_SET(options, "producer-name", _producers) chain::controller& chain = chain_plug->chain(); From 153ff89c19f0b94da04da402a11c959168f0bde9 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Thu, 15 Jun 2023 11:39:32 -0400 Subject: [PATCH 14/15] Don't throw when `app().quit()` called during plugin_startup. --- libraries/appbase | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/appbase b/libraries/appbase index fa081b40ae..73e7a76498 160000 --- a/libraries/appbase +++ b/libraries/appbase @@ -1 +1 @@ -Subproject commit fa081b40ae2d9822de5073e88fbfdebc55dab986 +Subproject commit 73e7a764985d976b16fa040c249d746b6fb7dda0 From b45ef199b0cecc8cda086d02cddaff516bc41455 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Thu, 15 Jun 2023 12:00:09 -0400 Subject: [PATCH 15/15] Update appbase to branch tip --- libraries/appbase | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/appbase b/libraries/appbase index 73e7a76498..39043bafe2 160000 --- a/libraries/appbase +++ b/libraries/appbase @@ -1 +1 @@ -Subproject commit 73e7a764985d976b16fa040c249d746b6fb7dda0 +Subproject commit 39043bafe2e9edd73539c68ec076f98831842ab5