Test case and fix for for https://github.com/ros2/rclcpp/issues/2652 by jmachowinski · Pull Request #2713 · ros2/rclcpp

jmachowinski · 2024-12-17T16:11:59Z

This is a test case and fix for the bug #2652

jmachowinski · 2024-12-17T17:19:57Z

@luca-della-vedova @fmrico can you test this fix ?

rclcpp/test/rclcpp/executors/test_executors.cpp

luca-della-vedova

I tried on the reproduction repo and this branch indeed fixes it!

fmrico · 2024-12-19T05:54:32Z

@luca-della-vedova @fmrico can you test this fix ?

Sure!! testing...

fmrico · 2024-12-19T07:13:48Z

I have tested this PR against:

rclcpp tests, including the ones in Add executor tests for testing destroying nodes cellumation/rclcpp#3
Tests in https://github.com/fmrico/cascade_lifecycle just before the commits related to this bug in Explicit removal of node from executor before destroy fmrico/cascade_lifecycle#14
Tests in https://github.com/PlanSys2/ros2_planning_system using SingleThreadedExecutor instead of EventsExecutor, which I had to use because the bug related to this fix (Maintenance rolling PlanSys2/ros2_planning_system#332)

Everything is working now: LGTM 🔥 🚀

fujitatomoya

@jmachowinski lgtm with green CI.

i would like to confirm one thing. how come do you consider this breaks ABI by the way? Templated functions are instantiated at compile-time, and their changes do not affect the binary interface (ABI) of the library.

rclcpp/test/rclcpp/executors/test_executors.cpp

jmachowinski · 2025-01-08T17:08:25Z

@jmachowinski lgtm with green CI.

i would like to confirm one thing. how come do you consider this breaks ABI by the way? Templated functions are instantiated at compile-time, and their changes do not affect the binary interface (ABI) of the library.

This specific case is likely not to break ABI, but leaves you in a bad place. Depending on what was compiled at which time, you end up with the fix or not. E.g. you have libraries, that use the Executor therefore they contain a symbol of the Executor. These libraries were compiled before the fix. Even though you now compile with the new fixed template, the linker will choose one symbol to use later on during linking, and may use the 'broken' version from the library.

In general, you can break ABI with templates, but you need to have a libraries. To break ABI, you need a to have a templated type, that changed its memory layout due to an alteration of the template. If this type is used by library A, but this was compiled against the old version of the template, and you now try to use the library with the new template, you get an ABI break, and most likely a crash...

mjcarroll · 2025-01-08T20:43:49Z

As we discussed in PMC, this doesn't really break ABI, but updating rclcpp by itself is insufficient to get the fix. You must also recompile any downstream users of the template. It's enough to justify a notification, and we are planning on doing a discourse post about it.

fujitatomoya · 2025-01-09T20:19:33Z

Pulls: #2713
Gist: https://gist.githubusercontent.com/fujitatomoya/6af50bf834f3f8281e47c44d8829eba0/raw/535be838d71dec82d719fa76bfd467d8419dd27a/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15047

Linux
Linux-aarch64
Linux-rhel
Windows

fujitatomoya · 2025-01-10T16:46:01Z

Pulls: #2713
Gist: https://gist.githubusercontent.com/fujitatomoya/42828e7e96e4dbe26cbb029b0587a9ac/raw/535be838d71dec82d719fa76bfd467d8419dd27a/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15051

Linux
Linux-aarch64
Linux-rhel
Windows

fujitatomoya · 2025-01-10T16:46:09Z

just in case i started the CI after rebasing.

wjwwood

lgtm, but I had one question that might lead to an edit

rclcpp/include/rclcpp/wait_set_policies/dynamic_storage.hpp

wjwwood · 2025-01-10T17:42:11Z

rclcpp/include/rclcpp/wait_set_policies/dynamic_storage.hpp

+    if(this->needs_pruning_) {
+      this->storage_prune_deleted_entities();
+      this->needs_pruning_ = false;
+    }
+
    this->storage_release_ownerships();


Should this check and the pruning happen after releasing ownerships? It seems like holding the ownership by the storage might cause the weak pointers to have not expired even though all the user's shared pointers have been destroyed? Maybe it gets caught on the next run through?

