[armaho]

Description

In #3178, I called has_ready_entities method of FirstInFirstOutCallbackGroupHandle without holding ready_mutex. This can result to data races while accessing ready_entities. This PR fixes that.

Is this user-facing behavior change?

Did you use Generative AI?

No

Additional Information

[github-actions]

ABI Compliance Check

✅ Verdict: compatible

Library	Verdict	Summary
libcomponent_manager.so	✅ compatible	No ABI changes detected.
librclcpp.so	✅ compatible	No ABI changes detected.
librclcpp_action.so	✅ compatible	No ABI changes detected.
librclcpp_lifecycle.so	✅ compatible	No ABI changes detected.

✅ libcomponent_manager.so — full abidiff report

Compared:

• Base: lib-base/libcomponent_manager.so
• Head: lib-pr/libcomponent_manager.so @ 67e878e

(empty report — no differences printed by abidiff)

✅ librclcpp.so — full abidiff report

Compared:

• Base: lib-base/librclcpp.so
• Head: lib-pr/librclcpp.so @ 67e878e

(empty report — no differences printed by abidiff)

✅ librclcpp_action.so — full abidiff report

Compared:

• Base: lib-base/librclcpp_action.so
• Head: lib-pr/librclcpp_action.so @ 67e878e

(empty report — no differences printed by abidiff)

✅ librclcpp_lifecycle.so — full abidiff report

Compared:

• Base: lib-base/librclcpp_lifecycle.so
• Head: lib-pr/librclcpp_lifecycle.so @ 67e878e

(empty report — no differences printed by abidiff)

_{Updated for commit 67e878e · check run · suppressions: /home/runner/work/_temp/ros2-abi-suppressions.txt}

[fujitatomoya]

lgtm with green CI.

@jmachowinski can you take a look at it just in case?

[fujitatomoya]

Pulls: #3288
Gist: https://gist.githubusercontent.com/fujitatomoya/2e111bb8d843206ce0f30666e3ee13a2/raw/27a5a0a5eeaa38ccf3f8451b413d112403e7d2e1/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/20563

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

[armaho]

@fujitatomoya

if approved, #3290 can also fix this and I think it’s a better approach.

[jmachowinski]

This PR is not valid.
@armaho in_queue is protected by the ready_callback_groups_mutex and it is always held whenever we modify the variable.
We should perhaps document this.

Taking the second mutex here is also harmful as it might introduce an lock order inversion and therefore a deadlock.

I'll close this PR as invalid.

[armaho]

@jmachowinski

I was worried about the ready_cbg->has_ready_entities() call without holding CallbackGroupHandle::ready_mutex, not the in_queue field. Since the add_ready_entity() method only holds ready_mutex while modifying the handle's internal queue.

[jmachowinski]

You are right, this is a problem....

[armaho]

@jmachowinski

Taking the second mutex here is also harmful as it might introduce an lock order inversion and therefore a deadlock.

Sorry I didn't quite understand this. Don't we take that second mutex anyways in the ready_cbg->get_next_ready_entity() call?

[jmachowinski]

You are right, we do, okay, in this case it should be fine to take it in this order.
Man... I wrote this code 2 years ago and forgot almost all details ;-)

[armaho]

Okay so this pr fixes the problem if #3290 doesn't get approved :)

[skyegalaxy]

Pulls: #3288
Gist: https://gist.githubusercontent.com/skyegalaxy/4ebc1c4326f05ad20b6faeb404a77c9b/raw/19edc67ea80399799ab2dadd164761a9aa3c8d54/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/20833

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

[skyegalaxy]

@armaho , could you please rebase on latest rolling? Looks like the removed statistics collector stuff is breaking the build for this branch

[skyegalaxy]

Pulls: #3288
Gist: https://gist.githubusercontent.com/skyegalaxy/2408fe7f3092d30955331403d11b39ea/raw/19edc67ea80399799ab2dadd164761a9aa3c8d54/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/20839

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