-
Notifications
You must be signed in to change notification settings - Fork 274
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
High check_frequency values for BufferClient break tf2 #178
Comments
I can confirm that I saw this error once but cannot reproduce it again.
On repeated calls I get the expected
Cranking the rate up to 100000 I was also able to get a double answer:
@g-gemignani Does it fail every time for you what sort of hardware/network setup are you using? I'm currently testing on loopback. Note for others reproducing you also need to run a buffer_server. I suspect it's in this block jumping out early: https://github.com/ros/geometry2/blob/indigo-devel/tf2_ros/src/tf2_ros/buffer_client.py#L177-L180 since that's the only block that the rate interacts with assuming it's the rate. |
@tfoote We always experience this problem on our SCITOS base. We are currently running everything on the base (Intel(R) Core(TM) i7-4770S CPU @ 3.10GHz, 8GB ram). All the traffic is local. I will spend a little bit of time trying to come up with a server that reproduces the error. I'll keep you updated. Thanks! :) |
@tfoote As promised, here is the package. |
@g-gemignani Thanks for the simple reproducable example. After a bunch of digging I believe that what's happening is that there's a I did some debugging into this and I definitely can see the issue. However I haven't yet traced down the source. My current speculation is that this is actually a race condition inside the simple action client usage. In particular whether the get_result() and get_state() calls. This range The only thing between them before was the sleep for the rate. Adding a sleep right before the get_result() call makes almost all the errors go away. Both the simple_action_client errors as well as the @mikaelarguedas FYI |
We are also experiencing this issue. Is it possible to work around this race condition in the buffer client by adding a check inside the __is_done() function? Require a result if the state is SUCCEEDED?
|
@jorgenfb Could you propose the change in a PR for review? |
PR is ready. Any chance this can be backported to kinetic? |
This doesn't break ABI or API so it should be good to consider for backport. I'll plan to include it next time I do a backport sweep. I'm going to close this. @g-gemignani if you could verify the patch resolves your issue that would be great. If not please comment back here and we can reopen this. |
Hello @tfoote, hello @jorgenfb. I apologize for the late reply. I went on, checked out the pr and reran my test with and without the patch. Without the patch the output given by the node is:
with the patch the output is:
From this short test I believe that the problem was partially mitigated. In fact, no more tf errors are thrown, however there is still something wrong with the tf state machine. Thank you for the patch @jorgenfb ! |
@g-gemignani thanks for the updated testing. I'll reopen this to track the remaining issue. |
I did some further digging. The issue is with the action client, not with BufferClient (state is available before result). I created a simplified case without using bufferclient, which I will post along with an issue in actionlib when I have cleaned it up. However during my testing I found two solutions that can be used for BufferClient. SimpleActionClient does have a The other solution is to check the SimpleGoalState instead of the state:
To this one:
and remove __is_done() complete. Do we want to update to the last solution, I can prepare a new PR? And if so, do we close this issue and continue once I have posted an issue in actionlib? |
It would be great to move to the actionlib API but you're right that it doesn't support the adjustable timeout. Making that a parameter in the actionlib API would be a great enhancement ticket/PR. In the mean time we should copy the logic internal to that method. https://github.com/ros/actionlib/blob/d317e63ebabc6c437f8b9f73ece06acf348700ea/src/actionlib/simple_action_client.py#L124 Actually on looking closer, internal to the action client the wait is a condition wait, which gets notified when there's new content. Which means that it shouldn't actually need a parameter to do the polling at any specific frequency and it will be better. The looping logic is just implemented to make sure that we don't jump out to early on an invalid wakeup and that the condition wait will also respond to shutdown requests. So I might suggest that we just switch to use wait_for_result and deprecate the now unused argument. The loop rate could still be parameterized since the 0.1 seconds is a magic number, but that's lower priority for me as it's just the shutdown response time and won't effect runtime result responsiveness. |
Using I would be happy to provide the actual code changes if you like, but then would need assistance on handling the deprecation. |
I would suggest making two changes. The first will simply switch over to using the |
…ate check_frequency Use the API provided by actionlib for waiting for result. This will improve the response time and prevent problems with custom solutions (see ros#178). This change makes constructor parameter check_frequency obsolute and deprecates it.
I have created a pull request for the suggested changes and opened an issue for the original cause of this issue in actionlib repo, including a reproducable test case. |
Use the API provided by actionlib for waiting for result. This will improve the response time and prevent problems with custom solutions (see ros#178). This change makes constructor parameter check_frequency obsolute and deprecates it.
Use the API provided by actionlib for waiting for result. This will improve the response time and prevent problems with custom solutions (see #178). This change makes constructor parameter check_frequency obsolute and deprecates it.
Use the API provided by actionlib for waiting for result. This will improve the response time and prevent problems with custom solutions (see #178). This change makes constructor parameter check_frequency obsolute and deprecates it.
Use the API provided by actionlib for waiting for result. This will improve the response time and prevent problems with custom solutions (see #178). This change makes constructor parameter check_frequency obsolute and deprecates it.
Error:
How to reproduce it:
The text was updated successfully, but these errors were encountered: