fix(command_manager): avoid using std::remove to erase elements from …

…std::vector (#734)

include/dsn/perf_counter/perf_counters.h

@@ -26,6 +26,7 @@
 
 #pragma once
 
+#include <dsn/c/api_common.h>
 #include <dsn/utility/singleton.h>
 #include <dsn/utility/synchronize.h>
 #include <dsn/perf_counter/perf_counter.h>
@@ -161,6 +162,11 @@ class perf_counters : public utils::singleton<perf_counters>
 
     // 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

include/dsn/tool-api/command_manager.h

@@ -64,7 +64,6 @@ class command_manager : public ::dsn::utils::singleton<command_manager>
 
     ::dsn::utils::rw_lock_nr _lock;
     std::map<std::string, command_instance *> _handlers;
-    std::vector<command_instance *> _commands;
 };
 
 } // namespace dsn

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()

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<remote_copy_request> &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",

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

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<std::string> &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,

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;

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)
 {

src/runtime/service_engine.h

@@ -128,6 +128,9 @@ class service_engine : public utils::singleton<service_engine>
     service_spec _spec;
     env_provider *_env;
 
+    dsn_handle_t _get_runtime_info_cmd;
+    dsn_handle_t _get_queue_info_cmd;
+
     // <port, servicenode>
     typedef std::map<int, service_node *>
         node_engines_by_port; // multiple ports may share the same node

src/utils/command_manager.cpp

@@ -39,20 +39,22 @@ dsn_handle_t command_manager::register_command(const std::vector<std::string> &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