Replace pointers with values

[ChrisThrasher]

I found two instances of shared pointers which can be represented as values. Because values have fewer features and few indirections required to use them, they're the better choice in general. I can't think of any reason why these members need to be pointers but if they do, we can still use unique_ptr since shared ownership of these members is not necessary.

[tylerjw]

I'm sorry but because of what I know about ClassLoader and how it is unsafe to destroy it; this should stay a shared_ptr.

A better change would be to copy the shared_ptr into a global to make sure it has the same lifetime as the process.

[ChrisThrasher]

and how it is unsafe to destroy it

I'm unsure what you mean since the shared_ptr will destroy it too.

[tylerjw]

If you copy that shared ptr into a global you can extend the lifetime. Making it allocated on the stack gives the user the impression it is safe to delete.

[ChrisThrasher]

If you copy that shared ptr into a global you can extend the lifetime.

It sounds like you're describing a bug in the status quo of the library. Does this PR exacerbate that problem?

Making it allocated on the stack gives the user the impression it is safe to delete.

This is private data. It's nothing users should be able to observe in the first place.

[destogl]

This looks good to me. Nevertheless, we are also using loaders in “Resource Manager” so the same changes should also be done there.

Other than that, since we are now resetting the loader object, we have to be sure that objects created by this loader are really independent of it. Do we have a test for it already?

[tylerjw]

since we are now resetting the loader object, we have to be sure that objects created by this loader are really independent of it

because of how class_loader works, this is not true. seem my PR here:

ros/class_loader#199

[ChrisThrasher]

This looks good to me. Nevertheless, we are also using loaders in “Resource Manager” so the same changes should also be done there.

Are you talking about the ResourceManager::resource_storage_ data member? That can be converted from a unique_ptr to a value so long as we're willing to move the definition of ResourceStorage from resource_manager.cpp to resource_manager.hpp.

EDIT: I have this implemented locally and the diff is a few hundred lines long just to convert a single unique_ptr<T> to a T. I'm not sure all this is worth removing a single heap allocation.

[destogl]

This should be fine. I think we can go with it. It is a bit cleaner.

[ChrisThrasher]

I see the ABI check is failing which makes sense because changing the layout of a class is a potential ABI break. Does that invalidate this PR or are there options to deal with that?

[bmagyar]

@ChrisThrasher I'd not worry too much about the ABI breaking jobs. Look at the result from this job.

Relevant print is here:

    [INFO] [1669573554.627052878] [test_controller_manager]: Controller manager: reloaded controller libraries for 'controller_interface'
    [INFO] [1669573554.627289276] [test_controller_manager]: Loading controller 'test_controller_name'
    [INFO] [1669573554.632657624] [test_controller_manager]: Controller manager: Stopping all active controllers
    [INFO] [1669573554.632699523] [test_controller_manager]: Empty activate and deactivate list, not requesting switch
    [WARN] [1669573554.632729323] [rcl_lifecycle]: No transition matching 2 found for current state unconfigured
Error: ROR] [1669573554.632740723] []: Unable to start transition 2 from current state unconfigured: Transition is not registered., at ./src/rcl_lifecycle.c:355
    Warning: class_loader::ClassLoader: Cannot unload library /home/runner/work/ros2_control/ros2_control/.work/target_ws/install/controller_manager/lib/libtest_controller.so even though last shared pointer went out of scope. This is because createUnmanagedInstance was used within the scope of this process, perhaps by a different ClassLoader. Library will NOT be closed.
             at line 278 in /opt/ros/rolling/include/class_loader/class_loader/class_loader.hpp

[mergify]

This pull request is in conflict. Could you fix it @ChrisThrasher?

[destogl]

This causes segmentation fault on runtime-tests - closing.