clear handles before node destruction in test_memory_strategy.#2969
clear handles before node destruction in test_memory_strategy.#2969fujitatomoya merged 1 commit intorollingfrom
Conversation
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
fujitatomoya
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
(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. rclcpp/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp Lines 502 to 506 in 715a983 |
|
Pulls: #2969 |
Description
Fixes #2968
Is this user-facing behavior change?
Did you use Generative AI?
Additional Information