Fix race condition in MessageFilter #538
Closed
Conversation
This file contains 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
... revealing that propagating messages via a ROS callback queue is inherently unsafe. If the filter is going to be destroyed, a CBQueueCallback::call() might still be in action, locking the underlying Signal1's mutex_. This will cause as assertion failure during destruction of the mutex: boost::mutex::~mutex(): Assertion `!posix::pthread_mutex_destroy(&m)' failed.
When MessageFilter::signalMessage() is called asynchronouyls via a ROS callback queue, this call might still be in progress while another (holding Signal1::mutex_), while another thread might attempt to remove the MessageFilter. This results in a failed assertion as the MessageFilter destructor attempts to destroy a locked mutex. Here, by introducing another mutex in MessageFilter, we ensure that CBQueueCallback::call() is finished before destroying the MessageFilter.
PR ros#144 attempted to resolve a deadlock by postponing calls to callbacks. However, this has the drawback of using references to TransformableRequests - outside the protection by transformable_requests_mutex_. Thus, these requests might get deleted by another thread, resulting in segfaults: terminate called after throwing an instance of 'boost::wrapexcept<boost::lock_error>' what(): boost: mutex lock failed in pthread_mutex_lock: Invalid argument This reverts commit bfb8038.
As pointed out in ros#144 the deadlock arises due to an inversion of locking order: - MessageFilter::add(), when removing the oldest message, locks: - MessageFilter::messages_mutex_ - BufferCore::transformable_requests_mutex_ (via cancelTransformableRequest()) - BufferCore::testTransformableRequests() locks: - transformable_requests_mutex_ - MessageFilter::message_mutex_ (via MessageFilter::transformable() callback) PR ros#144 attempted to resolve the issue by postponing callback calls. However, this has the drawback of using references to TransformableRequests - outside the protection by transformable_requests_mutex_. These requests might got deleted by another thread meanwhile! Here the deadlock is resolved in MessageFilter::add, cancelling the requests outside the messages_mutex_ protection.
|
Looks like there is still a deadlock 😞 |
Resolved as well. |
While MessageFilter::clear() removes pending messages from the callback queue, there might be still callbacks active when attempting to remove the MessageFilter. Depending on what goes first, either the already destroyed mutex is tried to lock or a locked mutex is tried to destroy, both resulting in an assertion failure. This race condition cannot completely avoided. Using a shared mutex for all callers and moving the unique lock to the end of the destructor hopefully reduces the probability of such a race condition.
|
I'm afraid the remaining race condition cannot be fixed. Rather, the ROS callback queue mechanism should be avoided. While |
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.
A bug reported in ros-visualization/rviz#1753 pointed to race condition(s) in
tf2_ros::MessageFilter.I implemented a stress test to analyze the problem in detail. There were two issues:
As pointed out in Solve a bug that causes a deadlock in MessageFilter #144 the deadlock arises due to an inversion of locking order:
MessageFilter::add(), when removing the oldest message, locks:MessageFilter::messages_mutex_BufferCore::transformable_requests_mutex_(viacancelTransformableRequest())BufferCore::testTransformableRequests()locks:transformable_requests_mutex_MessageFilter::message_mutex_(viaMessageFilter::transformable()callback)CBQueueCallbackWhen
MessageFilter::signalMessage()is called asynchronously via a ROS callback queue, this call might still be in progress (holdingSignal1::mutex_), while another thread might attempt to remove the MessageFilter.This results in a failed assertion as the MessageFilter's destructor attempts to destroy a locked mutex.
PR #144 attempted to resolve the first issue by postponing callback calls. However, this has the drawback of using references to
TransformableRequestsoutside the protection bytransformable_requests_mutex_. Thus, these requests might get deleted by another thread, resulting in a segfault as revealed by the stress test.Here the deadlock is resolved in
MessageFilter::add, canceling the requests outside themessages_mutex_protection.To resolve the second race condition, another mutex is introduced in MessageFilter, to ensure that
CBQueueCallback::call()is finished before destroying the MessageFilter.These two changes make the stress test pass.
@tfoote, please note that this is a PR against Melodic. I will prepare a port for Noetic as well.