EventsExecutor fails to handle async service servers

[bmartin427]

Generated by Generative AI

No response

Operating System:

Doesn't matter (Ubuntu 24.04)

ROS version or commit hash:

jazzy, rolling

RMW implementation (if applicable):

N/A

RMW Configuration (if applicable):

No response

Client library (if applicable):

rclpy

'ros2 doctor --report' output

N/A

Steps to reproduce issue

Attempt to implement a service server with an async handler and the experimental EventsExecutor

Expected behavior

Service works

Actual behavior

(Abbreviating from actual output)

Traceback (most recent call last):
  File "/opt/ros/jazzy/lib/python3.12/site-packages/rclpy/service.py", line 75, in send_response
    raise TypeError()
TypeError
sys:1: RuntimeWarning: coroutine 'handle_request' was never awaited

Additional information

This is my fault, I failed to handle or test async service servers when I implemented EventsExecutor. There was a fair amount of existing test coverage of async stuff already in tree, and I didn't realize this was missing.

The fix is probably very straightforward, HandleServiceReady() just needs to check if the thing returned from the callback was a coroutine and schedule a task, just like HandleTimerReady() does. For that matter, the other entity types probably need similar handling.

[fujitatomoya]

@bmartin427 are you going to create PR for this issue probably with test cases?

[bmartin427]

I can probably find some time to do that at some point, yes. However, I think I'm a bit confused about how this is supposed to work. I wrote a test case against SingleThreadedExecutor which, during an async service handler, set up a timer and awaited it, before then responding to the service. The timer never fired, I think because the executor was still blocked in the middle of 'taking' the ready service, and wasn't able to check for anything else being ready. That being the case, I guess I'm confused what good async handling is if the executor still blocks. I looked around a bit and didn't find any existing test coverage for async services, so I don't have a model for a workflow EventsExecutor should support. Can anyone comment on this?

[christophebedard]

We talked about this in today's client libraries working group meeting.

I wrote a test case against SingleThreadedExecutor which, during an async service handler, set up a timer and awaited it, before then responding to the service. The timer never fired, I think because the executor was still blocked in the middle of 'taking' the ready service, and wasn't able to check for anything else being ready.

None of us in the meeting were very familiar with async in rclpy and how it's supposed to behave. However, my assumption was that "real async" isn't currently supported by the rclpy executors. What you tried to do there is equivalent to a blocking client call in a subscription callback with rclcpp: it's fundamentally not supported and results in a deadlock, as you figured out.

Maybe @nadavelkabets can confirm? Since I believe this is one of the goals of #1399.

[nadavelkabets]

The timer never fired, I think because the executor was still blocked in the middle of 'taking' the ready service, and wasn't able to check for anything else being ready.

Maybe @nadavelkabets can confirm? Since I believe this is one of the goals of #1399.

Async programming in rclpy is well supported, but poorly documented and not trivial to understand.

Imagine a similar scenario in MultiThreadedExecutor:
A service callback is executed in a Thread, in which the thread blocks on an event until a timer sets the event.
By default, this scenario will end in a dead lock, due to the default MutuallyExclusiveCallbackGroup preventing the timer callback from executing while the previous service callback is still blocking.
Switching to ReentrantCallbackGroup will enable the timer callback to execute while a different callback is still blocking.
This behavior is common to other cases in rclpy, for example calling a service from within a subscription callback.

The implementation of 'SingleThreadedExecutor' shares most of the logic with MultiThreadedExecutor, including callback groups.
Somewhat unintuitively, callback groups apply the same way to SingleThreadedExecutor callbacks.
As far as I recall the documentation of callback groups only covers the multi threaded use case, except this one example: client_async_callback.py

@sloretz said in the review for #1399 that CallbackGroups are no longer required.
I think I agree, but perhaps we should discuss that in the next working group meeting.

[bmartin427]

Ohh, got it. Yeah, I had mentally filed callback groups as something that inherently only applied to multi-threaded cases, but you're right, that makes sense and does in fact work if I modify my test case.

So I guess that leaves the question what should be done with EventsExecutor. Fixing the TypeError explosion documented above is easy enough, but adding support for callback groups is more ambitious. As things stand, I suspect no mutual exclusion constraints are applied to existing async callbacks, and would not be after the trivial service fix either. I could see how difficult that is to add, however I'm not sure a lot of work on it is justified with #1399 coming along.