Skip to content

clear handles before node destruction in test_memory_strategy.#2969

Merged
fujitatomoya merged 1 commit intorollingfrom
fujitatomoya/issues/2968
Oct 20, 2025
Merged

clear handles before node destruction in test_memory_strategy.#2969
fujitatomoya merged 1 commit intorollingfrom
fujitatomoya/issues/2968

Conversation

@fujitatomoya
Copy link
Copy Markdown
Collaborator

Description

Fixes #2968

Is this user-facing behavior change?

Did you use Generative AI?

Additional Information

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya fujitatomoya requested a review from Copilot October 20, 2025 06:12
@fujitatomoya fujitatomoya self-assigned this Oct 20, 2025
Copy link
Copy Markdown
Collaborator Author

@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.

the memory strategy is holding owning references to the underlying C rcl handles (shared pointer of rcl_service_t / rcl_subscription_t etc).
those shared_ptrs keep the rcl entities alive past the lifetime of the Node (which un-initializes the rcl_node implementation).
when the rcl service/subscription is finally destroyed, rcl tries to access the node implementation which is already invalid
this generates the error messages during this test.

the objective with those tests are not lifetime of endpoint objects against the node.
these tests basically check if memory strategy APIs work okay with each endpoint.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Purpose: Ensure test cases manually clear memory strategy handles before node destruction to avoid accessing destroyed nodes via weak pointers and prevent potential cleanup issues.
Key changes:

  • Added calls to memory_strategy()->clear_handles() inside multiple test scopes.
  • Added client_handle.reset() in get_client_by_handle test to prevent deleter accessing node.
  • Inserted identical explanatory comments about clearing handles before node destruction.

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

(i do not think this blocks this PR merge.)

i would like to discuss on the following enhancement as well.

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.

VectorRebind<std::shared_ptr<const rcl_subscription_t>> subscription_handles_;
VectorRebind<std::shared_ptr<const rcl_service_t>> service_handles_;
VectorRebind<std::shared_ptr<const rcl_client_t>> client_handles_;
VectorRebind<std::shared_ptr<const rcl_timer_t>> timer_handles_;
VectorRebind<std::shared_ptr<Waitable>> waitable_handles_;

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

fujitatomoya commented Oct 20, 2025

Pulls: #2969
Gist: https://gist.githubusercontent.com/fujitatomoya/97f5d3b73ec7f23155287ca3d7ed8730/raw/da2e3f4ce221c7638d092cc511b37b117fcb6f97/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/17316

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

@fujitatomoya fujitatomoya merged commit db4f9c2 into rolling Oct 20, 2025
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🧑‍🌾 rclcpp.test_memory_strategy returns -11 in CI

3 participants