add support for reentrant callback group to EventsCBGExecutor by armaho · Pull Request #3178 · ros2/rclcpp

armaho · 2026-06-18T15:40:36Z

Description

Fixes #3175

Is this user-facing behavior change?

Did you use Generative AI?

No

Additional Information

Signed-off-by: Arman Hosseini <armahosseini878787@gmail.com>

skyegalaxy

lgtm with green CI. will let @jmachowinski take a pass as well

skyegalaxy · 2026-06-18T15:58:02Z

+      {
+        ready_callback_groups.erase(it);
+      }
+      return CBGScheduler::ExecutableEntityWithInfo{


note to self to remove C++20 initialization when backporting this to Jazzy and Kilted specifically

skyegalaxy · 2026-06-18T17:11:27Z

actually @armaho can you add a test which exercises the reentrant path like we discussed at the client library WG meeting?

armaho · 2026-06-19T10:31:50Z

@skyegalaxy

Sure let me see how to test it.

mergify · 2026-06-19T13:28:15Z

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

Queue this pull request

…er EventCBGExecutor Signed-off-by: Arman Hosseini <armahosseini878787@gmail.com>

armaho · 2026-06-19T13:31:58Z

@skyegalaxy

I just added a test covering the reentrant callback group behavior. Could you take another look when you have time?

jmachowinski

Sorry for the late reply.
I wonder if it makes sense to introduce a second class of callback handle, providing a seperate logic.
In this case the ready queue would be something like
std::deque<std::variant<ReentrantCallbackGroupHandle *, MutalExclusiveCallbackGroupHandle *>

Even though this would lead to some code duplication, I think it would make the code easier and better understandable in the end. The current approach of selectively doing things made it hard to understand what is going on.

I also think the compiler might optimize the variant better.

jmachowinski · 2026-06-24T11:40:54Z

+
+      // Reentrant callback groups might not be removed from the queue when one of
+      // their entities starts executing.
+      if (handle->get_type() == CallbackGroupType::Reentrant) {
+        bool already_in_queue = false;
+
+        for (auto it = ready_callback_groups.begin(); it != ready_callback_groups.end(); it++) {
+          if (*it == handle) {
+            already_in_queue = true;
+            break;
+          }
+        }
+
+        if (!already_in_queue) {
+          ready_callback_groups.push_back(handle);
+        }
+      } else {
+        ready_callback_groups.push_back(handle);
+      }


This is a big no go, you introduce O(n) characteristics by searching the ready list.
This code should actually not be needed as the callback group handle should hold the information
if it is in the queue or not.

I wasn't able to find anything in the handle itself that indicates whether it's in the queue or not. There was an idle field, but I had trouble using it to determine that. I ended up adding an in_queue field to the handle instead, which is simple to understand and implement. What do you think about that?

skyegalaxy · 2026-07-02T17:05:04Z

@armaho - do you have bandwidth to respond to the feedback / still want to work on this?

armaho · 2026-07-02T17:10:34Z

@skyegalaxy Yes. Sorry I’ve been busy I will address the problems of current code and check out that variant idea tomorrow and get back to you with the results.

Signed-off-by: Arman Hosseini <armahosseini878787@gmail.com>

armaho · 2026-07-03T13:40:03Z

@jmachowinski

I also think that the current implementation of EventCBGExecutor is hard to understand. At least, it was for me.

But I don't think that creating a separate handle for reentrant cbgs would help that much. Right now, the only conditional piece of code within the CallbackGroupHandle itself is inside mark_as_executing:

void mark_as_executing()
{
  if (type != CallbackGroupType::Reentrant) {
    not_ready = true;
  }
}

Which is not hard to understand, and I don't think that creating a separate handle would fundamentally reduce the conditional logic elsewhere in the code. To me, it doesn’t seem worth the amount of code duplication it would introduce.

That said, you know this codebase much better than I do, so I'll go in whichever direction you think is best.

jmachowinski · 2026-07-03T16:03:46Z

The commit looks good so far. I started playing around with the API to see if splitting the handle base class makes it more readable. I'll report back next week. Have a nice weekend.

jmachowinski · 2026-07-15T16:48:47Z

Pulls: #3178
Gist: https://gist.githubusercontent.com/jmachowinski/263c67751e131d384f43e888d5e1d7e8/raw/3baffe34b4da0f4ab0f1c5da02d1a636647bd8cb/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/19863

Linux
Linux-aarch64
Linux-rhel
Windows

jmachowinski · 2026-07-15T16:53:01Z

I don't have the capacity right now to look into making the code prettier.
Therefore I am going to merge this fix as it fixes the issue (if we get green CI).

Perhaps I'll have time later to play around with the code...

armaho · 2026-07-16T12:05:52Z

@skyegalaxy
@jmachowinski

Thank you both for your reviews. I'm working on ROS2 executors right now and I might come back here and work on this a bit further in the future.

Is there anything else I should do for now? :)

jmachowinski · 2026-07-16T14:46:12Z

@Mergifyio backport lyrical

mergify · 2026-07-16T14:46:37Z

backport lyrical

✅ Backports have been created

Details

#3200 add support for reentrant callback group to EventsCBGExecutor (backport #3178) has been created for branch lyrical

skyegalaxy · 2026-07-16T14:52:50Z

could we backport this to jazzy and kilted also?

jmachowinski · 2026-07-16T14:56:42Z

You wrote a note to yourself to adjust the code to c++17.... ;-P

add support for reentrant callback group to EventsCBGExecutor

9fb7aa2

Signed-off-by: Arman Hosseini <armahosseini878787@gmail.com>

skyegalaxy requested review from jmachowinski and skyegalaxy and removed request for jmachowinski June 18, 2026 16:00

skyegalaxy approved these changes Jun 18, 2026

View reviewed changes

add test to check that reentrant callback groups run concurrently und…

855d1b2

…er EventCBGExecutor Signed-off-by: Arman Hosseini <armahosseini878787@gmail.com>

armaho force-pushed the events_cbg_executor_reentrant branch from 554512d to 855d1b2 Compare June 19, 2026 13:29

jmachowinski requested changes Jun 24, 2026

View reviewed changes

address change requests

490c6b0

Signed-off-by: Arman Hosseini <armahosseini878787@gmail.com>

armaho force-pushed the events_cbg_executor_reentrant branch from cfebaef to 490c6b0 Compare July 3, 2026 13:20

armaho requested a review from jmachowinski July 3, 2026 13:29

jmachowinski self-assigned this Jul 9, 2026

jmachowinski approved these changes Jul 15, 2026

View reviewed changes

jmachowinski merged commit 0f0b693 into ros2:rolling Jul 16, 2026
3 checks passed

mergify Bot mentioned this pull request Jul 16, 2026

add support for reentrant callback group to EventsCBGExecutor (backport #3178) #3200

Open

This was referenced Jul 17, 2026

add support for reentrant callback group to EventsCBGExecutor (Backport #3178) #3205

Open

add support for reentrant callback group to EventsCBGExecutor (Backport #3178) #3204

Open

Uh oh!

Conversation

armaho commented Jun 18, 2026

Description

Is this user-facing behavior change?

Did you use Generative AI?

Additional Information

Uh oh!

skyegalaxy left a comment

Choose a reason for hiding this comment

Uh oh!

skyegalaxy Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

skyegalaxy commented Jun 18, 2026

Uh oh!

armaho commented Jun 19, 2026

Uh oh!

mergify Bot commented Jun 19, 2026

Uh oh!

armaho commented Jun 19, 2026

Uh oh!

jmachowinski left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmachowinski Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

armaho Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

skyegalaxy commented Jul 2, 2026

Uh oh!

armaho commented Jul 2, 2026

Uh oh!

armaho commented Jul 3, 2026

Uh oh!

jmachowinski commented Jul 3, 2026

Uh oh!

jmachowinski commented Jul 15, 2026

Uh oh!

jmachowinski commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

armaho commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jmachowinski commented Jul 16, 2026

Uh oh!

mergify Bot commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

skyegalaxy commented Jul 16, 2026

Uh oh!

jmachowinski commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmachowinski left a comment •

edited

Loading

jmachowinski commented Jul 15, 2026 •

edited

Loading

armaho commented Jul 16, 2026 •

edited

Loading

mergify Bot commented Jul 16, 2026 •

edited

Loading

jmachowinski commented Jul 16, 2026 •

edited

Loading