Skip to content

Fix deadlock in buffer core - #979

Closed
mini-1235 wants to merge 1 commit into
ros2:rollingfrom
mini-1235:fix/transformable-callback-reentrancy
Closed

Fix deadlock in buffer core#979
mini-1235 wants to merge 1 commit into
ros2:rollingfrom
mini-1235:fix/transformable-callback-reentrancy

Conversation

@mini-1235

@mini-1235 mini-1235 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Description

This came up when I switch the default executor to EventsCBGExecutor in Nav2, tf2, and rclcpp. I have summarized my issue to my agent, and asks it to create some regression tests, one can verify that, without modifying anything in buffer core, the tests should end with a deadlock.

I also asked my agent to summarize the issue and changes:

## Summary

This change prevents `tf2::BufferCore` from invoking transformable callbacks while holding
`transformable_requests_mutex_`.

## Problem

`BufferCore::testTransformableRequests()` previously called application code while holding the
request mutex. A callback that re-entered `BufferCore`, or depended on another thread that needed to
add or cancel a request, could deadlock.

The issue is executor-independent, but EventsCBG exposed it consistently in Nav2 by increasing the
overlap between TF delivery and sensor callbacks. Nav2 remained active while `map -> odom` stopped
advancing and TF readiness events accumulated.

For example, AMCL uses a `MessageFilter<LaserScan>`. The TF listener delivers a ready transform
synchronously through the filter into `AmclNode::laserReceived()`, while another executor thread
may receive the next scan and call:


MessageFilter::add()
  -> Buffer::waitForTransform()
    -> BufferCore::addTransformableRequest()


Holding the request mutex across the first application callback unnecessarily blocks the next
request and can form a lock convoy or dependency cycle.

## Changes

When a request is ready, `testTransformableRequests()` now:

1. Saves the callback arguments locally.
2. Extracts the callback from its map.
3. Removes the completed request.
4. Releases the request mutex.
5. Invokes the callback.
6. Reacquires the mutex and restarts scanning because callback execution may have changed the
   request vector.

Removing the request before invocation establishes a clear cancellation boundary: cancellation
either removes a pending request first, or delivery has already been claimed and the callback runs
once. No container reference or iterator survives the unlocked region.

Resolved frame names are stored when the request is registered so callback arguments remain stable
after the request is removed. `unordered_map::extract()` retains callback ownership without copying
the `std::function`.

I have verified that this pass all regression tests I added, and also works in Nav2 + tf2 + rclcpp when using EventsCBGExecutor by default.

Is this user-facing behavior change?

Yes, previously there is a deadlock, so this is a bugfix

Did you use Generative AI?

Yes, Codex Sol 5.6

Additional Information

@alireza-moayyedi

Copy link
Copy Markdown
Contributor

Thanks for opening the PR and my apologies for introducing the deadlock in #966 . I agree with the idea introduced here i.e., breaking the ABBA pattern by unlocking the mutex when running the cb. But restarting 'i' would mean complexity increases proportional to the number of pending requests. I think that can be avoided by first extracting all the pending callbacks, and then calling them. I have opened #982 which is based on your suggested idea while keeping the swap and pop behavior of the pending requests. Let me know if you agree with it.

Comment thread tf2/src/buffer_core.cpp
callback_node.mapped()(request_handle, target_frame, source_frame, request_time, result);
lock.lock();
// The callback or another thread may have changed the request vector.
i = 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If M callbacks fire per setTransform and N requests are pending, you get O(M·N) work instead of O(N) by going through the requests again each time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I am happy to close this and follow up on your PR instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants