Resource Manager does not throw when component plugin failed to load

[baconbrot]

Describe the bug
The return value of bool ResourceManager::load_and_initialize_components( const std::string & urdf, const unsigned int update_rate) in the resource manager constructor

ResourceManager::ResourceManager(
  const std::string & urdf, rclcpp::node_interfaces::NodeClockInterface::SharedPtr clock_interface,
  rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr logger_interface, bool activate_all,
  const unsigned int update_rate)
: resource_storage_(std::make_unique<ResourceStorage>(clock_interface, logger_interface))
{
  load_and_initialize_components(urdf, update_rate);
...

is ignored.

In consequence, tests like ASSERT_NO_THROW(TestableResourceManager rm(node_, urdf)); in generic system load test are passed, even when e.g. a system plugin could not be loaded in bool load_and_initialize_system(const HardwareInfo & hardware_info).
Only a ROS 2 log message is generated.

To Reproduce
Steps to reproduce the behavior:

1. Clone this repo and install rosdeps
2. Replace <plugin>mock_components/GenericSystem</plugin> in the definition of attribute hardware_system_2dof_ with a non existing component, e.g. <plugin>mock_components/Nein</plugin>
3. Compile the repo with tests, e.g. colcon build --packages-up-to hardware_interface --mixin build-testing-on
4. Run the test, e.g. colcon test --packages-select hardware_interface --event-handlers console_cohesion+
5. The corresponding test will pass with OK and a ROS 2 ERROR log is generated

Expected behavior
The constructor should throw and the test should fail if the component plugin could not be loaded.

Test result

7: [==========] Running 22 tests from 1 test suite.
7: [----------] Global test environment set-up.
7: [----------] 22 tests from TestGenericSystem
7: [ RUN      ] TestGenericSystem.load_generic_system_2dof
7: [INFO] [1743348257.395870523] [TestGenericSystem.resource_manager]: Loading hardware 'MockHardwareSystem'
7: [ERROR] [1743348257.396073635] [TestGenericSystem.resource_manager]: Caught exception of type : N9pluginlib20LibraryLoadExceptionE while loading hardware: According to the loaded plugin descriptions the class mock_components/Nein with base class type hardware_interface::SystemInterface does not exist. Declared types are  mock_components/GenericSystem test_hardware_components/TestSystemCommandModes test_hardware_components/TestTwoJointSystem
7: [       OK ] TestGenericSystem.load_generic_system_2dof (22 ms)
...

Environment:

• Docker test environment docker run --rm -it --network host ros:rolling bash
• Version Rolling / Jazzy

[saikishor]

Hello @baconbrot !

I'm rethinking a bit about #2153. Can you explain why you want it to throw an exception?. Ideally, when the load is failed, you don't have the CM services exposed right?. If so, why?

[baconbrot]

Hello @baconbrot!

I'm rethinking a bit about #2153. Can you explain why you want it to throw an exception? Ideally, when the load fails, you don't have the CM services exposed, right? If so, why?

Hey @saikishor!

Indeed, I don't have a use case for the resource manager to throw an exception when a component fails to load.

I was creating a test for a custom hardware component and noticed—while referring to the supplied test here—that my custom component could not be loaded (i.e., I received an error message from pluginlib from this method). I had assumed, based on ASSERT_NO_THROW(TestableResourceManager rm(node_, urdf)); here, that the resource manager was expected to throw in this case.

However, I overlooked the hardware_interface_testing package, which contains tests that catch such behavior, such as this one.

I am fine with closing this issue and #2152, but there is still a small fix in 8a2d3f95c4b2c48b97c8b3de7287a423c4c2d7f3, which I would resubmit in another PR if that's fine with you :)

[saikishor]

@christophfroehlich do you have any opinion here on whether to throw an exception here or not?

[christophfroehlich]

I prefer not to throw. However, if it is the initial state the CM will throw afterwards anyways ;)

[saikishor]

Yes! That's what I was thinking too. Perfect then the other PR is not needed :)