Add timeout option to call_service messages #984
Conversation
| @@ -81,6 +81,7 @@ def call_service(self, message): | |||
| fragment_size = message.get("fragment_size", None) | |||
| compression = message.get("compression", "none") | |||
| args = message.get("args", []) | |||
| timeout = message.get("timeout", 5.0) | |||
There was a problem hiding this comment.
So the previous default was to never time out. Should this continue to be the case?
There was a problem hiding this comment.
If we want to retain current behavior, it should by default never timeout.
There was a problem hiding this comment.
Though I think we could change the defaults for rolling and future distros. @sea-bass what do you think about branching out for Jazzy ?
There was a problem hiding this comment.
I'm OK either way.
It would be the "right" thing to do to branch out for Jazzy, but I think we still have a few more months where it's okay to land breaking changes to Jazzy.
As an unofficial rule, I would say we can keep putting changes into Jazzy until May 2025 where you may see more people upgrading from ROS 1.
| while rclpy.ok() and not future.done(): | ||
| start_time = time.monotonic() | ||
| while ( | ||
| rclpy.ok() and not future.done() and time.monotonic() - start_time < server_response_timeout |
There was a problem hiding this comment.
Is there no option to never time out? Like a -1 value or something?
There was a problem hiding this comment.
We could treat any non-positive value as "no timeout"
Public API Changes
Adds
timeoutoption tocall_servicemessagesDescription
If during calling a service, the Service Server dies, the ServiceCaller will get stuck waiting for response. This also makes the whole protocol stuck if
call_services_in_new_threadsoption is not set.This PR adds a response timeout parameter to ServiceCaller and adds an optional
timeoutoption tocall_servicemessages with a default value of 5 seconds.If the API change gets accepted I will finish this PR by adding tests that cover this feature