Filter service requests for inactive managed nodes

[huchijwk]

Implement the service request filtering function based on managed node's requirements in https://design.ros2.org/articles/node_lifecycle.html

Signed-off-by: Wonguk Jeong [email protected]

[huchijwk]

This PR is about unimplemented service request filtering in the managed node design.

Primary State: Inactive

... While in this state, the node will not receive any execution time to read topics, perform processing of data, respond to functional service requests, etc.

This includes API changes where LifecycleNode's create_service API return type has changed.

• rclcpp::Service -> rclcpp_lifecycle:LifecycleService

[alsora]

Thank you for the PR. Indeed there is currently a gap between the design and the implementation of lifecycle nodes. This applies also to subscriptions.

However, I think that this is a complex situation where we need to evaluate various possible approaches.

What happens with your PR is that if an inactive server receives a request, it will still try to execute it (i.e. calling the new handle_request), but then it will return immediately before doing any work or producing a response. The consequence is that on the client side, there is no way to know that the request has been dropped. When the server node becomes active again, the dropped request is now lost and it will not be processed.

In my opinion, a better approach would be to act at the executor layer. If a server is inactive, it shouldn't take new requests still leaving them into the middleware buffers. This has the advantage that as soon as the server returns active, it will be able to process those requests (assuming that it didn't receive too many of them and its queue was overrun). This has also the advantage of avoiding the performance cost required for extracting a request that will just be dropped.

[huchijwk]

@alsora thank you. I have the same concerns as you about my PR. Actually, I was thinking about overriding the take_type_erased_request of ServiceBase as a virtual function and returning false. what do you think? This prevents the executor from taking the request.

[alsora]

Mmm, I need to double check, but I'm afraid that this approach may result in the executor continuously trying to take the request and so, never going back to sleep.

I think that it's necessary to propagate the information about the service being inactive to the executor itself, such that this server is not used. For example, considering the existing waitset-based executor, the executor could check if the entity is active before adding it to the waitset

[huchijwk]

@alsora I think you're right. (executor recognizes the state and controls the waitset.) If so, isn't it right to generalize the rclcpp service to handle the active/inactive state instead of the Lifecycle-specific service (LifecycleService)? This seems to be a common feature

Ah, and I think that once this part is finished, subscription can be handled similarly as you said.

[alsora]

In general, I think it would be better to keep the concept of active/inactive entities confined to a lifecycle wrapper.

However, we currently have that in rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp we add services to a waitset via rcl_wait_set_add_service(wait_set, service.get(), NULL) but for waitables we use waitable->add_to_wait_set(wait_set);.

We could create add the add_to_wait_set also to subscriptions, services, etc. Then, the implementation of this function for a regular server would just call rcl_wait_set_add_service(wait_set, service.get(), NULL) as before, but for a lifecycle server it would first check the boolean denoting if the entity is active or not.

A call to service->deactivate() would change this boolean to false, thus making the add_to_wait_set() function do nothing.

[huchijwk]

@alsora I re-implemented,

• declare add_to_wait_set() (pure virtual) in ServiceBase
• implement add_to_wait_set() in Service
• override add_to_wait_set() in LifecycleService

And,, since add_handles_to_wait_set() of rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp traverses rcl_service_t (shared_ptr) not ServiceBase, maintains map to find ServiceBase weak pointer

[huchijwk]

@alsora done what you've mentioned.

[huchijwk]

@alsora Thanks for the review. Added a commit that applies the last comment. :)

[bpwilcox]

This PR looks great! I agree that we should apply similar changes to subscriptions and perhaps action servers as well to comply with the intended lifecycle design.

@huchijwk Do you intend to add subsequent changes for subscriptions after this PR? Also, perhaps a demo that mirrors the one in https://github.com/ros2/demos/tree/master/lifecycle but with servics/clients would be a helpful aid.

[huchijwk]

@bpwilcox Yes, I will work on subscription and demo as well after this PR is merged.

[huchijwk]

overall looks good to me, but a couple of questions.

probably this is off topic from this PR. with current implementation application is responsible to activate / inactivate endpoints such as rclcpp_lifecycle::LifecyclePublisher and rclcpp_lifecycle::LifecycleService, but i think it should be managed by LifecycleNode based on the current state?

@fujitatomoya According to the design concept, you are right. I think it makes sense to manage it in the lifecycle node, unless there is a specific rationale that it is implemented to change the state of the publisher through the API explicitly.

[huchijwk]

Rebased

[alsora]

Hi @ivanpauno @fujitatomoya , it looks like all comments have been addressed. Any reason for not moving forward with this PR?

[ivanpauno]

Hi @ivanpauno @fujitatomoya , it looks like all comments have been addressed.

Hi @alsora, I haven't done a thorough out review yet. I will try to take a look on the first week of January.

[fujitatomoya]

@alsora @bpwilcox @huchijwk could you check your comments can be resolved?

[alsora]

For some reason I don't see the "Resolve conversation" button on my comments. Anyhow, I confirm that for me and Brian (who is on vacation) this is good to go.

[huchijwk]

Comments resolved

[ivanpauno]

I think we can ignore my feedback for the moment and double check with other maintainers what they think.

@jacobperron @wjwwood could you share your thoughts about https://github.com/ros2/rclcpp/pull/1836#discussion_r774616562, https://github.com/ros2/rclcpp/pull/1836#discussion_r774618682 and https://github.com/ros2/rclcpp/issues/1846 (I followed #1846 ideas in the rclpy managed node implementation).