Executor strong reference fix by jmachowinski · Pull Request #2745 · ros2/rclcpp

jmachowinski · 2025-02-14T13:37:50Z

@mjcarroll we should merge this ASAP and backport to jazzy, to keep the ABI breakage to one release

jmachowinski · 2025-02-14T13:46:00Z

Pulls: #2745
Gist: https://gist.githubusercontent.com/jmachowinski/3a04e1e78a56e9e964d4ae0ac2ae01fc/raw/ddfb92e12cf7c0ce9b6ecc81882b328e6034df2b/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15193

Linux
Linux-aarch64
Linux-rhel
Windows

rclcpp/include/rclcpp/wait_result.hpp

christophebedard · 2025-02-14T16:14:05Z

rclcpp/include/rclcpp/wait_result.hpp

+          if(ret) {
+            break;
+          }


Suggested change

if(ret) {

break;

}

if (ret) {

break;

}

christophebedard · 2025-02-14T16:14:13Z

rclcpp/include/rclcpp/wait_result.hpp

+          if(ret) {
+            break;
+          }


Suggested change

if(ret) {

break;

}

if (ret) {

break;

}

christophebedard · 2025-02-14T16:14:37Z

rclcpp/include/rclcpp/wait_result.hpp

+          if(ret) {
+            break;
+          }


Suggested change

if(ret) {

break;

}

if (ret) {

break;

}

christophebedard · 2025-02-14T16:16:13Z

rclcpp/test/rclcpp/executors/test_executors.cpp

+
+  // wait for both subs to be connected
+  auto max_end_time = std::chrono::steady_clock::now() + std::chrono::milliseconds(1500);
+  while((sub1->get_publisher_count() == 0) || (sub2->get_publisher_count() == 0)) {


Suggested change

while((sub1->get_publisher_count() == 0) || (sub2->get_publisher_count() == 0)) {

while ((sub1->get_publisher_count() == 0) || (sub2->get_publisher_count() == 0)) {

christophebedard · 2025-02-14T16:16:20Z

rclcpp/test/rclcpp/executors/test_executors.cpp

+  // publish some messages, until one subscriber fired. As both subscribers are
+  // connected to the same topic, they should fire in the same wait.
+  max_end_time = std::chrono::steady_clock::now() + std::chrono::milliseconds(100);
+  while(!sub1_works && !sub2_works) {


Suggested change

while(!sub1_works && !sub2_works) {

while (!sub1_works && !sub2_works) {

wjwwood

lgtm with style fixups

But, I do not believe it fixes #2678, it simply makes it less likely (at least for the multi-threaded executor case or two separate executor case). If async .reset() happens after the executor has acquired the lock, but before it starts executing the callback then you'll still potentially get into a callback after the "owning" class was reset, thus you still need to capture a weak_ptr in the callback.

The right answer to #2678 is documentation, imo.

This pr is still worth doing because it frees up resources more quickly, but it doesn't solve the original problem completely.

mjcarroll · 2025-02-15T14:41:27Z

@marcoag FYI we want to get this backported to Jazzy and it does end up with an ABI break.

I think it's important enough to get in, though. Let's coordinate that this gets in before the next sync.

fujitatomoya

lgtm with green CI. just a minor comment.

rclcpp/test/rclcpp/executors/test_executors.cpp

ros2#2678 reported that the executor was holding strong references to entities during execution. This commit adds a regression test for this. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

This fixes a bug, were dropping an entity during a callback would not prevent further callbacks. Note, there might still be a race in the case of the mutithreaded executor. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

jmachowinski · 2025-02-17T09:38:02Z

Pulls: #2745
Gist: https://gist.githubusercontent.com/jmachowinski/b0a8898b10d9d067c61f5b40f4940d9b/raw/ddfb92e12cf7c0ce9b6ecc81882b328e6034df2b/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15204

Linux
Linux-aarch64
Linux-rhel
Windows

marcoag · 2025-02-20T06:45:53Z

https://github.com/Mergifyio backport jazzy

mergify · 2025-02-20T06:46:00Z

backport jazzy

✅ Backports have been created

Details

#2754 Executor strong reference fix (backport #2745) has been created for branch jazzy

#2678 reported that the executor was holding strong references to entities during execution. This commit adds a regression test for this. * fix(Executor): Don't hold strong references to entities during spin This fixes a bug, were dropping an entity during a callback would not prevent further callbacks. Note, there might still be a race in the case of the mutithreaded executor. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com> (cherry picked from commit 9c493c4)

#2678 reported that the executor was holding strong references to entities during execution. This commit adds a regression test for this. * fix(Executor): Don't hold strong references to entities during spin This fixes a bug, were dropping an entity during a callback would not prevent further callbacks. Note, there might still be a race in the case of the mutithreaded executor. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com> (cherry picked from commit 9c493c4) Co-authored-by: Janosch Machowinski <jmachowinski@users.noreply.github.com>

mjcarroll approved these changes Feb 14, 2025

View reviewed changes

mjcarroll self-assigned this Feb 14, 2025

christophebedard reviewed Feb 14, 2025

View reviewed changes

wjwwood approved these changes Feb 14, 2025

View reviewed changes

fujitatomoya approved these changes Feb 15, 2025

View reviewed changes

rclcpp/test/rclcpp/executors/test_executors.cpp Show resolved Hide resolved

alsora approved these changes Feb 15, 2025

View reviewed changes

Janosch Machowinski added 2 commits February 17, 2025 10:35

test(Executors): Added test case for dropping of entity in callback

cfd6490

ros2#2678 reported that the executor was holding strong references to entities during execution. This commit adds a regression test for this. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

jmachowinski force-pushed the executor_strong_reference_fix branch from c8bdffc to c553699 Compare February 17, 2025 09:36

mjcarroll merged commit 9c493c4 into ros2:rolling Feb 20, 2025
3 checks passed

mergify bot mentioned this pull request Feb 20, 2025

Executor strong reference fix (backport #2745) #2754

Merged

ewak mentioned this pull request Mar 14, 2025

Investigate adding in explicit IPC for Nav2 nodes again ros-navigation/navigation2#4691

Closed

	while((sub1->get_publisher_count() == 0) \|\| (sub2->get_publisher_count() == 0)) {
	while ((sub1->get_publisher_count() == 0) \|\| (sub2->get_publisher_count() == 0)) {

	while(!sub1_works && !sub2_works) {
	while (!sub1_works && !sub2_works) {

Conversation

jmachowinski commented Feb 14, 2025

Uh oh!

jmachowinski commented Feb 14, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

mjcarroll commented Feb 15, 2025

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmachowinski commented Feb 17, 2025

Uh oh!

Uh oh!

marcoag commented Feb 20, 2025

Uh oh!

mergify bot commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mergify bot commented Feb 20, 2025 •

edited

Loading