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

ControllerManager: wait for done-callback #1783

Merged
merged 2 commits into from
Nov 27, 2019

Conversation

rhaschke
Copy link
Contributor

This is an alternative for #1741. Thanks to the very detailed analysis of @jschleicher, the race condition reported in #1493 was tracked down to an issue in actionlib: ros/actionlib#155.
"As the done callback is really short" (#1741 (comment)), the busy-waiting solution offered in #1741 is temporarily acceptable. Due to the same fact, I suggest to drop the remaining boilerplate (considering the timeout) of #1741 as well and keep the quick fix as simple as possible.

@v4hn
Copy link
Contributor

v4hn commented Nov 26, 2019

I don't agree.
It's not a good idea to introduce one more potential issue while working around one.

@v4hn v4hn closed this Nov 26, 2019
@rhaschke
Copy link
Contributor Author

Where do you see the potential issue?

@rhaschke rhaschke reopened this Nov 26, 2019
@v4hn
Copy link
Contributor

v4hn commented Nov 26, 2019

The function clearly gets passed a timeout parameter.
If you introduce a (syntactic) endless-loop, that can violate the call specification.
I agree that this will usually not be an issue, but if it becomes one, I really don't want to debug it...

@rhaschke
Copy link
Contributor Author

In general, you are right and I would agree, but here the function we are waiting for is a no-brainer, setting a few variables only. I don't think this will change in the very next future. And as far as I have seen, users cannot pass an arbitrary function there. In this case we wouldn't like to busy-wait anyway.
So, from my point of view, we need to balance code maintainability (being able to remove this quick fix in the near future) vs. some potential change of the done callback, which then might take ages to finish. If you want to avoid the (potentially) infinite loop, I'm fine to limit the time to, say 0.1s, given that the corresponding code snippet fits into the #ifdef block.

@v4hn
Copy link
Contributor

v4hn commented Nov 27, 2019 via email

@rhaschke
Copy link
Contributor Author

@v4hn: Done

@rhaschke rhaschke merged commit 9b7ab3d into moveit:master Nov 27, 2019
@rhaschke rhaschke deleted the fix-controller-donecb branch November 27, 2019 12:25
rhaschke added a commit to ubi-agni/moveit that referenced this pull request Nov 27, 2019
@rhaschke
Copy link
Contributor Author

I will cherry-pick this to melodic as well. Travis is currently validating it.

@captain-yoshi
Copy link
Contributor

For your information. The race condition still occurs when testing with URSim in docker. I didin't test with other limit time.

The pull request from ros/actionlib #156 fixes the problem.

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.

None yet

4 participants