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.

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

Fix #868: calling service capability locks PlanningSceneMonitor #1033

Merged
merged 6 commits into from Sep 10, 2018

Conversation

Projects
None yet
2 participants
@rhaschke
Copy link
Collaborator

commented Aug 23, 2018

As described in #868 (comment), ROS service capabilities will essentially block the global event queue. Thus, key components as the PlanningSceneMonitor (and the associated CurrentStateMonitor) should use their own event queue and spinner to become independent of the global queue.

@rhaschke rhaschke force-pushed the ubi-agni:fix-868 branch from 14200bf to ea9c0dc Aug 23, 2018

@rhaschke rhaschke requested a review from v4hn Aug 23, 2018

@v4hn

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

I actually forgot about this issue.. Looks good!

However, there have been complains by users in the past when MoveIt classes start their own threads unconditionally.
I believe we should provide another constructor here to use a user-provided CallbackQueue instead.

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 23, 2018

Passing a user-provided CallbackQueue, will not stop creating a thread by the internal spinner.
I think there is no chance to avoid conflicts without another thread (as the main spinner thread blocks on the global callback queue).
Regarding the complains: All action-server-based capabilities will spawn an own thread as well...

@v4hn

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 23, 2018

Would you prefer to pass a CallbackQueue or a NodeHandle (as in other locations of the API)?
NodeHandle might also change the namespace, which is undesirable...

@v4hn

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

NodeHandle is definitely the more common interface for something like this, but then we have root_nh_ as well as nh_.
We don't want to pass two handles (I guess), the alternatives being
(1) taking a root node-handle for the constructor, creating a private handle internally and copy the queue from the root nh we received
(2) pass the queue directly and set it for both nodes
(3) similar to MoveGroupInterface we could have an Options struct that can contain both node-handles

I would say (2) is the simplest option.

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 23, 2018

There is no problem in using (1). I think, it's not even necessary to copy the the event queue explicitly if we initialize the private handle from the root: nh_(root_nh, "~").
However, passing a root handle with a custom/wrong namespace set, will probably break everything. Hence, option (2) might be safer.

@v4hn

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

initialize the private handle from the root: nh_(root_nh, "~")

As far as I remember this is no valid constructor call in ROS.

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 29, 2018

@v4hn I added a new constructor to accept an external NodeHandle. Can you please review?

@@ -171,6 +177,7 @@ planning_scene_monitor::PlanningSceneMonitor::~PlanningSceneMonitor()
parent_scene_.reset();
robot_model_.reset();
rm_loader_.reset();
spinner_.stop();

This comment has been minimized.

Copy link
@v4hn

v4hn Aug 29, 2018

Member

Is it well defined what happens if this was never started?

This comment has been minimized.

Copy link
@rhaschke

rhaschke Sep 7, 2018

Author Collaborator
planning_scene_monitor::PlanningSceneMonitor::PlanningSceneMonitor(
const planning_scene::PlanningScenePtr& scene, const robot_model_loader::RobotModelLoaderPtr& rm_loader,
const ros::NodeHandle& nh, const std::shared_ptr<tf2_ros::Buffer>& tf_buffer, const std::string& name)
: monitor_name_(name), nh_(nh, "~"), root_nh_(nh), spinner_(1, &queue_), tf_buffer_(tf_buffer), rm_loader_(rm_loader)

This comment has been minimized.

Copy link
@v4hn

v4hn Aug 29, 2018

Member

Tested on kinetic:

$ cat src/mytest/src/mytest.cpp
#include <ros/ros.h>

int main(int argc, char **argv) {
  ros::init(argc, argv, "mytest");

  ros::NodeHandle nh;
  ros::NodeHandle pnh(nh, "~");
  return 0;
}
$ rosrun mytest mytest
terminate called after throwing an instance of 'ros::InvalidNameException'
  what():  Using ~ names with NodeHandle methods is not allowed.  If you want to use private names with the NodeHandle interface, construct a NodeHandle using a private name as its namespace.  e.g. ros::NodeHandle nh("~");  nh.getParam("my_private_name"); (name = [~])
Aborted (core dumped)

I guess you did not add a test for this?
Maybe they changed this in melodic, but I doubt it.

This comment has been minimized.

Copy link
@rhaschke

rhaschke Sep 7, 2018

Author Collaborator

Fixed.

* @param rml A pointer to a kinematic model loader
* @param nh external parent NodeHandle
* Ensure that this NodeHandle's CallbackQueue is different from the global queue and that
* it is served by an external spinner!

This comment has been minimized.

Copy link
@v4hn

v4hn Aug 29, 2018

Member

This mechanism can also be used to explicitly run in the global queue, if the user wants it that way (we did it for a long time and it's not necessarily a problem).
Can you change this to just state that the Monitors will be updated with the queue of this node handle?

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 29, 2018

@v4hn Done.

@rhaschke rhaschke force-pushed the ubi-agni:fix-868 branch from 0e1b670 to 870f518 Aug 29, 2018

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 7, 2018

@v4hn Is there anything left that keeps you from approving and merging this?

@v4hn
Copy link
Member

left a comment

Not much except for time sigh.

Still found another (1-line-fix) problem.

Also, please add a note to MIGRATION.md about the change in semantics and how to invoke the monitor to get the old behavior (provide the default node handle).

@@ -163,6 +173,7 @@ planning_scene_monitor::PlanningSceneMonitor::~PlanningSceneMonitor()
stopStateMonitor();
stopWorldGeometryMonitor();
stopSceneMonitor();
spinner_->stop();

This comment has been minimized.

Copy link
@v4hn

v4hn Sep 8, 2018

Member

This will segfault when a custom NodeHandle was provided for construction.

@rhaschke rhaschke force-pushed the ubi-agni:fix-868 branch from 2b3f879 to 844f298 Sep 10, 2018

@rhaschke rhaschke merged commit 9f532aa into ros-planning:melodic-devel Sep 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rhaschke rhaschke deleted the ubi-agni:fix-868 branch Sep 10, 2018

davetcoleman added a commit that referenced this pull request Sep 11, 2018

Cherry-Pick #1033: Fix #868 (#1057)
* Revert "disable waitForCurrentRobotState() for PlanService capability (#923)"

This reverts commit cf2217c.

* only waitForCurrentRobotState when start_state.is_diff

as discussed in #868 (comment)

* reduce code duplication

* use own EventQueue in PlanningSceneMonitor

... to become independent of any capabilities that might block the global queue

* PlanningSceneMonitor: accept parent NodeHandle

* add MIGRATION note

ipa-hsd added a commit to ipa-hsd/moveit that referenced this pull request Mar 18, 2019

Cherry-Pick ros-planning#1033: Fix ros-planning#868 (ros-planning#1057)
* Revert "disable waitForCurrentRobotState() for PlanService capability (ros-planning#923)"

This reverts commit cf2217c.

* only waitForCurrentRobotState when start_state.is_diff

as discussed in ros-planning#868 (comment)

* reduce code duplication

* use own EventQueue in PlanningSceneMonitor

... to become independent of any capabilities that might block the global queue

* PlanningSceneMonitor: accept parent NodeHandle

* add MIGRATION note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.