Single-threaded executor can be starved by timers

[dhood]

Context (comes from ros2/demos#187): A single-threaded executor has a timer scheduled for every N seconds, and also a service server.

Bug: If the timer callback takes >=N seconds to complete, the executor will never process service requests once the timer is triggered for the first time.

This is because of the combination of the following:

1. When the spinning executor calls to get_next_executable, it does not call rcl_wait since the timer's ready, so the service is not marked as ready.
2. Even if you force get_next_executable to always call wait_for_work so the server can get receive its request, the server will still not get processed by the executor because the timer will always get chosen first as the next_ready_executable.

Forcing get_next_executable to always call wait_for_work and giving timers lower priority in get_next_ready_executable will fix this situation, but it is inefficient to wait when it's not necessary, and I'm not sure checking timers last is a fix-all (could there be a parallel situation when the server needs to be the lowest priority?).

@dirk-thomas mentioned that a queue of some sort is probably more appropriate, so that get_next_executable processes events in the order that they were received.

For now, we have to recommend that users not permit timer callbacks to block for longer than the duration at which they're scheduled (this might be in our documentation somewhere already?).

[mikaelarguedas]

related to executor starvation reported #280
Right now we will match the behavior of rclpy and provide documentation as of how to make custom executors.

[Karsten1987]

@dhood I assigned you to this ticket, but added it to the untargeted milestone given the last comment of @mikaelarguedas that documentation might suffice for the moment.
Please feel free to iterate over it again or assign it to bouncy in case you this behavior has to be fixed before the release.

[dirk-thomas]

We shouldn't mark this with the milestone untargeted since that implies that we won't "ever" look at it again. Since this is actually a bug we would like to fix at some point I will remove the milestone.

[ubald]

👋 I ended up here trying to find our why when I had multiple timers with short intervals (and longer callbacks) only the first one worked, the others are never executed.

Is it possible that both issues are related?

[wjwwood]

@ubald I do not believe what you've described is the issue here. This issue is essential that we do not have fair scheduling and so as long as timers are always ready then other things like Services will never get handled. However, more than "the first one" work in this case.

[ubald]

Thanks for the quick reply!

Please bear with me here, I just started with ROS2 yesterday, I don't have a full grasp of all the pieces involved yet, but when looking at the code it looks to me like it's related:

When calling get_next_timer:

rclcpp/rclcpp/src/rclcpp/executor.cpp

Line 518 in 070b312

get_next_timer(any_exec);

It iterates over all timers in the same order each time, so it will always start by checking the first one created

rclcpp/rclcpp/src/rclcpp/executor.cpp

Lines 500 to 502 in 070b312

	for (auto & timer_ref : group->get_timer_ptrs()) {
	auto timer = timer_ref.lock();
	if (timer && timer->is_ready()) {

Then if it has been running for longer than its interval it will always be ready here in rcl

I'll see if I can switch from a debian install to a source build, this way I might try to fix it and make a PR.