I suppose this would only happen if something lost all ownership (except the wait set's) after needs_pruning_ became true but before this step.

Either way, since storage_prune_deleted_entities() iterates through all of the wait set's weak pointers and checks them for expired, we might as well release ownerships first no?

This needs to happen while holding the ownership, in order to be sure that the indices still match. If you release ownership, and prune, and some references were dropped in between, the indices of the rcl waitset will not match any more the one in the storage. Basically leading to the same bug as before.
I don't like this implicit matching of indixes in general. I would like to introduce a backmapping datastructure later on, that uses the indices returned by the rcl_add functions

@wjwwood does this answer your question ?

rclcpp/include/rclcpp/wait_set_policies/static_storage.hpp

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

Signed-off-by: Francisco Martín Rico <fmrico@gmail.com>

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

jmachowinski · 2025-02-07T12:57:08Z

Pulls: #2713
Gist: https://gist.githubusercontent.com/jmachowinski/7423371ee1072d9222ca5cea3688573c/raw/6476e941aba254b1fc4f99d986f6f67573799ddc/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15133

Linux
Linux-aarch64
Linux-rhel
Windows

jmachowinski · 2025-02-10T09:09:48Z

Pulls: #2713
Gist: https://gist.githubusercontent.com/jmachowinski/4135dd462926c71e91b22f5c7ae408f8/raw/6476e941aba254b1fc4f99d986f6f67573799ddc/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15145

Linux
Linux-aarch64
Linux-rhel
Windows

mjcarroll · 2025-02-10T18:46:54Z

Only flakes on Windows here looks like.

jmachowinski · 2025-02-10T19:02:10Z

The windows test is my pi something. It's been there in all recent ci runs and seems unrelated.

jmachowinski · 2025-02-11T17:21:09Z

@Mergifyio backport jazzy

mergify · 2025-02-11T17:21:28Z

backport jazzy

❌ Command disallowed due to command restrictions in the Mergify configuration.

Details

sender-permission >= write

jmachowinski · 2025-02-11T17:22:01Z

@fujitatomoya could you trigger the backport ?

fujitatomoya · 2025-02-11T17:23:02Z

@jmachowinski yeah i will take care of that.

fujitatomoya · 2025-02-11T17:23:12Z

@Mergifyio backport jazzy

mergify · 2025-02-11T17:23:26Z

backport jazzy

✅ Backports have been created

Details

#2740 Test case and fix for for https://github.com/ros2/rclcpp/issues/2652 (backport #2713) has been created for branch jazzy

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> Signed-off-by: Francisco Martín Rico <fmrico@gmail.com> Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com> Co-authored-by: Francisco Martín Rico <fmrico@gmail.com> (cherry picked from commit 605251b)

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> Signed-off-by: Francisco Martín Rico <fmrico@gmail.com> Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com> Co-authored-by: Francisco Martín Rico <fmrico@gmail.com> (cherry picked from commit 605251b) Co-authored-by: Janosch Machowinski <jmachowinski@users.noreply.github.com>

jmachowinski force-pushed the executor_index_bug branch from 6906974 to 660ffaa Compare December 17, 2024 17:18

jmachowinski changed the title ~~Test case for https://github.com/ros2/rclcpp/issues/2652~~ Test case and fix for for https://github.com/ros2/rclcpp/issues/2652 Dec 17, 2024

jmachowinski marked this pull request as ready for review December 17, 2024 17:20

alsora reviewed Dec 18, 2024

View reviewed changes

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

luca-della-vedova approved these changes Dec 18, 2024

View reviewed changes

jmachowinski force-pushed the executor_index_bug branch from 660ffaa to e3142d0 Compare December 18, 2024 12:06

fmrico mentioned this pull request Dec 19, 2024

Add executor tests for testing destroying nodes cellumation/rclcpp#3

Merged

fujitatomoya approved these changes Jan 8, 2025

View reviewed changes

jmachowinski force-pushed the executor_index_bug branch from 78d859d to 27b2d0e Compare January 10, 2025 15:16

wjwwood approved these changes Jan 10, 2025

View reviewed changes

jmachowinski force-pushed the executor_index_bug branch from 27b2d0e to 8f8aab8 Compare January 17, 2025 12:59

Janosch Machowinski and others added 5 commits February 7, 2025 10:39

test: Added test for broken executor behavior

5d83c11

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

fix: Prune dynamic storage if needed

959b86e

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

fix(static_storage): Throw if an invalid entry was detected

b7c45f0

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

Add executor tests for testing destroying nodes

dc62e9e

Signed-off-by: Francisco Martín Rico <fmrico@gmail.com>

chore: style and warning fixes

0c42945

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

jmachowinski force-pushed the executor_index_bug branch from 8f8aab8 to 0c42945 Compare February 7, 2025 09:48

mjcarroll merged commit 605251b into ros2:rolling Feb 11, 2025
2 of 3 checks passed

mergify bot mentioned this pull request Feb 11, 2025

Test case and fix for for https://github.com/ros2/rclcpp/issues/2652 (backport #2713) #2740

Merged

Conversation

jmachowinski commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmachowinski commented Dec 17, 2024

Uh oh!

Uh oh!

luca-della-vedova left a comment

Choose a reason for hiding this comment

Uh oh!

fmrico commented Dec 19, 2024

Uh oh!

fmrico commented Dec 19, 2024

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmachowinski commented Jan 8, 2025

Uh oh!

mjcarroll commented Jan 8, 2025

Uh oh!

fujitatomoya commented Jan 9, 2025

Uh oh!

fujitatomoya commented Jan 10, 2025

Uh oh!

fujitatomoya commented Jan 10, 2025

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wjwwood Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

jmachowinski Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

jmachowinski Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmachowinski commented Feb 7, 2025

Uh oh!

jmachowinski commented Feb 10, 2025

Uh oh!

mjcarroll commented Feb 10, 2025

Uh oh!

jmachowinski commented Feb 10, 2025

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

mergify bot commented Feb 11, 2025

❌ Command disallowed due to command restrictions in the Mergify configuration.

Uh oh!

jmachowinski commented Feb 11, 2025

Uh oh!

fujitatomoya commented Feb 11, 2025

Uh oh!

fujitatomoya commented Feb 11, 2025

Uh oh!

mergify bot commented Feb 11, 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!

jmachowinski commented Dec 17, 2024 •

edited

Loading

jmachowinski commented Feb 11, 2025 •

edited

Loading

mergify bot commented Feb 11, 2025 •

edited

Loading