Executor callbacks are no longer in a predictable order

[thomasmoore-torc]

Bug report

Required Info:

• Operating System:
  • Ubuntu 20.04
• Installation type:
  • Source
• Version or commit hash:
  • https://github.com/ros2/ros2/blob/d4c09985560a2357ad7fca9e20f39d9ab040666b/ros2.repos
• DDS implementation:
  • Fast-RTPS
• Client library (if applicable):
  • rclcpp

Steps to reproduce issue

1. Build the https://github.com/boschresearch/ros2_response_time_analysis project against ROS2 Jazzy (note that some minimal changes are required).
2. Observe that the results of running the test show that the callbacks are executed in an arbitrary order as compared to the previous ordering which was with respect to the order in which the subscriptions/services were added to the node.

Expected behavior

With ROS2 Eloquent through Iron, the callbacks all executed in the following order, where the ordering was defined by the order in which the subscriptions/services were added to the node.

Actual behavior

With ROS2 Jazzy, the ordering is now arbitrary and not related to the order in which messages were received or the order in which subscriptions were added to the node. The execution order of timers also appears to be affected.

NOTE: While the low, medium, and high subscriptions are processed in the same order that they are transmitted, I believe this to be a fluke as the service and timer callbacks are not processed in the order they were registered or sent. For reference, the callbacks are registered in the following order:

1. high
2. medium
3. low
4. srv_high
5. srv_medium
6. srv_low
7. timers (as directed by the test)

And the topics/services are sent in the following order:

1. low
2. medium
3. high
4. srv_high
5. srv_low
6. low
7. medium
8. high
9. srv_high
10. srv_low
11. srv_medium
12. srv_medium
13. high

With timers set to expire at 200, 200, 2300, and 2300 milliseconds into the test.

Additional information

I believe this change is a result of #2142 due to the EntityCollection class being based on std::unordered_map where the key is the handle returned by rcl for the timer/subscription/etc. The entities are then added to the wait set in whatever order they are iterated through by the std::unordered_map and thus processed in that order. I haven't had the opportunity to test it, but I believe the ordering could be different from system to system or even run to run as the rcl handle values change.

The default AllocatorMemoryStrategy used in previous versions of rclcpp utilized std:vector for storing rcl handles, which maintained the order in which they were added to the node.

[alsora]

Having a fixed order of callbacks is a design flaw of the SingleThreadedExecutor.
IMO users shouldn't rely on it.

Callbacks should be part of an event-driven pipeline and so they should be invoked in the order in which they are triggered, this is what happens with other executors such as the EventsExecutor.

[fujitatomoya]

@thomasmoore-torc thanks for pointing out the behavior difference.

although i am not sure if that is design flaw of SingleThreadedExecutor, i am with @alsora 's comment that the application should not rely on that.
however, some user application takes advantage of this fixed order behavior to prioritize the event in the same entity as described here. in that case, this is a big behavior change for user application... at least we should put this down on jazzy release note?

CC: @clalancette @mjcarroll @wjwwood @alsora

[thomasmoore-torc]

For me, the concern is that the executor has behaved in the same manner likely since its inception and certainly across many releases of ROS2, and there is published work that describes this behavior. It's very likely that some users of the default executor have made use of this behavior and will break when migrating to Jazzy.

It's also concerning to me that, while the previous behavior was not necessarily defined, it was at least somewhat deterministic in that whatever callbacks were ready in the wait set were executed in the order that the callbacks were registered. The new approach is completely non-deterministic because the order is based on the hash of the memory addresses of the rcl handles for the waitables (e.g. the keys of the unordered maps). This seems like a step backwards to me since determinism is something that has been discussed as a need for users of ROS2.

This change certainly caught me off guard and caused me to spend a few hours today understanding what changed, which is why I thought it was important to raise it as a potential issue.

[mjcarroll]

I fall somewhere in the middle on this one.

While I wouldn't go so far as to call it a design flaw, I think relying on the order that the callbacks are invoked based on how they are registered is probably a bit of a code smell. At the same time, ROS 2 has been around long enough for Hyrum's law to apply at this point.

With a sufficient number of users of an API,
it does not matter what you promise in the contract:
all observable behaviors of your system
will be depended on by somebody.

I think that the new implementation is still deterministic in that things that are ready from a particular instance of wait will be executed, though the order may now be different.

To understand this a bit more, can you expand on what the impact you are seeing is? It was clearly enough to get your attention, so I'm curious what caused you to notice.

[wjwwood]

Thanks for bringing this up @thomasmoore-torc, and for making such a thorough issue about it. We certainly didn't mean to disrupt folks with the change, but that was always a possibility.

If I understand it correctly, then this change in behavior is rooted in our use of the unordered map, now ordering based on hashing rather than registration order (as a "final" tie-breaker)? If that's the case, then I think we could try to get back to that behavior, as long as it doesn't significantly impact performance (which it isn't clear to me if that's the case or not) or break any of our new tests (it shouldn't as those tests shouldn't care about execution order).

For me, the concern is that the executor has behaved in the same manner likely since its inception and certainly across many releases of ROS2, and there is published work that describes this behavior.

At the same time, ROS 2 has been around long enough for Hyrum's law to apply at this point.

