Fix race condition in SynchronizedStringParameter#1039
Fix race condition in SynchronizedStringParameter#1039corycrean wants to merge 7 commits intomoveit:mainfrom
Conversation
|
|
||
| std::string content_; | ||
| boost::lockfree::spsc_queue<std::string> queue_; | ||
| std::mutex queue_mutex_; |
There was a problem hiding this comment.
Is there a need for a mutex? I thought lock-free mean no mutex needed
There was a problem hiding this comment.
The reason for the lock is that pushing into a spsc queue is not re-entrant safe. Because there are many threads that can be run by a ROS executor and there is the possibility of the callback being called on two threads at once you need to protect pushing into the spsc_queue.
| rclcpp::Publisher<std_msgs::msg::String>::SharedPtr string_publisher_; | ||
|
|
||
| std::string content_; | ||
| boost::lockfree::spsc_queue<std::string> queue_; |
There was a problem hiding this comment.
I would vote for a solution that doesn't require Boost since we're planning to get rid of it as a dependency
There was a problem hiding this comment.
There is no stl replacement for the spsc queue. The plan to remove boost is to remove the parts of boost that have been migrated into the stl. The spsc_queue is not one of those things.
| auto start = node->now(); | ||
|
|
||
| urdf_string_ = | ||
| urdf_ssp_.loadInitialValue(node, ros_name, std::bind(&RDFLoader::urdfUpdateCallback, this, std::placeholders::_1), |
There was a problem hiding this comment.
I'm missing the context a little, but why did you remove the callbacks?
There was a problem hiding this comment.
It looked to me like the callback was responsible for:
- Saving the
urdf_string_ - Calling
loadFromStrings
but those things are already done in the constructor, so I thought the callbacks could be safely removed. I'm noticing now that the callbacks also call new_model_cb_() -- should this be added to the constructor as well?
Also, calling loadFromStrings from urdfUpdateCallback, before srdf_string_ was loaded, produced an error message https://github.com/ros-planning/moveit2/blob/2ee83c31e107ff71f4469fe0d048eb18606c8e93/moveit_ros/planning/rdf_loader/src/rdf_loader.cpp#L106 which was harmless but a little confusing, and removing the callbacks got rid of that error message as well.
moveit_ros/planning/rdf_loader/src/synchronized_string_parameter.cpp
Outdated
Show resolved
Hide resolved
moveit_ros/planning/rdf_loader/src/synchronized_string_parameter.cpp
Outdated
Show resolved
Hide resolved
moveit_ros/planning/rdf_loader/src/synchronized_string_parameter.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Tyler Weaver <squirrel428@protonmail.com>
|
It looks like my implementation here fails when |
Description
This PR addresses the issue in moveit/moveit2_tutorials#262 where the robot model can't be loaded. Currently, the subscription created in
SynchronizedStringParameter::waitForMessageis added to twoWaitSetobjects (one belonging to the executor, and one belonging to the SynchronizedStringParameter), and it's unpredictable whichWaitSetwill end up processing the messages received by that subscription. This PR fixes the problem by removing theWaitSetfromSynchronizedStringParameter.Checklist