feat: Added callback group events executor by jmachowinski · Pull Request #3097 · ros2/rclcpp

jmachowinski · 2026-03-13T17:29:50Z

This commit adds the callback group events executor. It features:

multithreading support
correct handling of sim time
usage of the events subsystem

Description

This moved the events cbg executor from cm_executors into rlcpp mainline.

Did you use Generative AI?

No

alsora · 2026-03-15T13:47:51Z

@jmachowinski can you add this to the executor unit-tests?

jmachowinski · 2026-03-23T14:16:14Z

Pulls: #3097
Gist: https://gist.githubusercontent.com/jmachowinski/f9f71b19f46697be097713df6f9f6016/raw/f0f3fa2b988318178c70a0f1c9f90dbc50321ee8/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/18616

Linux
Linux-aarch64
Linux-rhel
Windows

skyegalaxy

Aside from some spelling nits and a handful of small implementation questions, this is looking great! Very excited to get this over the finish line soon.

skyegalaxy · 2026-03-23T16:15:01Z

+    }
+  }
+
+  // FIXME inform scheduler about remove cbgs


what are the side-effects of leaving this as is and not informing the scheduler here?

mini-1235

Will/should this replace the experimental event executor here?

rclcpp/rclcpp_components/src/component_container_event.cpp

Line 26 in 1e07045

    
           auto exec = std::make_shared<rclcpp::experimental::executors::EventsExecutor>();

jmachowinski · 2026-03-27T18:26:27Z

Will/should this replace the experimental event executor here?

rclcpp/rclcpp_components/src/component_container_event.cpp

Line 26 in 1e07045

auto exec = std::make_shared<rclcpp::experimental::executors::EventsExecutor>();

yes, that is the idea.

skyegalaxy · 2026-04-01T18:01:25Z

+
+  // as, we remove an reappend ready callback_groups during execution,
+  // the first ready cbg may not contain the lowest id. Therefore we
+  // need to search the whole deque


Not a blocker for this PR, but I think it would be interesting to see if we could find a way to efficiently index ready_callback_groups by ID as well as the order, so that instead of searching through the entire deque, we could do something like ready_callback_groups.pop_by_id(max_id)

skyegalaxy · 2026-04-01T18:14:20Z

@jmachowinski we also need DCO for some of the newer commits

skyegalaxy

lgtm! can we rebase on latest rolling before running CI?

jmachowinski · 2026-04-08T09:24:32Z

This currently depends on ros2/ament_cmake_ros#62

SteveMacenski · 2026-04-08T20:59:00Z

Per a discussion with @skyegalaxy today: would it be possible to get the component container + isolated variant of this? I can have this as a tested variant in Nav2 then

skyegalaxy · 2026-04-08T21:05:08Z

@SteveMacenski - I see that there are these two PRs #3055, #3080 to try and generalize the component container across executors, but I'm not sure if that'll get in before the feature freeze. Absent that, we could probably just create a dedicated one for now

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

jmachowinski · 2026-04-18T13:50:23Z

Pulls: #3097
Gist: https://gist.githubusercontent.com/jmachowinski/f1346405fe53fb47db78a17e69302a38/raw/d9e8b77750a3d78e0cde45da82566294c32e4a05/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/18993

Linux
Linux-aarch64
Linux-rhel
Windows

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

This introduced a subtle bug, were the guard condition of the callback group would not trigger a resync of the callback groups. Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>

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

This also fixes bugs were the executor would not wake up. Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>

skyegalaxy · 2026-04-19T13:54:20Z

Pulls: #3097
Gist: https://gist.githubusercontent.com/jmachowinski/f1346405fe53fb47db78a17e69302a38/raw/d9e8b77750a3d78e0cde45da82566294c32e4a05/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/18998/

Linux
Linux-aarch64
Linux-rhel
Windows

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

This is the same place as were they are set, so this makes the logic easier. Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>

This fixes a segfault, as the callbackgroup was deleted were the logic did not explect it. Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>

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

skyegalaxy · 2026-04-19T15:54:47Z

Linux
Linux-aarch64
Linux-rhel
Windows

This fixes a performance regression as we would wake threads for no reason in single cbg processing. Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>

skyegalaxy · 2026-04-19T16:22:19Z

Linux
Linux-aarch64
Linux-rhel
Windows

* feat: Added callback group events executor This commit adds the callback group events executor. It features: - multithreading support - correct handling of sim time - usage of the events subsystem Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> * fix: Moved implementation into own namespace cbg_executor Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com> * chore: Moved out of experimental Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com> * chore: cleanup Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com> * feat: Refactored and extended executor benchmarks Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com> * tests: Added EventsCBGExecutor to executor tests Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> * fix: Call timer->call() from the execution context, not the timer manager This code was copied straight from the executor and seems to be a workaround for the multithreaded executor, that breaks in this use case. The correct solution for us is to do the timer->call() from within the worker thread. This fixes a deadlock due to double acquisition of an internal lock within the timer manager. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> * perf: Don't wake the timer thread if not needed In case the next timer wakeup time is not changed by an insertion of a timer, don't wake up the timer thread. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> * test: Added test if service works as expected with executor Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> * chore: cpplint fix Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> * chore: fixed typos Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> * fix: Removed hack for getting clock from timer Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> * chore: more typos Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> * chore: Minor cleanups Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> * chore: fix compiler warning Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> * fix: Implemented get_xyz_callback_groups functions properly Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com> * fix: Don't use moved function This introduced a subtle bug, were the guard condition of the callback group would not trigger a resync of the callback groups. Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com> * fix: Wake executor if node needs resync Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com> * fix: Unified resync code This also fixes bugs were the executor would not wake up. Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com> * fix: Don't sync if in shutdown Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com> * fix: Clear callbacks in RegisteredEntityCache This is the same place as were they are set, so this makes the logic easier. Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com> * fix: Don't sync in sync callback, only signal that a sync is needed This fixes a segfault, as the callbackgroup was deleted were the logic did not explect it. Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com> * fix: Resync in collect_and_execute_ready_events during execution Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com> * fix: Clear node guard condition cb on removal Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com> * fix: Clear guard condition callback even if cbg was already deleted Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com> * fix: Don't wake more threads on sync event This fixes a performance regression as we would wake threads for no reason in single cbg processing. Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com> --------- Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com> Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com>

jmachowinski force-pushed the add_cbg_executor branch from 273c322 to 66467b3 Compare March 13, 2026 17:30