Galactic: for_each_callback_group backport #1741

adityapande-1995 · 2021-08-03T19:39:42Z

This PR aims takle ros-simulation/gazebo_ros_pkgs#1296 by backporting #1723 to galactic without breaking ABI. To do this, I added a new non virtual function in node_base class, and added static members to it to keep track of a global map of mutexes. This map stores the mutexes for each node_base instance. However, while using the new for_each_callback_group method, the pointer of type NodeBaseInterface has to be typecasted to NodeBase. The global map of mutexes is written as a separate class and access to it is protected by an internal mutex.

TODO

General code cleanup, linting issues
Modify static_executor_entities_collector
Modify the corresponding test cases

clalancette

I left a preliminary round of review. I would also suggest rebasing all of the individual commits into a single commit, adding a useful commit message, and signing it properly (which will make the DCO bot happy).

rclcpp/include/rclcpp/node.hpp

rclcpp/include/rclcpp/node_interfaces/node_base.hpp

rclcpp/src/rclcpp/executor.cpp

rclcpp/src/rclcpp/node_interfaces/node_base.cpp

…se.hpp, updated the for_each_callback_group wrapper method in lifecycle_node and node.cpp, added a thread safe global map of mutexes in node_base. Implemented the suggestions in preliminary review. Signed-off-by: Aditya Pande <aditya050995@gmail.com>

adityapande-1995 · 2021-08-04T23:04:49Z

Changes:

Squashed commits, signed
Removed debug prints, macro
removed typedef lines, CallbackGroupFunction now works the same way as in the ROS rolling commit
Changed variable name to node_base
map_of_mutexes now in rclcpp::node_interfaces namespace
count_of_instances changed to boolean flag, protected by a mutex now
Added docstring for for_each_callback_group in node.cpp and lifecycle_node.cpp
Removed blank spaces

TODO

Modify tests
Make changes to static_entities_collector
general cleanup, linting fixes, ament, uncrustify checks

…ixes Signed-off-by: Aditya Pande <aditya050995@gmail.com>

Signed-off-by: Aditya Pande <aditya050995@gmail.com>

…_cb_group Signed-off-by: Aditya Pande <aditya050995@gmail.com>

…each_cb_group Signed-off-by: Aditya Pande <aditya050995@gmail.com>

ivanpauno

I have a bunch of questions/comments.

rclcpp/include/rclcpp/node.hpp

rclcpp/include/rclcpp/node_interfaces/node_base.hpp

rclcpp/src/rclcpp/node.cpp

Signed-off-by: Aditya Pande <aditya050995@gmail.com>

…ointer cast Signed-off-by: Aditya Pande <aditya050995@gmail.com>

clalancette

Overall, I think this is getting close. I've left a few more things to think about.

rclcpp/include/rclcpp/node_interfaces/node_base.hpp

rclcpp/src/rclcpp/node_interfaces/node_base.cpp

rclcpp/include/rclcpp/node_interfaces/node_base.hpp

…_mutexes Signed-off-by: Aditya Pande <aditya050995@gmail.com>

clalancette

I have a bunch of nits inline. Once those are all fixed, I think this is good to go from my perspective, though I'd like to get @ivanpauno 's final thoughts before we run CI on this.

rclcpp/include/rclcpp/node_interfaces/node_base.hpp

rclcpp/src/rclcpp/node_interfaces/node_base.cpp

Signed-off-by: Aditya Pande <aditya050995@gmail.com>

clalancette

This looks good to me with three things:

Green CI.
Running abi-compliance-checker before and after this PR to make sure we aren't changing ABI.
An approval from @ivanpauno and @cottsay

cottsay

Looks good from an ABI compatibility perspective. I had a couple of other comments inline, but nothing critical.

rclcpp/src/rclcpp/node_interfaces/node_base.cpp

rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp

Signed-off-by: Aditya Pande <aditya050995@gmail.com>

adityapande-1995 · 2021-08-16T22:51:07Z

CI Jobs:

Linux
Linux-aarch64
macOS
Windows

adityapande-1995 · 2021-08-17T17:06:06Z