Hyrum's rule aside, we have major releases and versions specifically so we can change things over time, as well as API specs so that we can control what is depended on and what isn't. And users should not have been depending on this particular behavior in my opinion, even if that behavior made sense and/or was least surprising. I have run into similar assumptions on my part about how APIs should behave in other libraries, e.g. the C++ std library, but after understanding the reason for the behavior, I never felt like it was the APIs fault, merely a lack of understand of the fundamentals on my part. We can't operate under the assumption that any part of the API or implementation is "done" and will never change, published works or not.

We have to also consider how this impacts other users who want more performance and throughput but do not depend on the ordering as described above. Should we restore the ordering at a performance loss? For me that's problematic because it violates the "don't pay for what you don't use" principal.

That all being said, hopefully we can do both (keep the improvements of these changes and make the behavior more like the old behavior). And/or we can consider making this behavior explicit in the API contract, so we can test it and prevent regression against it, but I would say we could only do that for specific executors like the SingleThreadedExecutor perhaps. Putting that requirement about ordering on the generic Executor API is probably too restrictive to be applied to all implementations.

For now, we can add some documentation to the release notes and to the executor API docs to make clear that this ordering isn't guaranteed and that's changed in Jazzy. Anyone willing to take a stab at that?

In the meantime, I'll look at fixing the order of the events without breaking any other improvements we've made. I'll have a look at https://github.com/boschresearch/ros2_response_time_analysis, but if there were a simple regression test that passed on Iron and failed on Jazzy, that would be easiest to work with. Do you have anything like that @thomasmoore-torc?

[fujitatomoya]

For now, we can add some documentation to the release notes and to the executor API docs to make clear that this ordering isn't guaranteed and that's changed in Jazzy. Anyone willing to take a stab at that?

@wjwwood i can take care of this. CC: @thomasmoore-torc

[thomasmoore-torc]

To understand this a bit more, can you expand on what the impact you are seeing is? It was clearly enough to get your attention, so I'm curious what caused you to notice.

@mjcarroll - I was experimenting with an idea to use the clock jump callback to allow for execution of timer callbacks immediately on receipt of the /clock message as opposed to after the timer's guard condition is set to allow for a more deterministic ordering of callbacks. I was basing my experiment on the observation that callbacks are executed in their registration order, which would allow my experiment to call the timer callbacks before the other subscription callbacks. While it was just an experiment, I noticed that the callback ordering was not what it had been in previous version of ROS, which is what prompted me to raise this issue.

We have to also consider how this impacts other users who want more performance and throughput but do not depend on the ordering as described above. Should we restore the ordering at a performance loss? For me that's problematic because it violates the "don't pay for what you don't use" principal.

@wjwwood - this is a fair statement. However, I believe that determinism in robotics is just as important as performance, and maybe even more so in some aspects of robotics where safety is a consideration. That said, it's really hard to make a general purpose framework that meets the needs of everyone, so I understand where you are coming from.

I'm still wrapping my head around how the WaitSet is incorporated into the executor. It seems to me that the MemoryStrategy approach previously used by the executor has now been distributed among the wait_set_policies and the ExecutorEntitiesCollection, neither of which are configurable in the current implementation of the executor in a similar manner to how the MemoryStrategy was configurable previously. Perhaps a potential solution to this issue is to find a way to make these configurable within the executor to allow users to provide their own implementations without having to fully create a new executor.

[alsora]

Perhaps a potential solution to this issue is to find a way to make these configurable within the executor to allow users to provide their own implementations without having to fully create a new executor.

The EventsExecutor indeed allows to define custom "queue" implementations.
The queue class defines the ordering in which ready entities are processed in one iteration of "spin".

We currently only have a FIFO queue, based on the order in which entities are triggered.
It's possible to define queues with fixed order of entities (either arbitrary or user-defined).
Custom queues can live outside of rclcpp, in the users workspaces.

We should consider if/when we will deprecate the waitset-based executors like SingleThreadedExecutor due to their problems (mostly performance).

[thomasmoore-torc]

@alsora - to be fair, the EventsExecutor does indeed work for the experiment I was performing. However, this issue is more about the fact that the order in which the waitset-based executors like SingleThreadedExecutor processes callbacks has changed.

[thomasmoore-torc]

@fujitatomoya - it might be worth adding something to the docs page for the Executors in the Scheduling semantics section: https://docs.ros.org/en/jazzy/Concepts/Intermediate/About-Executors.html#scheduling-semantics

[alsora]

this issue is more about the fact that the order in which the waitset-based executors like SingleThreadedExecutor processes callbacks has changed.

Yes, the reason why I brought up the EventsExecutor is mostly because if we want to make improvements to the executors, which is a critical part of ROS applications, we should be very clear about what's an expected behavior and part of the API contract or not.
For example, if we explicitly indicated that the SingleThreadedExecutor (which is the current default) must have a fixed order of execution, it may be a lot harder to eventually change the default to a different executor, since other implementation may not guarantee that (for whatever reason).

IMO, it's worth investigating if it's possible to keep the old behavior without affecting performance, but I wouldn't go as far as adding to the API description (nor I would restore the old behavior if it has performance implications).
We should rather warn users that they shouldn't rely on the implementation details of the executor.