Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

fix(command_manager): avoid using std::remove to erase elements from std::vector #734

Merged
merged 4 commits into from
Feb 3, 2021

Conversation

zhangyifan27
Copy link
Contributor

@zhangyifan27 zhangyifan27 commented Jan 21, 2021

On account of std::remove could not actually erase elements from containers,

The removal is done by replacing the elements that compare equal to val by the next element that does not, ... [1]

we may delete command_instance * repeatly when destroy command_manager.

void command_manager::deregister_command(dsn_handle_t handle)
{
    auto c = reinterpret_cast<command_instance *>(handle);
    dassert(c != nullptr, "cannot deregister a null handle");
    utils::auto_write_lock l(_lock);
    for (const std::string &cmd : c->commands) {
        ddebug("unregister command: %s", cmd.c_str());
        _handlers.erase(cmd);
    }
    std::remove(_commands.begin(), _commands.end(), c);	
    delete c;	
}

command_manager::~command_manager()
{	
    for (command_instance *c : _commands) {	
        delete c;	
    }	
}

The correct way to remove command_instance from the vector should be:

    auto iter = std::remove(_commands.begin(), _commands.end(), c);
    _comands.erase(iter, _commands.end());
    delete c;	

This patch remove std::vector<command_instance *> _commands from class command_manager because we already have std::map<std::string, command_instance *> _handlers, and also improve the use of register_command and deregister_command.

[1] https://www.cplusplus.com/reference/algorithm/remove/

hycdong
hycdong previously approved these changes Jan 22, 2021
@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend wrapping command_instance* into a RAII class, so that we do not need to UNREGISTER_VALID_HANDLER manually. If we don't refactor in this PR, I will take this later.

src/utils/command_manager.cpp Show resolved Hide resolved
@zhangyifan27 zhangyifan27 merged commit 4c9b6ef into XiaoMi:master Feb 3, 2021
zhangyifan27 added a commit to zhangyifan27/rdsn that referenced this pull request Feb 3, 2021
zhangyifan27 added a commit to zhangyifan27/rdsn that referenced this pull request Feb 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants