Introduce EventsExecutor implementation

[bmartin427]

Closes #1389

[bmartin427]

Draft form of this feature to drive discussion only. I've done some minimal work on this relative to the original code to get things in roughly the right shape; however the functional test currently seems to explode, and a bunch of the linters are unhappy. Even once this nominally works, there's a fair bit more work to be done on it, starting with the removal of the boost dependency.

[jmachowinski]

This happens directly before the the execution of the timer in the c++ version. I wonder if doing it here introduces some unwanted behavior...

[bmartin427]

None that I've encountered yet? It just seemed important to get started on the next interval as quickly as possible to keep the timer rate from drifting due to the latency in dispatching the timer. Are there any specific issues you suspect with this?

[jmachowinski]

I remember some test failing around this detail in my implementations, sadly I don't remember the details. I guess the behavior around cancel is broken now.
E.g.
A callback taking some time is running.
Timer becomes ready, is added to the ready queue
Some other callback cancels the timer
Timer is executed anyway

Is there a check if the timer was canceled directly before the execution, that I missed ?

The interval should not drift, if this call is issued later. The reason for this is, that the next trigger time is computed as :

while(nextTime < now)
{
   nextTimer = nextTime + period;
}

I think there is another bug lurking here. If a timer takes longer than its period, your queue will fill up. You need to block the timer from recomputing the next execution time, until it was executed.

[bmartin427]

Ah gotcha, yeah it won't drift but it will potentially miss whole intervals, but perhaps that was intended per your concern about queuing multiple calls.

As far as canceling, I'm not sure even SingleThreadedExecutor is safe from races on that front:

• I think a cancel between the is_timer_ready() check in _wait_for_ready_callbacks() and the call_timer_with_info() call in _take_timer() will cause an uncaught RCLError exception to be thrown:

  File "/home/brad/rclpy_ws/install/rclpy/lib/python3.12/site-packages/rclpy/executors.py", line 380, in _take_timer
    info = tmr.handle.call_timer_with_info()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
rclpy._rclpy_pybind11.RCLError: failed to call timer: timer is canceled, at ./src/rcl/timer.c:268

• A cancel any time after call_timer_with_info() will not prevent the callback already in motion from eventually being delivered

I agree though that spacing out the rcl_timer_call() from the callback dispatch does make this race more likely to hit though. I'll go ahead and rearrange that per the other concerns above.

[bmartin427]

I think this should be resolved now. Both the rcl_timer_call() and the user callback now occur from the same asio::post() event.

[jmachowinski]

hm, isn't there a race here, that the asio callback still happens into the now deleted clock_manager ?

[bmartin427]

I think the timer's own callback is safe, because it calls a user callback directly without capturing the clock manager itself. However, it may be possible for HandleJump() and UpdateTimers() to get called with a deleted this pointer, if you remove the last timer from a clock immediately after a callback was posted, and before the callback is dispatched. I'll make a note to look at this closer later.

[bmartin427]

I think this is resolved now.

[jmachowinski]

Hm, the rclcpp layer explicitly forces the waitables, to implement events handling. I don't know much about the rclpy waitable internals, but the approach taken here, would break the rclcpp waitables, so we should check this... A good candidate to have a look at should be the action interfaces.

[bmartin427]

There are test cases in test_events_executor.py specifically for actions, so I know that rclpy Waitable type works, unless you think there's something specific missing from that test coverage.

Can you clarify what it is you're looking for here? Are you wanting me to modify the Waitable interface to better support callbacks directly? That's probably a bit more ambitious and invasive than I was hoping to be, but I can look into it if you think it's necessary.

[jmachowinski]

I remember some test failing around this detail in my implementations, sadly I don't remember the details. I guess the behavior around cancel is broken now.
E.g.
A callback taking some time is running.
Timer becomes ready, is added to the ready queue
Some other callback cancels the timer
Timer is executed anyway

Is there a check if the timer was canceled directly before the execution, that I missed ?

The interval should not drift, if this call is issued later. The reason for this is, that the next trigger time is computed as :

while(nextTime < now)
{
   nextTimer = nextTime + period;
}

I think there is another bug lurking here. If a timer takes longer than its period, your queue will fill up. You need to block the timer from recomputing the next execution time, until it was executed.