ActionClient deadlocks if accessing goalHandle in feedback_callback

[jmachowinski]

Bug report

• Operating System:
  • Ubuntu 22.04
• Installation type:
  • binaries
• Version or commit hash:
  • iron
• DDS implementation:
  • Fast-RTPS
• Client library (if applicable):
  • rclcpp-action

Steps to reproduce issue

Create a ActionClient with a custom feedback_callback

wait for the fallback to be called

call get_status() on the passes GoalHandle

Expected behavior

It should not deadlock

Actual behavior

It deadlocks

Additional information

The function call_feedback_callback (

rclcpp/rclcpp_action/include/rclcpp_action/client_goal_handle_impl.hpp

Line 163 in c0d72c3

ClientGoalHandle<ActionT>::call_feedback_callback(

) holds the mutex of the handle while calling the
feedback_callback. Therefore using any method on the handle, that tries to also acquire
the mutex will deadlock the system.

[CursedRock17]

What GoalHandle are you calling with the feedback_callback, I set up a minimal ActionClient and can't seem to find how you're getting a deadlock, for instance I have:

void feedback_callback(
GoalHandle::SharedPtr, const std::shared_ptr<const ParamType::Feedback> feedback){
// Do stuff to feedback
}

Then I just have a send_goal function on a timer in a ClientConstructor, where I bind the feedback_callback. There's a mutex on every function in the file you gave in additional information. Can you explain, with code, how you're using call_feedback_callback?

[jmachowinski]

https://github.com/jmachowinski/rclcppActionDeadlock

This code demonstrates the issue.

There are two ways to fix this problem, either release the handle_mutex_ before calling the callback (here

rclcpp/rclcpp_action/include/rclcpp_action/client_goal_handle_impl.hpp

Line 177 in 22a954e

feedback_callback_(shared_this, feedback_message);

),
or make the handle_mutex a std::recursive_mutex.

[tgroechel]

Would is_feedback_aware and is_result_aware calls also cause this? Took a quick look and I would guess they do (header APIs, impl)

[CursedRock17]

I tried using the code @jmachowinski provided and none of the those three public methods, which use a std::lock_guard worked correctly. It seems that due to some sort of scoping with the executors to the client, that the data cannot even be fetched by it's own class. So anything with lock_guard may deadlock. This was still an issue even when moving from MultiThreadedExecutor to SingleThreadedExecutor which I find really odd. I don't entirely understand why the class itself can't access a function that it has ownership, especially if only one thread is trying to gain access to the data.

Since things like the return functions don't even work and we're using lock_guard to have RAII and not need to release early, it may be better to go with either std::recursive_mutex like said earlier, or now that we have access to C++17 use std::scoped_lock. Also the tests that were written a bit ago probably couldn't catch the deadlock, since they never got a false from a try_unlock or something like that.

[jmachowinski]

So, what's the way forward here, should I provide a merge request that changes to recursive mutex ?

[clalancette]

So, what's the way forward here, should I provide a merge request that changes to recursive mutex ?

That is probably the way forward. Dropping and then reacquiring the lock can lead to its own set of problems/race conditions, so I'd rather not do that.