Skip to content

use unique ptr and remove unuseful container#2013

Merged
fujitatomoya merged 1 commit intoros2:rollingfrom
iuhilnehc-ynos:topic-use-unique-ptr
Sep 19, 2022
Merged

use unique ptr and remove unuseful container#2013
fujitatomoya merged 1 commit intoros2:rollingfrom
iuhilnehc-ynos:topic-use-unique-ptr

Conversation

@iuhilnehc-ynos
Copy link
Copy Markdown
Collaborator

@iuhilnehc-ynos iuhilnehc-ynos commented Sep 16, 2022

related to ros2/rcl#1009 (comment)

Signed-off-by: Chen Lihui lihui.chen@sony.com

use unique ptr and remove unuseful container
3233f03 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
@alsora
Copy link
Copy Markdown
Collaborator

alsora commented Sep 16, 2022

Would you mind providing more details about the lifespan of the loaded libraries?
Are the libraries unloaded when the unique pointer goes out of scope?

@iuhilnehc-ynos
Copy link
Copy Markdown
Collaborator Author

iuhilnehc-ynos commented Sep 16, 2022

@alsora

Thanks for asking questions.
I am not sure if the following backtrace information can answer that.

(gdb) bt
#0  class_loader::impl::loadLibrary (library_path="libtopics_library.so", loader=0x5555555fca20)
    at /home/chenlh/Projects/ROS2/ros2-master/src/ros/class_loader/src/class_loader_core.cpp:432
#1  0x00007ffff71ab25f in class_loader::ClassLoader::loadLibrary (this=0x5555555fca20)
    at /home/chenlh/Projects/ROS2/ros2-master/src/ros/class_loader/src/class_loader.cpp:106
#2  0x00007ffff71ab091 in class_loader::ClassLoader::ClassLoader (this=0x5555555fca20, library_path="libtopics_library.so", ondemand_load_unload=false)
    at /home/chenlh/Projects/ROS2/ros2-master/src/ros/class_loader/src/class_loader.cpp:65
#3  0x00005555555683ca in std::make_unique<class_loader::ClassLoader, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&>
    () at /usr/include/c++/9/bits/unique_ptr.h:857
#4  0x0000555555565f65 in main (argc=1, argv=0x7fffffffc5d8)
    at /home/chenlh/Projects/ROS2/ros2-master/build/demo_nodes_cpp/rclcpp_components/node_main_talker.cpp:39

(gdb) bt
#0  class_loader::impl::unloadLibrary (library_path="libtopics_library.so", loader=0x5555555fca20)
    at /home/chenlh/Projects/ROS2/ros2-master/src/ros/class_loader/src/class_loader_core.cpp:539
#1  0x00007ffff71ab3b4 in class_loader::ClassLoader::unloadLibraryInternal (this=0x5555555fca20, lock_plugin_ref_count=true)
    at /home/chenlh/Projects/ROS2/ros2-master/src/ros/class_loader/src/class_loader.cpp:136
#2  0x00007ffff71ab2ef in class_loader::ClassLoader::unloadLibrary (this=0x5555555fca20)
    at /home/chenlh/Projects/ROS2/ros2-master/src/ros/class_loader/src/class_loader.cpp:115
#3  0x00007ffff71ab115 in class_loader::ClassLoader::~ClassLoader (this=0x5555555fca20, __in_chrg=<optimized out>)
    at /home/chenlh/Projects/ROS2/ros2-master/src/ros/class_loader/src/class_loader.cpp:75
#4  0x00007ffff71ab144 in class_loader::ClassLoader::~ClassLoader (this=0x5555555fca20, __in_chrg=<optimized out>)
    at /home/chenlh/Projects/ROS2/ros2-master/src/ros/class_loader/src/class_loader.cpp:76
#5  0x00005555555691c0 in std::default_delete<class_loader::ClassLoader>::operator() (this=0x7fffffffb638, __ptr=0x5555555fca20)
    at /usr/include/c++/9/bits/unique_ptr.h:81
#6  0x000055555556845e in std::unique_ptr<class_loader::ClassLoader, std::default_delete<class_loader::ClassLoader> >::~unique_ptr (
    this=0x7fffffffb638, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/unique_ptr.h:292
#7  0x00005555555664b9 in main (argc=1, argv=0x7fffffffc5d8)
    at /home/chenlh/Projects/ROS2/ros2-master/build/demo_nodes_cpp/rclcpp_components/node_main_talker.cpp:39

The key to fix ros2/rcl#1009 is to call class_loader::ClassLoader::unloadLibrary in the ClassLoader destructor to use purgeGraveyardOfMetaobjects to destroy MetaObject which created by the macro before.

fujitatomoya
fujitatomoya approved these changes Sep 16, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Sep 16, 2022

this does not depend on ros/class_loader#200, but related.

@alsora
Copy link
Copy Markdown
Collaborator

alsora commented Sep 17, 2022

@iuhilnehc-ynos thank you.
From the back trace it looks like the libraries are unloaded when the unique pointers go out of scope.
This happens right after a node is created.

I'm a little bit concerned about this.
Usually when dealing with dynamically loaded libraries it is best practice to keep the library in scope until objects are using it.
This is important especially for architectures using linkers with lazy binding (I.e. Symbols are not resolved when you load a library but rather when the first time you use them).

I think we should keep the libraries loaded until we have destroyed the nodes.

@iuhilnehc-ynos
Copy link
Copy Markdown
Collaborator Author

iuhilnehc-ynos commented Sep 17, 2022

I think we should keep the libraries loaded until we have destroyed the nodes.

rclcpp/rclcpp_components/src/node_main.cpp.in

Line 67 in 3233f03

node_wrappers.clear();
can destroy the nodes before unloading the libraries.

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Sep 17, 2022

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@iuhilnehc-ynos
Copy link
Copy Markdown
Collaborator Author

iuhilnehc-ynos commented Sep 19, 2022

@ros-pull-request-builder retest this please

  • Windows Build Status

@iuhilnehc-ynos
Copy link
Copy Markdown
Collaborator Author

iuhilnehc-ynos commented Sep 19, 2022

Error happened in the CI as following
14:08:19 CVTRES : fatal error CVT1107: 'C:\Program Files\rti_connext_dds-6.0.1\lib\x64Win64VS2017\nddscored.lib' is corrupt

Give it another shot,

  • Windows Build Status

@fujitatomoya fujitatomoya merged commit 6a8c61c into ros2:rolling Sep 19, 2022
@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Sep 19, 2022

@Mergifyio backport humble galactic foxy

mergify bot pushed a commit that referenced this pull request Sep 19, 2022
@mergify
use unique ptr and remove unuseful container (#2013)
022fb64 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>

Signed-off-by: Chen Lihui <lihui.chen@sony.com>
(cherry picked from commit 6a8c61c)
This was referenced Sep 19, 2022
Open
Closed
mergify bot pushed a commit that referenced this pull request Sep 19, 2022
@mergify
use unique ptr and remove unuseful container (#2013)
53cbad1 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>

Signed-off-by: Chen Lihui <lihui.chen@sony.com>
(cherry picked from commit 6a8c61c)
@mergify mergify bot mentioned this pull request Sep 19, 2022
Closed
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Sep 19, 2022

backport humble galactic foxy

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Sep 19, 2022
@mergify
use unique ptr and remove unuseful container (#2013)
a334206 
Signed-off-by: Chen Lihui <lihui.chen@sony.com>

Signed-off-by: Chen Lihui <lihui.chen@sony.com>
(cherry picked from commit 6a8c61c)
@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Sep 19, 2022

@iuhilnehc-ynos can you take care of the backpors for foxy/galactic/humble?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alsora alsora alsora approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iuhilnehc-ynos @alsora @fujitatomoya