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

Minimal service client implementation without using wait_until_future_complete #4993

Open
wants to merge 2 commits into
base: rolling
Choose a base branch
from

Conversation

santiago-tapia
Copy link

This PR relates to Tutorial for writing a service with an asynchronous client node, as it originates from the discussion on synchronous vs. asynchronous service requests in that PR. However, I believe this proposal should follow its own review process.

I am not entirely sure this proposal will be accepted, but I strongly believe the other one should be, particularly due to some of the arguments I outline here. My intention here is to show how to send an asynchronous request in a timer. That is a basic example not truly meant to understanding asynchronism, just to use it.

Since I anticipate a thoughtful technical discussion, I will first provide a brief introduction to key concepts (as I understand them), then explain why this new version of the minimal client is an improvement, and finally discuss its benefits for learning.

Concepts and Facts:

C1. In general (not just in ROS 2), an asynchronous request returns immediately after sending the request; it does not wait for a response, and its return value cannot contain the actual response.
C2. To retrieve the result of an asynchronous request, the programming language or framework must provide a mechanism to either: wait for the response, poll for it, or specify a callback to be executed upon response arrival.
C3. In ROS 2, two methods are available for handling responses: waiting (while spinning) and specifying a callback.
C4. Asynchronous calls align well with ROS 2's design because they do not interfere with spinning. This allows the execution model to remain external to the node and be defined using any Executor.
C5. Blocking a callback should be avoided, as it also blocks spinning. In a single-threaded execution model (or when using the default callback group for all callbacks), this means no other callbacks can be executed. Blocking spinning may lead to deadlocks.
C6. A hypothetical call to wait_until_future_complete inside a callback creates a peculiar situation because: a) It blocks the callback execution. b) It launches another spin within an already spinning context. Moreover, as far as I know, this approach is not feasible within a method of a node class since wait_until_future_complete requires a std::shared_ptr<NodeT> as a parameter.

Advantages of this approach:

A1. The thread that calls the asynchronous request without waiting remains unblocked and can continue executing other tasks until the response is received.
A2. As a result, deadlocks cannot occur in this context.
A3. No data races can arise, as no additional threads are required for handling the asynchronous call.
A4. The main program can select any Executor without additional considerations or precautions. In fact, it remains as simple as the ones used for topics and can be easily modified.
A5. Recurrent service calls are naturally handled by placing them in a timer callback, which is not feasible when using wait_until_future_complete.

Why this approach is better for learning ROS 2:

L1. New ROS 2 users tend to apply the first approach they learn. If their initial exposure to service clients involves wait_until_future_complete, they may try to use it in every context, even where it is inappropriate.
L2. This approach helps newcomers recognize that service clients receive data similarly to topics, reinforcing the consistency of the ROS 2 design.
L3. Understanding and using callbacks is essential in ROS 2, so introducing them in an additional context should not pose an issue.
L4. ROS 1 users may initially complain about using a pure async interface for service calls, but this will remain true regardless. The sooner they adapt to asynchronous patterns, the better.

As mentioned, I expect an extensive discussion on these matters, especially since this proposal suggests significant changes to an existing and established implementation. I encourage everyone to approach this discussion with an open mind.

@Yadunund
Copy link
Member

@fujitatomoya could please take a look? 🧇

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.

2 participants