Fix possible data race while calling has_ready_entities in EventsCBGExecutor

[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: incompatible

Library	Verdict	Summary
libcomponent_manager.so	✅ compatible	No ABI changes detected.
librclcpp.so	❌ incompatible	ABI-incompatible 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 @ 56b11bd

(empty report — no differences printed by abidiff)

❌ librclcpp.so — full abidiff report

Compared:

• Base: lib-base/librclcpp.so
• Head: lib-pr/librclcpp.so @ 56b11bd

Functions changes summary: 10 Removed (46 filtered out), 2 Changed (134 filtered out), 2 Added (3 filtered out) functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
Function symbols changes summary: 4 Removed, 2 Added function symbols not referenced by debug info
Variable symbols changes summary: 0 Removed, 0 Added variable symbol not referenced by debug info

10 Removed functions:

  [D] 'method void rclcpp::executors::cbg_executor::CBGScheduler::block_worker_thread(rclcpp::executors::cbg_executor::Worker*)'    {_ZN6rclcpp9executors12cbg_executor12CBGScheduler19block_worker_threadEPNS1_6WorkerE}
  [D] 'method void rclcpp::executors::cbg_executor::CBGScheduler::block_worker_thread_for(rclcpp::executors::cbg_executor::Worker*, std::chrono::nanoseconds)'    {_ZN6rclcpp9executors12cbg_executor12CBGScheduler23block_worker_thread_forEPNS1_6WorkerENSt6chrono8durationIlSt5ratioILl1ELl1000000000EEEE}
  [D] 'method bool rclcpp::executors::cbg_executor::CBGScheduler::prepare_and_enqueue_worker(rclcpp::executors::cbg_executor::Worker*)'    {_ZN6rclcpp9executors12cbg_executor12CBGScheduler26prepare_and_enqueue_workerEPNS1_6WorkerE}
  [D] 'method void rclcpp::executors::cbg_executor::CBGScheduler::suppress_thread_wakeup()'    {_ZN6rclcpp9executors12cbg_executor12CBGScheduler22suppress_thread_wakeupEv}
  [D] 'method void rclcpp::executors::cbg_executor::Worker::block()'    {_ZN6rclcpp9executors12cbg_executor6Worker5blockEv}
  [D] 'method void rclcpp::executors::cbg_executor::Worker::block_for(std::chrono::nanoseconds)'    {_ZN6rclcpp9executors12cbg_executor6Worker9block_forENSt6chrono8durationIlSt5ratioILl1ELl1000000000EEEE}
  [D] 'method void rclcpp::executors::cbg_executor::Worker::unblock()'    {_ZN6rclcpp9executors12cbg_executor6Worker7unblockEv}
  [D] 'method void rclcpp::executors::cbg_executor::Worker::unblock_thread_safe()'    {_ZN6rclcpp9executors12cbg_executor6Worker19unblock_thread_safeEv}
  [D] 'method rclcpp::executors::cbg_executor::Worker* rclcpp::executors::cbg_executor::WorkerQueue::pop_blocked_worker_thread()'    {_ZN6rclcpp9executors12cbg_executor11WorkerQueue25pop_blocked_worker_threadEv}
  [D] 'method void rclcpp::executors::cbg_executor::WorkerQueue::remove_worker_thread(rclcpp::executors::cbg_executor::Worker*)'    {_ZN6rclcpp9executors12cbg_executor11WorkerQueue20remove_worker_threadEPNS1_6WorkerE}

2 Added functions:

  [A] 'method void rclcpp::executors::cbg_executor::CBGScheduler::block_worker_thread()'    {_ZN6rclcpp9executors12cbg_executor12CBGScheduler19block_worker_threadEv}
  [A] 'method void rclcpp::executors::cbg_executor::CBGScheduler::block_worker_thread_for(std::chrono::nanoseconds)'    {_ZN6rclcpp9executors12cbg_executor12CBGScheduler23block_worker_thread_forENSt6chrono8durationIlSt5ratioILl1ELl1000000000EEEE}

2 functions with some indirect sub-type change:

  [C] 'method rclcpp::executors::cbg_executor::CBGScheduler::CBGScheduler(std::function<void()>)' at scheduler.hpp:326:1 has some indirect sub-type changes:
    implicit parameter 0 of type 'rclcpp::executors::cbg_executor::CBGScheduler*' has sub-type changes:
      in pointed to type 'class rclcpp::executors::cbg_executor::CBGScheduler' at scheduler.hpp:32:1:
        type size changed from 2624 to 1984 (in bits)
        1 member function insertion:
          'method virtual rclcpp::executors::cbg_executor::CBGScheduler::~CBGScheduler()' at scheduler.hpp:204:1
        no member function changes (6 filtered);
        2 data member insertions:
          'bool release_workers', at offset 1344 (in bits) at scheduler.hpp:398:1
          'bool release_worker_once', at offset 1352 (in bits) at scheduler.hpp:399:1
        3 data member changes (1 filtered):
          name of 'rclcpp::executors::cbg_executor::CBGScheduler::worker_queue' changed to 'rclcpp::executors::cbg_executor::CBGScheduler::work_ready_conditional' at scheduler.hpp:402:1, size changed from 1024 to 384 (in bits) (by -640 bits)
          'bool worker_checking_for_work' offset changed from 1344 to 1360 (in bits) (by +16 bits)
          'std::__cxx11::list<std::unique_ptr<rclcpp::executors::cbg_executor::CBGScheduler::CallbackGroupHandle, std::default_delete<rclcpp::executors::cbg_executor::CBGScheduler::CallbackGroupHandle> >, std::allocator<std::unique_ptr<rclcpp::executors::cbg_executor::CBGScheduler::CallbackGroupHandle, std::default_delete<rclcpp::executors::cbg_executor::CBGScheduler::CallbackGroupHandle> > > > callback_groups' offset changed from 2432 to 1792 (in bits) (by -640 bits)

  [C] 'method virtual std::unique_ptr<rclcpp::executors::cbg_executor::CBGScheduler::CallbackGroupHandle, std::default_delete<rclcpp::executors::cbg_executor::CBGScheduler::CallbackGroupHandle> > rclcpp::executors::cbg_executor::FirstInFirstOutScheduler::get_handle_for_callback_group(const rclcpp::CallbackGroup::SharedPtr&)' at first_in_first_out_scheduler.cpp:149:1 has some indirect sub-type changes:
    implicit parameter 0 of type 'rclcpp::executors::cbg_executor::FirstInFirstOutScheduler*' has sub-type changes:
      in pointed to type 'class rclcpp::executors::cbg_executor::FirstInFirstOutScheduler' at first_in_first_out_scheduler.hpp:66:1:
        type size changed from 2816 to 2176 (in bits)
        1 base class change:
          'class rclcpp::executors::cbg_executor::CBGScheduler' at scheduler.hpp:163:1 changed:
            details were reported earlier
        no member function changes (3 filtered);
        1 data member change:
          'std::vector<std::unique_ptr<rclcpp::executors::cbg_executor::FirstInFirstOutCallbackGroupHandle, std::default_delete<rclcpp::executors::cbg_executor::FirstInFirstOutCallbackGroupHandle> >, std::allocator<std::unique_ptr<rclcpp::executors::cbg_executor::FirstInFirstOutCallbackGroupHandle, std::default_delete<rclcpp::executors::cbg_executor::FirstInFirstOutCallbackGroupHandle> > > > callback_group_handles' offset changed from 2624 to 1984 (in bits) (by -640 bits)

4 Removed function symbols not referenced by debug info:

  [D] _ZN6rclcpp9executors12cbg_executor11WorkerQueueC1Ev
  [D] _ZN6rclcpp9executors12cbg_executor11WorkerQueueC2Ev, aliases _ZN6rclcpp9executors12cbg_executor11WorkerQueueC1Ev
  [D] _ZN6rclcpp9executors12cbg_executor6WorkerC1Ev, aliases _ZN6rclcpp9executors12cbg_executor6WorkerC2Ev
  [D] _ZN6rclcpp9executors12cbg_executor6WorkerC2Ev

2 Added function symbols not referenced by debug info:

  [A] _ZZN6rclcpp9executors12cbg_executor12CBGScheduler19block_worker_threadEvENKUlvE_clEv
  [A] _ZZN6rclcpp9executors12cbg_executor12CBGScheduler23block_worker_thread_forENSt6chrono8durationIlSt5ratioILl1ELl1000000000EEEEENKUlvE_clEv

✅ librclcpp_action.so — full abidiff report

Compared:

• Base: lib-base/librclcpp_action.so
• Head: lib-pr/librclcpp_action.so @ 56b11bd

(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 @ 56b11bd

(empty report — no differences printed by abidiff)

_{Updated for commit 56b11bd · 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 :)