Fix ABBA deadlock between waitForTransform and testTransformableRequests

[alireza-moayyedi]

Description

As mentioned in #979, an ABBA deadlock situation was introduced in #966 where testTransformableRequests() locks transformable_requests_mutex_ and tries to run cb which requires timer_to_request_map_mutex_ while parallely waitForTransform acquires timer_to_request_map_mutex_ and tries to call addTransformableRequest which internally needs transformable_requests_mutex_ resulting in ABBA situation.

My apologies for not noticing this issue previously and a special thanks to @jplapp and @mini-1235 for bringing it to my attention.

@mini-1235 has already opened #979 to fix this issue. I agree with the approach in that PR (to unlock mutex before running callbacks and lock again) but I think restarting the i and going through the beginning of the requests for every callback would add unnecessary computation time. More precisely, the cost is that if M callbacks fire per setTransform and N requests are pending, you get O(M·N) work instead of O(N).

I tried to improve that by going through the requests first, extracting the pending callbacks and then running them. That way we keep the swap-and-pop behavior as before.

I have also copied the test @jplapp added in #980 here as well. @mini-1235 could you please also check this in your nav2 setup where this problem became highlighted?

Credits to @jplap and @mini-1235 for their idea and suggested changes.

Did you use Generative AI?

Yes, Claude Sonned 4.6

[fallenmi]

The production change looks sound; I am requesting changes only for the regression test's determinism. I reviewed exact head d3a1a87d45c6d7234a1986006230ea470b44d014 and current merge a09efbf2d862d6b51cb6894f195d41cb2530a313; the two touched blobs are identical between them. GitHub currently exposes one successful Mergify check but no build/test workflow for this head, so I did not treat the new test as executed upstream.

AI disclosure: I used OpenAI Codex and Claude Max to trace the public lock ordering, compare the exact refs and related PR interactions, and independently red-team the finding. I verified the cited source, SHAs, CI layers, and conclusion before submission.

[fallenmi]

The 50 ms delay does not force the intended interleaving. in_transformable_callback only releases the waiter; it never confirms that the waiter has acquired timer_to_request_map_mutex_ and is blocked in addTransformableRequest(). If that thread is not scheduled until after this sleep, the pre-fix setter can process the second ready callback while the timer mutex is still free, finish, and release transformable_requests_mutex_; the waiter then acquires both locks uncontended, so the old deadlocking implementation passes this regression. Please replace the timing assumption with a handshake or narrowly scoped test hook that proves the waiter reached the lock-order point before gate_cb returns.

[alireza-moayyedi]

The production change looks sound; I am requesting changes only for the regression test's determinism. I reviewed exact head d3a1a87d45c6d7234a1986006230ea470b44d014 and current merge a09efbf2d862d6b51cb6894f195d41cb2530a313; the two touched blobs are identical between them. GitHub currently exposes one successful Mergify check but no build/test workflow for this head, so I did not treat the new test as executed upstream.

AI disclosure: I used OpenAI Codex and Claude Max to trace the public lock ordering, compare the exact refs and related PR interactions, and independently red-team the finding. I verified the cited source, SHAs, CI layers, and conclusion before submission.

Reported as spam.

[mini-1235]

I initially had a similar implementation 😄 . My concern was that callbacks were previously invoked while scanning, whereas this invokes them only after the full scan. That behavioral change led me to the more conservative approach in #979. Curious what do you think about this?

However, I agree that #979 has some issues I did not fully consider. I do not know of any use case that depends on the old behavior, and I previously verified that this approach fixes my issue, so I am happy to approve it.

[alireza-moayyedi]

@mini-1235

I initially had a similar implementation 😄 . My concern was that callbacks were previously invoked while scanning, whereas this invokes them only after the full scan. That behavioral change led me to the more conservative approach in #979. Curious what do you think about this?

True but I think with the current mutexes and design it would be inevitable to bump into this kind of deadlock if we want to keep the fix that was originally intended in #966.

I guess an alternative which might be cleaner is to change the mutex implementation altogether such that instead of a shared map protected by a mutex, we give each pending request a std::shared_ptr<std::atomic<bool>> captured in both cb and timerCallback. Something like this:
auto resolved = std::make_shared<std::atomic<bool>>(false);
And then whichever fires first sets it to true: resolved->exchange(true). We then have to cancel the timer from cb when it wins.

But well, my preference is always to keep the original design, unless a refactor is really requested. Hence the current proposal. Curious to hear what the maintainers think.

[tfoote]

Storing and iterating on the callbacks at the end makes the most sense here. The diff's a lot smaller than it looks just due to the indention.

[ahcorde]

Pulls: #982
Gist: https://gist.githubusercontent.com/ahcorde/5809a00a28e5e193b20c4d317f4915fc/raw/7967b4631ba457159ed63d1258ccf854beff1ce4/ros2.repos
BUILD args: --packages-above-and-dependencies tf2 tf2_ros
TEST args: --packages-above tf2 tf2_ros
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/20349

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

[ahcorde]

https://github.com/Mergifyio backport lyrical kilted jazzy humble

[mergify]

backport lyrical kilted jazzy humble

✅ Backports have been created

Details

• #987 Fix ABBA deadlock between waitForTransform and testTransformableRequests (backport #982) has been created for branch lyrical
• #988 Fix ABBA deadlock between waitForTransform and testTransformableRequests (backport #982) has been created for branch kilted but encountered conflicts

Cherry-pick of 16cfc11 has failed:

On branch mergify/bp/kilted/pr-982
Your branch is up to date with 'origin/kilted'.

You are currently cherry-picking commit 16cfc11.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   tf2_ros/test/test_buffer.cpp

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   tf2/src/buffer_core.cpp

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

• #989 Fix ABBA deadlock between waitForTransform and testTransformableRequests (backport #982) has been created for branch jazzy but encountered conflicts

Cherry-pick of 16cfc11 has failed:

On branch mergify/bp/jazzy/pr-982
Your branch is up to date with 'origin/jazzy'.

You are currently cherry-picking commit 16cfc11.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   tf2_ros/test/test_buffer.cpp

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   tf2/src/buffer_core.cpp

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

• #990 Fix ABBA deadlock between waitForTransform and testTransformableRequests (backport #982) has been created for branch humble but encountered conflicts

Cherry-pick of 16cfc11 has failed:

On branch mergify/bp/humble/pr-982
Your branch is up to date with 'origin/humble'.

You are currently cherry-picking commit 16cfc11.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   tf2_ros/test/test_buffer.cpp

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   tf2/src/buffer_core.cpp

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally