fix trajectory service blocking callback queue#717
fix trajectory service blocking callback queue#717davetcoleman merged 3 commits intoindigo-develfrom
Conversation
As execute service request might block (waiting for trajectory execution to finish), processing of other events/callbacks from the main spinner thread would be blocked.
|
If accepted, this PR should be applied to Jade and Kinetic as well. It's an important bug fix. |
| // We need to serve each service request in a thread independent of the main spinner thread. | ||
| // Otherwise, a synchronous execution request (i.e. waiting for the execution to finish) would block | ||
| // execution of the main spinner thread. | ||
| // Hence, we use an own asynchronous spinner listening to our own callback queue. |
|
+1 after code-reviewing, would appreciate someone (doesn't have to be a maintainer) verifying this does as stated |
+1 |
|
Considered review comments. |
|
With #716 this PR isn't strictly necessary anymore. However,, as it is bad programming practice to block the main spinner thread, I suggest to merge anyway. Implementation provides a nice example how to solve similar situations. |
|
thanks! also - i notice you pushed the branch directly to the ros-planning org - i think its cleaner practice to keep your own branch on your fork, so that we don't have to occasionally cleanout forgotten branches (which I've done several times for MoveIt) and so users can quickly find relevant *-devel branches |
serve ExecuteService from separate spinner thread
serve ExecuteService from separate spinner thread
…eparate spinner thread indigo-devel merge: 5fbc400
As an
ExecuteServicerequest might block (waiting for trajectory execution to finish),processing of other events/callbacks from the main spinner thread would be blocked.
Hence,
ExecuteServiceis now served from a separate spinner thread, using a separate callback queue. This enables trajectory stopping in #713.An alternative, IMHO even better approach is to turn the service into an action, which provides feedback and can be explicitly interrupted. I did that, but it requires several changes to existing moveit_config packages: as the service capability would be renamed (from
MoveGroupExecuteServicetoMoveGroupExecuteActionallmove_group.launchfiles would need an update.However, I could think of a slow transition, adding the new capability in Kinetic and loading both the old and new capability in newly created
move_configpackages. In some later release we could remove the old service capability, hoping that meanwhile everybody has adapted their move configs.What do you think of that idea?