use weak_ptr for rcl entities in the memory strategy.

[fujitatomoya]

Description

i would like to discuss on this enhancement.

we should not keep the shared pointer to rcl entity handles inside the MemoryStrategy?
instead, we could use weak / non-owning references in the following vectors?
this avoids extending the lifetime of the underlying rcl entity beyond the node that owns it, while still allowing the memory strategy to observe live handles.

as you can see the test, this actually cleans up the destruction sequence.

Is this user-facing behavior change?

Did you use Generative AI?

No,

Additional Information

No,

[fujitatomoya]

@jmachowinski @skyegalaxy @wjwwood can you take a look at this when you have time?

[skyegalaxy]

lgtm, this definitely seems cleaner

[fujitatomoya]

Pulls: #2988
Gist: https://gist.githubusercontent.com/fujitatomoya/f3e38b945fe546b56afd8f7196c83352/raw/f97278a9cd02cac7983b3c887023bfb3813d5dd0/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/17516

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[jmachowinski]

This is dead code, and can be removed altogether.
This was a big part of the old executor implementation before the double buffering changes introduced shortly before jazzy. As far as I can see it it not used any more.

[fujitatomoya]

@skyegalaxy thanks for review.

@jmachowinski ah i did not remember that, thanks for point that out. i was checking the flaky result of test_allocator_memory_strategy.cpp, and trying to fix the problems.

probably what we should do is that deprecate those methods and classes, and remove the corresponding test cases.

[fujitatomoya]

@skyegalaxy thanks for review.

@jmachowinski ah i did not remember that, thanks for point that out. i was checking the flaky result of test_allocator_memory_strategy.cpp, and trying to fix the problems.

probably what we should do is that deprecate those methods and classes, and remove the corresponding test cases.

i reopend this pr because,

• since those are public APIs (not used in rclcpp anymore), we should 1st fix the problem to keep the CI stability and user application support.
• we deprecate the AllocatorMemoryStrategy and related methods and classes all together with deprecation period.
• after lyrical luth, we can remove the deprecated classes and methods with all test cases together.

i believe this is what needs to be done by procedure.

CC: @jmachowinski @skyegalaxy

[fujitatomoya]

Pulls: #2988
Gist: https://gist.githubusercontent.com/fujitatomoya/af18081bb0cced9c763ef723a95f8437/raw/8dd846b6dfd0c8bb0bda5dd26a8e4905e6fd972f/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/18054

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[jmachowinski]

@fujitatomoya Should we add a deprecation now together with this PR ?

[fujitatomoya]

@jmachowinski once we deprecate those unused classes, there were many places need to be removed to make the CI green. so i would say we can do that with another PR?

[fujitatomoya]

• Linux
• Linux-aarch64
• Linux-rhel
• Windows