jrl-umi3218 / mc_rtc

mc_rtc is an interface for simulated and real robotic systems suitable for real-time control
BSD 2-Clause "Simplified" License
122 stars 36 forks source link

mc_control::fsm::Executor should be non copyable #321

Closed JulienRo7 closed 1 year ago

JulienRo7 commented 1 year ago

Hi @gergondet

I encountered an issue where I was able to copy a locally initialized Executor in a map, which was not working later? After investigating with @arntanguy we found that Executor should not be copyable.

gergondet commented 1 year ago

Hi @JulienRo7 @arntanguy

Can you clarify what was the exact issue? I don't think it should be an issue to copy an executor.

JulienRo7 commented 1 year ago

Hi @gergondet

I am using a map of executors inside one of my classes and when I am initializing the executor I did something like this:

std::unordered_map<Hand, mc_control::fsm::Executor, internal::EnumClassHash> handExecutors_;
for(const auto & hand : BOTH_HANDS)
    {
      mc_control::fsm::Executor handExecutor;
      std::string executorName = "HandExecutor_"+std::to_string(hand);
      mc_rtc::Configuration config;
      // read the right configuration file here...

      handExecutorPtr.init(ctl(), config, executorName, {"HandExecutor", std::to_string(hand)});
      handExecutors_.insert({hand, handExecutor});
    }

Note that for simplification of the issue I removed the part on loading the configuration. But this code was crashing without any error. The solution was to use smart pointers like this:

std::unordered_map<Hand, std::shared_ptr<mc_control::fsm::Executor>, internal::EnumClassHash> handExecutors_;
for(const auto & hand : BOTH_HANDS)
    {
      std::shared_ptr<mc_control::fsm::Executor> handExecutorPtr = std::make_shared<mc_control::fsm::Executor>();
      std::string executorName = "HandExecutor_"+std::to_string(hand);
      mc_rtc::Configuration config;
      // read the right configuration file here...

      handExecutorPtr->init(ctl(), config, executorName, {"HandExecutor", std::to_string(hand)});
      handExecutors_.insert({hand, handExecutorPtr});
    }
gergondet commented 1 year ago

Thanks for the clarification. After looking deeper into it, the executor cannot be moved after it has been initialized, to prevent this kind of issue I will make Executor both non-copiable and non-movable