Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ensure thread safety in clear octomap #2500

Merged
merged 2 commits into from Mar 3, 2021
Merged

ensure thread safety in clear octomap #2500

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
321b8f6
ensure thread safety in clear octomap
simonschmeisser Jan 26, 2021
4153df0
Add comment about necessary scoping of lock
simonschmeisser Mar 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 17 additions & 10 deletions moveit_ros/planning/planning_scene_monitor/src/planning_scene_monitor.cpp
View file
Open in desktop
Expand Up @@ -531,18 +531,25 @@ void PlanningSceneMonitor::newPlanningSceneCallback(const moveit_msgs::PlanningS

void PlanningSceneMonitor::clearOctomap()
{
if (scene_->getWorldNonConst()->removeObject(scene_->OCTOMAP_NS))
triggerSceneUpdateEvent(UPDATE_SCENE);
if (octomap_monitor_)
{
octomap_monitor_->getOcTreePtr()->lockWrite();
octomap_monitor_->getOcTreePtr()->clear();
octomap_monitor_->getOcTreePtr()->unlockWrite();
}
else
bool removed = false;
{
ROS_WARN_NAMED(LOGNAME, "Unable to clear octomap since no octomap monitor has been initialized");
boost::unique_lock<boost::shared_mutex> ulock(scene_update_mutex_);
Copy link
Contributor

Choose a reason for hiding this comment

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

It wasn't hard to figure out how this works, but I had to think for a second because I don't use mutexes a lot. Do you want to add a comment to the effect of // Lock scene using mutex before removing the octomap. Lock expires after the code block for the less familiar? I'm on the fence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should probably add a comment about the extra brackets that are there so that the scoped lock gets lifted before notifying consumers (triggerSceneUpdateEvent) in order to avoid a deadlock ...

Copy link
Contributor

@felixvd felixvd Mar 3, 2021 •
edited

Choose a reason for hiding this comment

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

I didn't know that the lock needs to be lifted before that call. How about adding // Lift the scoped lock before calling triggerSceneUpdateEvent to avoid deadlock at the end of the block?

I'm fine with not adding the other comment because I suppose it's not "nonobvious to a reader who understands C++ well".

removed = scene_->getWorldNonConst()->removeObject(scene_->OCTOMAP_NS);

if (octomap_monitor_)
{
octomap_monitor_->getOcTreePtr()->lockWrite();
octomap_monitor_->getOcTreePtr()->clear();
octomap_monitor_->getOcTreePtr()->unlockWrite();
}
else
{
ROS_WARN_NAMED(LOGNAME, "Unable to clear octomap since no octomap monitor has been initialized");
} // Lift the scoped lock before calling triggerSceneUpdateEvent to avoid deadlock
}

if (removed)
triggerSceneUpdateEvent(UPDATE_GEOMETRY);
}

bool PlanningSceneMonitor::newPlanningSceneMessage(const moveit_msgs::PlanningScene& scene)
Expand Down