Add optional timeout to client.call()

[MuhongGuo]

Adding a small feature to support timeout setting for rclpy.Client.call(). Hope it would be also useful for other users :)

[fujitatomoya]

although this makes sense to me, i would reconsider this synchronous method is really needed. probably we can just delete and supports only async method? so that application can avoid possible deadlock and can have all management in the application? (rclcpp does not have sync method for service client though.)

[clalancette]

although this makes sense to me, i would reconsider this synchronous method is really needed. probably we can just delete and supports only async method? so that application can avoid possible deadlock and can have all management in the application? (rclcpp does not have sync method for service client though.)

Yeah, preferably we would deprecate and remove call from rclpy; it is dangerous to use unless you are absolutely, positively sure that it won't be called from within a single threaded executor.

[MuhongGuo]

although this makes sense to me, i would reconsider this synchronous method is really needed. probably we can just delete and supports only async method? so that application can avoid possible deadlock and can have all management in the application? (rclcpp does not have sync method for service client though.)

Yeah, preferably we would deprecate and remove call from rclpy; it is dangerous to use unless you are absolutely, positively sure that it won't be called from within a single threaded executor.

@fujitatomoya @clalancette I believe most of the users are actually calling the services in a sync manner, blocking until the future is done. They will love the client.call() function due to less coding lines if we can extend it's use scope. Why don't we consider to resolve the deadlock issue instead?

[fujitatomoya]

I believe most of the users are actually calling the services in a sync manner

i do not have any data that shows how many user application use sync manner...it just depends on use case and application.
just saying that async can support sync in user application, and the application can be more flexible.

Why don't we consider to resolve the deadlock issue instead?

sounds good to me, although i do not have concrete idea to fix it right away...

anyway, i think having timeout with current sync API makes sense for now.

[clalancette]

@fujitatomoya @clalancette I believe most of the users are actually calling the services in a sync manner, blocking until the future is done.

Unfortunately, in the default case (using a single threaded executor and the default callback group) this causes a deadlock. Even if you use separate callback groups and a multi-threaded executor, this is a bad idea; it is holding up the executor during the callback. That means that any subsequent callbacks in the callback group will be held up until the callback completes. In general, callbacks in ROS 2 should be as short as possible so as to minimize latency for all callbacks.

They will love the client.call() function due to less coding lines if we can extend it's use scope. Why don't we consider to resolve the deadlock issue instead?

The deadlock is called by the fact that subscription callbacks occur on a thread, but service calls need to use that very same executor to get the response. Using the multi-threaded executor doesn't help either, because by default all callbacks are in the default callback group, and it is non-reentrant (which means only one of the members of the callback group can be running at a time). Because of the design if this, we haven't found a way to make this work better (though if you have ideas, we are certainly willing to consider them).

[MuhongGuo]

Closing this PR as #1188 (merged) has added the same feature.

@KKSTB Thank you!

cc @clalancette