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

Thread safety #101

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Thread safety #101

wants to merge 4 commits into from

Conversation

xqms
Copy link

@xqms xqms commented Mar 22, 2017

This PR addresses some thread safety and race conditions present in the trajectory action server.

The problems/fixes include:

  • has_goal_ is accessed from doTraj() and the action server callbacks, so protect it by a mutex.
  • goalCB() and cancelCB() also have to be protected against affecting each other, since an async spinner with 3 threads is used - the action server itself does not provide any locking.
  • The UrRealtimeCommunication class is not thread-safe. If you call addCommandToQueue() from a different thread (e.g. the trajectory thread in UrDriver::uploadProg()), you can get data corruption.

There are other subtle changes (joining the trajectory thread properly, ...).

@ThomasTimm, we talked briefly at MBZIRC about your driver. If you want, I can spend some more time on separating the changes so that they are easier for you to check. Does that make sense or will you release your new version soon?

@smurray47
Copy link

I'm surprised this PR has been sitting for so long! It seems to fix a quite dangerous bug! It seems to have an occasional deadlock problem I tried to investigate, but its going to be a non-obvious one. My guess is that one of the threads must hang after acquiring goal_mutex or something.

@darkdragon-001
Copy link

Any progress on this?

@gavanderhoorn
Copy link
Member

Not that I'm aware of. This PR is also targeting master, which is deprecated. The refactor in kinetic-devel uses a different architecture so it would need to be seen whether it could also benefit from the changes in this PR (and whether it even needs them).

@jaymwong
Copy link

This PR has been merged into this fork of ur_modern_driver but I don't think it's made it back upstream to ros-industrial.

@gavanderhoorn
Copy link
Member

@jaymwong wrote:

This PR has been merged into this fork of ur_modern_driver but I don't think it's made it back upstream to ros-industrial.

that fork is also based on the deprecated master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants