Prevent stale MessageFilter futures after callback race - #986
Open
Parth-Oza wants to merge 1 commit into
Open
Conversation
Signed-off-by: Parth Oza <ozaparthu055@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
MessageFilter::add()currently stores the future returned bywaitForTransform()after that call returns. A transform callback running on another thread can complete in the gap between the return and insertion, fail to find its future, and leave the subsequently inserted future stale untilclear()or destruction.This change registers a pending entry before calling
waitForTransform(). The callback can erase that entry whether it runs before or after the returned future is installed. Exceptions remove the pending entry, andclear()only cancels entries that contain a realized future.A deterministic regression test uses a test buffer that invokes the callback from another thread before
waitForTransform()returns, then verifies thatclear()does not try to cancel the already-completed request.Fixes #827
Is this user-facing behavior change?
No. This fixes internal future bookkeeping and prevents completed transform requests from remaining registered.
Did you use Generative AI?
Yes. OpenAI Codex assisted with implementation and test construction; the change was reviewed and validated in a clean ROS 2 Rolling container.
Additional Information
Validated on
ros:rolling-ros-base(arm64):colcon build --packages-up-to tf2_ros --cmake-args -DBUILD_TESTING=ON -DCMAKE_CXX_FLAGS=-Werrorcolcon test --packages-select tf2_ros