@cottsay How should we handle the unstable windows CI ? The failures are in rosbag2_transport which don't seem to be related to this PR.

cottsay · 2021-08-17T22:15:02Z

How should we handle the unstable windows CI ? The failures are in rosbag2_transport which don't seem to be related to this PR.

If you're confident that they aren't a regression caused by this PR, then I think it's OK to ignore the test failures. It might be worth triggering a baseline build to be sure, though.

adityapande-1995 · 2021-08-18T16:34:38Z

CI on galactic branch of rclcpp:

Linux
Linux-aarch64
macOS
Windows

ivanpauno

LGTM!

ivanpauno · 2021-08-18T19:22:08Z

@cottsay How should we handle the unstable windows CI ? The failures are in rosbag2_transport which don't seem to be related to this PR.

I can't tell if those failures are related or not, +1 to compare with a baseline build.

adityapande-1995 · 2021-08-18T19:41:35Z

CI on galactic branch of rclcpp:

Linux

Linux-aarch64

macOS

Windows

So this CI was run by changing the branch of rclcpp to galactic, i.e without my changes and it produced the same error in Windows. @cottsay @ivanpauno

ivanpauno · 2021-08-18T20:38:21Z

So this CI was run by changing the branch of rclcpp to galactic, i.e without my changes and it produced the same error in Windows. @cottsay @ivanpauno

LGTM!

cottsay

Thanks for double-checking!

adityapande-1995 · 2021-08-19T20:13:24Z

ABI check report between galactic and aditya/thread_safe_cb branches of rclcpp passed, merging !

Added thread safe for_each_callback_group method Signed-off-by: Aditya Pande <aditya050995@gmail.com>

…ick-1741 Galactic: for_each_callback_group backport (ros2#1741)

clalancette requested changes Aug 3, 2021

View reviewed changes

adityapande-1995 force-pushed the aditya/thread-safe-cb branch from f779270 to 6971111 Compare August 4, 2021 22:54

adityapande-1995 force-pushed the aditya/thread-safe-cb branch from 6971111 to 2fbc5ce Compare August 4, 2021 22:56

adityapande-1995 added 4 commits August 6, 2021 16:20

Typecasting added to static_executor_entities_collector, uncrustify f…

64d258f

…ixes Signed-off-by: Aditya Pande <aditya050995@gmail.com>

test_node.cpp changed callback_groups test

0acbc8b

Signed-off-by: Aditya Pande <aditya050995@gmail.com>

changed test_memory_strategy, exec entities collector to use for_each…

9784a85

…_cb_group Signed-off-by: Aditya Pande <aditya050995@gmail.com>

modified tests: lifecycle_node, allocator_memory_strategy to use for_…

e6733bf

…each_cb_group Signed-off-by: Aditya Pande <aditya050995@gmail.com>

adityapande-1995 requested review from ivanpauno and wjwwood August 9, 2021 17:34

ivanpauno reviewed Aug 9, 2021

View reviewed changes

adityapande-1995 force-pushed the aditya/thread-safe-cb branch from 58174d0 to e6733bf Compare August 9, 2021 19:44

clalancette changed the title ~~for_each_callback_group backport~~ Galactic: for_each_callback_group backport Aug 10, 2021

adityapande-1995 added 3 commits August 10, 2021 13:36

cleanup, added nullptr check to node.cpp, lifecyclenode.cpp

d77e812

Signed-off-by: Aditya Pande <aditya050995@gmail.com>

changed map_ptr to map object, removed lazy init for map

87df4a7

Signed-off-by: Aditya Pande <aditya050995@gmail.com>

added free function global_for_each_callback_group, removed dynamic p…

9eb1cf7

…ointer cast Signed-off-by: Aditya Pande <aditya050995@gmail.com>

clalancette requested changes Aug 13, 2021

View reviewed changes

deleted default contructor/destuctor, added private members to map_of…

39245ee

…_mutexes Signed-off-by: Aditya Pande <aditya050995@gmail.com>

clalancette requested changes Aug 16, 2021

View reviewed changes

cleanup, removed mutex NULL check, added underscore to private members

f94f139

Signed-off-by: Aditya Pande <aditya050995@gmail.com>

clalancette approved these changes Aug 16, 2021

View reviewed changes

cottsay approved these changes Aug 16, 2021

View reviewed changes

rclcpp/src/rclcpp/node_interfaces/node_base.cpp Outdated Show resolved Hide resolved

rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp Outdated Show resolved Hide resolved

adityapande-1995 added 2 commits August 16, 2021 14:18

removed blank line and static member access using this

e654970

Signed-off-by: Aditya Pande <aditya050995@gmail.com>

added RCLCPP_PUBLIC to global_for_each_callback_group for windows CI

85590ed

Signed-off-by: Aditya Pande <aditya050995@gmail.com>

ivanpauno approved these changes Aug 18, 2021

View reviewed changes

cottsay approved these changes Aug 18, 2021

View reviewed changes

adityapande-1995 merged commit 6adab6e into galactic Aug 19, 2021

delete-merged-branch bot deleted the aditya/thread-safe-cb branch August 19, 2021 20:41

adityapande-1995 mentioned this pull request Aug 19, 2021

malloc() : corrupted top size error in CI on build.osrfoundation.org and build.ros2.org ros-simulation/gazebo_ros_pkgs#1296

Closed

mauropasse pushed a commit to mauropasse/rclcpp that referenced this pull request Oct 13, 2023

Galactic: for_each_callback_group backport (ros2#1741)

5b66912

Added thread safe for_each_callback_group method Signed-off-by: Aditya Pande <aditya050995@gmail.com>

alsora added a commit to irobot-ros/rclcpp that referenced this pull request Oct 16, 2023

Merge pull request #125 from mauropasse/mauro/galactic-ev-ex-cherry-p…

31030fb

…ick-1741 Galactic: for_each_callback_group backport (ros2#1741)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Galactic: for_each_callback_group backport #1741

Galactic: for_each_callback_group backport #1741

adityapande-1995 commented Aug 3, 2021 •

edited

Loading

clalancette left a comment

adityapande-1995 commented Aug 4, 2021 •

edited

Loading

ivanpauno left a comment

clalancette left a comment

clalancette left a comment

clalancette left a comment

cottsay left a comment

adityapande-1995 commented Aug 16, 2021

adityapande-1995 commented Aug 17, 2021 •

edited

Loading

cottsay commented Aug 17, 2021

adityapande-1995 commented Aug 18, 2021 •

edited

Loading

ivanpauno left a comment

ivanpauno commented Aug 18, 2021

adityapande-1995 commented Aug 18, 2021

ivanpauno commented Aug 18, 2021

cottsay left a comment

adityapande-1995 commented Aug 19, 2021 •

edited

Loading

Galactic: for_each_callback_group backport #1741

Galactic: for_each_callback_group backport #1741

Conversation

adityapande-1995 commented Aug 3, 2021 • edited Loading

TODO

clalancette left a comment

Choose a reason for hiding this comment

adityapande-1995 commented Aug 4, 2021 • edited Loading

ivanpauno left a comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

cottsay left a comment

Choose a reason for hiding this comment

adityapande-1995 commented Aug 16, 2021

adityapande-1995 commented Aug 17, 2021 • edited Loading

cottsay commented Aug 17, 2021

adityapande-1995 commented Aug 18, 2021 • edited Loading

ivanpauno left a comment

Choose a reason for hiding this comment

ivanpauno commented Aug 18, 2021

adityapande-1995 commented Aug 18, 2021

ivanpauno commented Aug 18, 2021

cottsay left a comment

Choose a reason for hiding this comment

adityapande-1995 commented Aug 19, 2021 • edited Loading

adityapande-1995 commented Aug 3, 2021 •

edited

Loading

adityapande-1995 commented Aug 4, 2021 •

edited

Loading

adityapande-1995 commented Aug 17, 2021 •

edited

Loading

adityapande-1995 commented Aug 18, 2021 •

edited

Loading

adityapande-1995 commented Aug 19, 2021 •

edited

Loading