@ros-pull-request-builder retest this please

Pulls: #2985
Gist: https://gist.githubusercontent.com/ahcorde/a411230422d2605afd32e975ecd65fad/raw/905ba8c783b44ae6c3ff1615db00916120eb0d5b/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/17519

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

Interesting. The CI failed due to the

Package '‎rclcpp' specified with --packages-above-and-dependencies was not found

Believe me or not, I didn't delete it in this PR ;)

Pulls: #2985
Gist: https://gist.githubusercontent.com/ahcorde/107f28b5a40704d3cc70d5a20fe428d9/raw/905ba8c783b44ae6c3ff1615db00916120eb0d5b/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/17524

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

Interesting. The CI failed due to the

Package '‎rclcpp' specified with --packages-above-and-dependencies was not found

Believe me or not, I didn't delete it in this PR ;)

😄

My terminal introduced a wierd character, relaunching again

I had a second thought about the current approach, and I think it does not fix the issue, just makes it less likely. The subscriber can be deleted at any point in time, even during the execution of callback.
I guess to solve this we will need to hold a lock during callback and we will need to acquire the lock during disabling.

I had a second thought about the current approach, and I think it does not fix the issue, just makes it less likely. The subscriber can be deleted at any point in time, even during the execution of callback. I guess to solve this we will need to hold a lock during callback and we will need to acquire the lock during disabling.

But is it really a problem? The callback has its own liveliness time anyway.
Even if the subscription will be deleted at the time when the callback is executing (which shouldn't be by design since the callback is called from the executor via the dispatch method and we keep an instance of the subscription there), it will not cause any faulty situations since the callback is independent from the subscription.
The key point is to make sure that no callbacks are called after we call disable_callbacks().

But is it really a problem? The callback has its own liveliness time anyway.

Yes, the problem is that you can't figure out when it is safe to delete the object the callback is pointing to.

Lets say you have something like class MyMessageDisapatcher that handles all your callbacks from the subscriber.
You live in a lifecycle world, and by policy you delete all objects used in the node on cycling from running -> configured-> inactive -> active on system error recovery. The command to cycle comes from a service call, using a different callback group, therefore the callback is executed in a second thread. In this scenario (and by the way this is not made up, this is our setup) you can't figure out when it is safe to delete MyMessageDisapatcher. If the disable_callback call does not block or somehow ensures, that no callback is going on, code that calls disable_callbacks and afterwards directly deletes MyMessageDisapatcher still triggers an invalid memory access.

@jmachowinski, I see your use case. In your scenario, the problematic part is that you need to delete MyMessageDisapatcher with the callbacks by the service request.
I will try to implement a lock as you described inside disable_callbacks and callback dispatch functions.
However, it seems your workflow is a bit suboptimal and not robust by design.
Maybe make sense to redesign it such a way that MyMessageDisapatcher is persistent and lives as long as the node is live, but deletes/recreates subscriptions by the service request, i.e., when the lifecycle node needs to change state.

@jmachowinski Done. Please see Protect enable/disable callbacks and callbacks execution with mutex

@jmachowinski, kindly ping here to reiterate on the review of the last two commits.

Pulls: #2985
Gist: https://gist.githubusercontent.com/MichaelOrlov/858df3b489887929e03235892f22539a/raw/905ba8c783b44ae6c3ff1615db00916120eb0d5b/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/17604

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

The CI is yellowish, but it seems it is unrelated.
Does anybody know if this is okay and expected to be this way?

Ahh, I've figured it out. The CI is yellowish due to the new warnings

‘void rclcpp::spin_some(Node::SharedPtr)’ is deprecated: use SingleThreadedExecutor::spin_some instead [-Wdeprecated-declarations] 346 | rclcpp::spin_some(node_);

That is because I was using a deprecated rclcpp::spin_some(node_) in the newly added tests.

I've just fixed it with the new commit by using spin_some() from the rclcpp::executors::SingleThreadedExecutor
directly.

Re-run CI jobs that had warnings

• Linux
• Linux-aarch64
• Linux-rhel

@ros-pull-request-builder retest this please

Re-run Linux CI job to see if it will pass after addressing pub-sub tests failure

• Linux

The Linux CI job passed successfully. Re-running another two

• Linux-aarch64
• Linux-rhel

All CI jobs passed green. Merging then.