From 4c9b6efd07265cd4fd4e38b50560326735e3c14f Mon Sep 17 00:00:00 2001 From: Zhang Yifan Date: Wed, 3 Feb 2021 15:17:01 +0800 Subject: [PATCH] fix(command_manager): avoid using std::remove to erase elements from std::vector (#734) --- include/dsn/perf_counter/perf_counters.h | 6 ++++++ include/dsn/tool-api/command_manager.h | 1 - src/meta/server_state.cpp | 17 +++-------------- src/nfs/nfs_client_impl.cpp | 8 ++++++-- src/nfs/nfs_client_impl.h | 2 ++ src/perf_counter/perf_counters.cpp | 16 +++++++++++----- src/replica/replica_stub.cpp | 23 +++++++++++------------ src/runtime/service_engine.cpp | 18 ++++++++++++------ src/runtime/service_engine.h | 3 +++ src/utils/command_manager.cpp | 16 ++++++---------- 10 files changed, 60 insertions(+), 50 deletions(-) diff --git a/include/dsn/perf_counter/perf_counters.h b/include/dsn/perf_counter/perf_counters.h index 2352a5621b..3aa946e505 100644 --- a/include/dsn/perf_counter/perf_counters.h +++ b/include/dsn/perf_counter/perf_counters.h @@ -26,6 +26,7 @@ #pragma once +#include #include #include #include @@ -161,6 +162,11 @@ class perf_counters : public utils::singleton // timestamp in seconds when take snapshot of current counters int64_t _timestamp; + + dsn_handle_t _perf_counters_cmd; + dsn_handle_t _perf_counters_by_substr_cmd; + dsn_handle_t _perf_counters_by_prefix_cmd; + dsn_handle_t _perf_counters_by_postfix_cmd; }; } // namespace dsn diff --git a/include/dsn/tool-api/command_manager.h b/include/dsn/tool-api/command_manager.h index 7694905456..2d118ac78a 100644 --- a/include/dsn/tool-api/command_manager.h +++ b/include/dsn/tool-api/command_manager.h @@ -64,7 +64,6 @@ class command_manager : public ::dsn::utils::singleton ::dsn::utils::rw_lock_nr _lock; std::map _handlers; - std::vector _commands; }; } // namespace dsn diff --git a/src/meta/server_state.cpp b/src/meta/server_state.cpp index f5a4b3bf47..c29238a162 100644 --- a/src/meta/server_state.cpp +++ b/src/meta/server_state.cpp @@ -72,20 +72,9 @@ server_state::server_state() server_state::~server_state() { _tracker.cancel_outstanding_tasks(); - if (_cli_dump_handle != nullptr) { - dsn::command_manager::instance().deregister_command(_cli_dump_handle); - _cli_dump_handle = nullptr; - } - if (_ctrl_add_secondary_enable_flow_control != nullptr) { - dsn::command_manager::instance().deregister_command( - _ctrl_add_secondary_enable_flow_control); - _ctrl_add_secondary_enable_flow_control = nullptr; - } - if (_ctrl_add_secondary_max_count_for_one_node != nullptr) { - dsn::command_manager::instance().deregister_command( - _ctrl_add_secondary_max_count_for_one_node); - _ctrl_add_secondary_max_count_for_one_node = nullptr; - } + UNREGISTER_VALID_HANDLER(_cli_dump_handle); + UNREGISTER_VALID_HANDLER(_ctrl_add_secondary_enable_flow_control); + UNREGISTER_VALID_HANDLER(_ctrl_add_secondary_max_count_for_one_node); } void server_state::register_cli_commands() diff --git a/src/nfs/nfs_client_impl.cpp b/src/nfs/nfs_client_impl.cpp index 6520a26b36..70818ad993 100644 --- a/src/nfs/nfs_client_impl.cpp +++ b/src/nfs/nfs_client_impl.cpp @@ -116,7 +116,11 @@ nfs_client_impl::nfs_client_impl() register_cli_commands(); } -nfs_client_impl::~nfs_client_impl() { _tracker.cancel_outstanding_tasks(); } +nfs_client_impl::~nfs_client_impl() +{ + _tracker.cancel_outstanding_tasks(); + UNREGISTER_VALID_HANDLER(_nfs_max_copy_rate_megabytes_cmd); +} void nfs_client_impl::begin_remote_copy(std::shared_ptr &rci, aio_task *nfs_task) @@ -562,7 +566,7 @@ void nfs_client_impl::register_cli_commands() static std::once_flag flag; std::call_once(flag, [&]() { - dsn::command_manager::instance().register_command( + _nfs_max_copy_rate_megabytes_cmd = dsn::command_manager::instance().register_command( {"nfs.max_copy_rate_megabytes"}, "nfs.max_copy_rate_megabytes [num | DEFAULT]", "control the max rate(MB/s) to copy file from remote node", diff --git a/src/nfs/nfs_client_impl.h b/src/nfs/nfs_client_impl.h index f7e641550b..76ea371d00 100644 --- a/src/nfs/nfs_client_impl.h +++ b/src/nfs/nfs_client_impl.h @@ -276,6 +276,8 @@ class nfs_client_impl : public ::dsn::service::nfs_client perf_counter_wrapper _recent_write_data_size; perf_counter_wrapper _recent_write_fail_count; + dsn_handle_t _nfs_max_copy_rate_megabytes_cmd; + dsn::task_tracker _tracker; }; } // namespace service diff --git a/src/perf_counter/perf_counters.cpp b/src/perf_counter/perf_counters.cpp index 7cc49af077..ef89a84267 100644 --- a/src/perf_counter/perf_counters.cpp +++ b/src/perf_counter/perf_counters.cpp @@ -46,14 +46,14 @@ namespace dsn { perf_counters::perf_counters() { - command_manager::instance().register_command( + _perf_counters_cmd = command_manager::instance().register_command( {"perf-counters"}, "perf-counters - query perf counters, filtered by OR of POSIX basic regular expressions", "perf-counters [regexp]...", [](const std::vector &args) { return perf_counters::instance().list_snapshot_by_regexp(args); }); - command_manager::instance().register_command( + _perf_counters_by_substr_cmd = command_manager::instance().register_command( {"perf-counters-by-substr"}, "perf-counters-by-substr - query perf counters, filtered by OR of substrs", "perf-counters-by-substr [substr]...", @@ -63,7 +63,7 @@ perf_counters::perf_counters() return cs.name.find(arg) != std::string::npos; }); }); - command_manager::instance().register_command( + _perf_counters_by_prefix_cmd = command_manager::instance().register_command( {"perf-counters-by-prefix"}, "perf-counters-by-prefix - query perf counters, filtered by OR of prefix strings", "perf-counters-by-prefix [prefix]...", @@ -74,7 +74,7 @@ perf_counters::perf_counters() ::memcmp(cs.name.c_str(), arg.c_str(), arg.size()) == 0; }); }); - command_manager::instance().register_command( + _perf_counters_by_postfix_cmd = command_manager::instance().register_command( {"perf-counters-by-postfix"}, "perf-counters-by-postfix - query perf counters, filtered by OR of postfix strings", "perf-counters-by-postfix [postfix]...", @@ -89,7 +89,13 @@ perf_counters::perf_counters() }); } -perf_counters::~perf_counters() = default; +perf_counters::~perf_counters() +{ + UNREGISTER_VALID_HANDLER(_perf_counters_cmd); + UNREGISTER_VALID_HANDLER(_perf_counters_by_substr_cmd); + UNREGISTER_VALID_HANDLER(_perf_counters_by_prefix_cmd); + UNREGISTER_VALID_HANDLER(_perf_counters_by_postfix_cmd); +} perf_counter_ptr perf_counters::get_app_counter(const char *section, const char *name, diff --git a/src/replica/replica_stub.cpp b/src/replica/replica_stub.cpp index f9585da6ec..b00b049935 100644 --- a/src/replica/replica_stub.cpp +++ b/src/replica/replica_stub.cpp @@ -2416,20 +2416,19 @@ void replica_stub::close() return; } - dsn::command_manager::instance().deregister_command(_kill_partition_command); - dsn::command_manager::instance().deregister_command(_deny_client_command); - dsn::command_manager::instance().deregister_command(_verbose_client_log_command); - dsn::command_manager::instance().deregister_command(_verbose_commit_log_command); - dsn::command_manager::instance().deregister_command(_trigger_chkpt_command); - dsn::command_manager::instance().deregister_command(_query_compact_command); - dsn::command_manager::instance().deregister_command(_query_app_envs_command); - dsn::command_manager::instance().deregister_command(_useless_dir_reserve_seconds_command); + UNREGISTER_VALID_HANDLER(_kill_partition_command); + UNREGISTER_VALID_HANDLER(_deny_client_command); + UNREGISTER_VALID_HANDLER(_verbose_client_log_command); + UNREGISTER_VALID_HANDLER(_verbose_commit_log_command); + UNREGISTER_VALID_HANDLER(_trigger_chkpt_command); + UNREGISTER_VALID_HANDLER(_query_compact_command); + UNREGISTER_VALID_HANDLER(_query_app_envs_command); + UNREGISTER_VALID_HANDLER(_useless_dir_reserve_seconds_command); #ifdef DSN_ENABLE_GPERF - dsn::command_manager::instance().deregister_command(_release_tcmalloc_memory_command); - dsn::command_manager::instance().deregister_command(_max_reserved_memory_percentage_command); + UNREGISTER_VALID_HANDLER(_release_tcmalloc_memory_command); + UNREGISTER_VALID_HANDLER(_max_reserved_memory_percentage_command); #endif - dsn::command_manager::instance().deregister_command( - _max_concurrent_bulk_load_downloading_count_command); + UNREGISTER_VALID_HANDLER(_max_concurrent_bulk_load_downloading_count_command); _kill_partition_command = nullptr; _deny_client_command = nullptr; diff --git a/src/runtime/service_engine.cpp b/src/runtime/service_engine.cpp index 5703fd0c10..9b264c6629 100644 --- a/src/runtime/service_engine.cpp +++ b/src/runtime/service_engine.cpp @@ -173,18 +173,24 @@ service_engine::service_engine() { _env = nullptr; - ::dsn::command_manager::instance().register_command({"engine"}, - "engine - get engine internal information", - "engine [app-id]", - &service_engine::get_runtime_info); - ::dsn::command_manager::instance().register_command( + _get_runtime_info_cmd = dsn::command_manager::instance().register_command( + {"engine"}, + "engine - get engine internal information", + "engine [app-id]", + &service_engine::get_runtime_info); + + _get_queue_info_cmd = dsn::command_manager::instance().register_command( {"system.queue"}, "system.queue - get queue internal information", "system.queue", &service_engine::get_queue_info); } -service_engine::~service_engine() = default; +service_engine::~service_engine() +{ + UNREGISTER_VALID_HANDLER(_get_runtime_info_cmd); + UNREGISTER_VALID_HANDLER(_get_queue_info_cmd); +} void service_engine::init_before_toollets(const service_spec &spec) { diff --git a/src/runtime/service_engine.h b/src/runtime/service_engine.h index dc7c94ab9d..38c5a389ef 100644 --- a/src/runtime/service_engine.h +++ b/src/runtime/service_engine.h @@ -128,6 +128,9 @@ class service_engine : public utils::singleton service_spec _spec; env_provider *_env; + dsn_handle_t _get_runtime_info_cmd; + dsn_handle_t _get_queue_info_cmd; + // typedef std::map node_engines_by_port; // multiple ports may share the same node diff --git a/src/utils/command_manager.cpp b/src/utils/command_manager.cpp index 2975cb5278..68c38c4ad8 100644 --- a/src/utils/command_manager.cpp +++ b/src/utils/command_manager.cpp @@ -39,20 +39,22 @@ dsn_handle_t command_manager::register_command(const std::vector &c command_handler handler) { utils::auto_write_lock l(_lock); + bool is_valid_cmd = false; for (const std::string &cmd : commands) { if (!cmd.empty()) { + is_valid_cmd = true; auto it = _handlers.find(cmd); dassert(it == _handlers.end(), "command '%s' already regisered", cmd.c_str()); } } + dassert(is_valid_cmd, "should not register empty command"); command_instance *c = new command_instance(); c->commands = commands; c->help_long = help_long; c->help_short = help_one_line; c->handler = handler; - _commands.push_back(c); for (const std::string &cmd : commands) { if (!cmd.empty()) { @@ -71,7 +73,6 @@ void command_manager::deregister_command(dsn_handle_t handle) ddebug("unregister command: %s", cmd.c_str()); _handlers.erase(cmd); } - std::remove(_commands.begin(), _commands.end(), c); delete c; } @@ -106,8 +107,8 @@ command_manager::command_manager() if (args.size() == 0) { utils::auto_read_lock l(_lock); - for (auto c : this->_commands) { - ss << c->help_short << std::endl; + for (const auto &c : this->_handlers) { + ss << c.second->help_short << std::endl; } } else { utils::auto_read_lock l(_lock); @@ -172,11 +173,6 @@ command_manager::command_manager() }); } -command_manager::~command_manager() -{ - for (command_instance *c : _commands) { - delete c; - } -} +command_manager::~command_manager() { _handlers.clear(); } } // namespace dsn