Backport of the Events Executor (#1391) #1435

Flova · 2025-03-23T17:47:06Z

Backport of the events executor from rolling to jazzy as discussed in #1391.

Signed-off-by: Brad Martin <[email protected]>

The previous version worked on 22.04+Iron, but fails on 24.04+Jazzy or Rolling. It seems like the behavior of std::bind() may have changed when asked to bind a py::object to a callback taking a py::handle. Signed-off-by: Brad Martin <[email protected]>

Signed-off-by: Brad Martin <[email protected]>

clang-format fixes things that uncrustify tends to leave alone, but then uncrustify seems slightly unhappy with that result. Also reflow comments at 100 columns. This uses the .clang-format file from the ament-clang-format package, with the exception of SortIncludes being set to false, because I didn't like what it tried to do with includes without that override. Signed-off-by: Brad Martin <[email protected]>

Signed-off-by: Brad Martin <[email protected]>

Tests that currently work are enabled, and those that don't are annotated what needs to be done before they can be enabled Signed-off-by: Brad Martin <[email protected]>

Signed-off-by: Brad Martin <[email protected]>

It was being used only inconsistently, and on reflection it wasn't adding any protection beyond what the GIL already provides. Signed-off-by: Brad Martin <[email protected]>

Needed to release the GIL while blocking on another lock. Signed-off-by: Brad Martin <[email protected]>

Signed-off-by: Brad Martin <[email protected]>

Never bool-test a py::object::attr() return value without an explicit py::cast<bool>. Signed-off-by: Brad Martin <[email protected]>

Signed-off-by: Brad Martin <[email protected]>

EventsExecutor was exploding because the test was leaving destroyed entities in the node by using an incorrect API to destroy them. Signed-off-by: Brad Martin <[email protected]>

…om it * Test methods need to be prefixed with 'test_' in order to function. This had been entirely dead code, and when I enabled it EventsExecutor deadlocked. * On reflection, it seems like a deadlock is exactly what should be expected from what this test is doing. Remove EventsExecutor from the test and document the problem. Signed-off-by: Brad Martin <[email protected]>

Co-authored-by: Janosch Machowinski <[email protected]> Signed-off-by: Brad Martin <[email protected]>

This both prevents an issue where user callbacks could potentially queue up if they didn't dispatch fast enough, and ensures that a timer that has passed its expiration without being dispatched yet can still be canceled without the user callback being delivered later. This commit also makes use of the new rcl API for querying the absolute timer expiration time, instead of the relative number of nanoseconds remaining until it expires. This should both make things more accurate, and potentially reduce overhead as we don't have to re-query the current time for each outstanding timer. Signed-off-by: Brad Martin <[email protected]>

…ckManager Signed-off-by: Brad Martin <[email protected]>

Signed-off-by: Brad Martin <[email protected]>

This both reduces duplicate code now, and simplifies the asio interface used which would need replacing. Signed-off-by: Brad Martin <[email protected]>

This method can't be allowed to leave its Future done callback outstanding in case the method is returning for a reason other than the Future being done. Signed-off-by: Brad Martin <[email protected]>

Signed-off-by: Brad Martin <[email protected]>

This is dumb on its own, but it helps me move towards eliminating asio. Signed-off-by: Brad Martin <[email protected]>

This isn't ideal because there are some ways async callbacks could become unblocked which wouldn't get noticed right away (if at all); however this seems to match the behavior of SingleThreadedExecutor. Signed-off-by: Brad Martin <[email protected]>

Signed-off-by: Brad Martin <[email protected]>

Python 3.9, still used by RHEL 9, doesn't seem to understand '|' syntax, and Optional/Union seems to be what gets used throughout the rest of the codebase. Additionally, fix a couple other mypy nits: * mypy is mad that I haven't explicitly annotated every __init__ as returning None * mypy wants generic arguments to service and action clients now Signed-off-by: Brad Martin <[email protected]>

pybind11::hash() is documented as returning ssize_t, but this seems to be a lie because MSVC doesn't understand that type; so, let's just return whatever we do get. Signed-off-by: Brad Martin <[email protected]>

MSVC didn't like the more concise method. Signed-off-by: Brad Martin <[email protected]>

Signed-off-by: Brad Martin <[email protected]>

time.monotonic() has a resolution of 16ms, which is way too coarse for the intervals this test is trying to measure. Signed-off-by: Brad Martin <[email protected]>

Flova · 2025-03-23T20:19:34Z

@alsora All tests pass in my clean jazzy docker container. Feel free to run the CI if you are happy with the PR.

I many cherry-picked all the relevant commits and adapted any issues that were indicated by the tests. This mainly included the following changes:

Generics have been added to some parts of rclpy in a later version. Therefore, we can not use them, and they were removed from the affected type hints.
As @bmartin427 predicted in Introduce EventsExecutor implementation #1391 (comment) the TimerInfo argument is not in jazzy. Anything related to it has been removed.
The execution order of the SingleThreadedExecutor is not fifo in jazzy. This has been fixed in rolling, but the PR will not be backported, as this would be a breaking change. Therefore, a special case was added to one test that flips the order so we expect a fifo behavior for the events executor.
A usage of the deprecated, but otherwise identical, QoSEventHandler was changed to EventHandler, because it leads to a test failure when the create_event_handlers API was used with a matched callback.

- Generics have been added to some parts of rclpy in a later version. Therefore, we can not use them, and they were removed from the affected type hints. - The TimerInfo argument is not present in jazzy. Anything related to it is removed. - The execution order of the SingleThreadedExecutor is not fifo in jazzy. This has been fixed in rolling, but the PR will not be backported, as this would be a breaking change. Therefore, a special case is added to one test that flips the order so we expect a fifo behavior for the events executor. - A usage of the deprecated, but otherwise identical, QoSEventHandler is changed to EventHandler, because it leads to a test failure when the create_event_handlers API is used with a matched callback. Signed-off-by: Florian Vahl <[email protected]>

jmachowinski · 2025-03-24T09:02:22Z

As @bmartin427 predicted the TimerInfo argument is not in jazzy. Anything related to it has been removed.

That is strange, as the rcl and rclcpp changes were merge prior to jazzy

The execution order of the SingleThreadedExecutor is not fifo in jazzy.

It the python executor different from the rclcpp one ? The rclcpp one is explicit entity priority based

jmachowinski · 2025-03-24T09:03:23Z

Pulls: #1435
Gist: https://gist.githubusercontent.com/jmachowinski/bc4676f491d443cefd086b998ec44a69/raw/fd0325253c33bd7b2baf3b2bf80798195cbd3299/ros2.repos
BUILD args:
TEST args:
ROS Distro: jazzy
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15445

Linux
Linux-aarch64
Linux-rhel
Windows

Flova · 2025-03-24T11:46:56Z

It the python executor different from the rclcpp one ? The rclcpp one is explicit entity priority based

I am not sure, but my comment was in reference to #1304, which is present in rolling, but not jazzy.

That is strange, as the rcl and rclcpp changes were merge prior to jazzy

It was introduced in #1292 and wasn't backported to jazzy, as it is not present in https://github.com/ros2/rclpy/blob/jazzy/rclpy/rclpy/timer.py. Also the PR was merged on Jul 30 which is after May 23, which is listed on https://docs.ros.org/en/jazzy/Releases.html as the release date.

Flova · 2025-03-24T19:31:43Z

It seems like the windows CI failed with an unrelated issue (some jenkins internal thing) right?

fujitatomoya · 2025-03-24T19:58:22Z

@Flova actually it looks like CI got cut off, i just restart the CI.

Windows

alsora · 2025-03-25T07:54:06Z

I only did a quick look, but the PR seems good to me!
The windows test error above seems unrelated.

ros-discourse · 2025-03-27T21:51:19Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/faster-rclpy-executor-now-in-rolling/42852/1

Brad Martin and others added 30 commits March 23, 2025 17:11

Introduce EventsExecutor implementation (ros2#1389)

4a08170

Signed-off-by: Brad Martin <[email protected]>

Fix ament linter complaints

a1c65f5

Signed-off-by: Brad Martin <[email protected]>

Switch to non-boost asio library

b0fbcda

Signed-off-by: Brad Martin <[email protected]>

Use internal _rclpy C++ types instead of jumping through Python hoops

63a00b2

Signed-off-by: Brad Martin <[email protected]>

Respect init signal handling options

c9d4c3c

Signed-off-by: Brad Martin <[email protected]>

Reconcile signal handling differences with SingleThreadedExecutor

6846d3b

Signed-off-by: Brad Martin <[email protected]>

test_executor.py: Add coverage for EventsExecutor

daa3464

Tests that currently work are enabled, and those that don't are annotated what needs to be done before they can be enabled Signed-off-by: Brad Martin <[email protected]>

Make spin_once() only return after a user-visible callback

3f2e4b2

Signed-off-by: Brad Martin <[email protected]>

Add get_nodes() method

2a88a1c

Signed-off-by: Brad Martin <[email protected]>

Add context management support

08ea5a2

Signed-off-by: Brad Martin <[email protected]>

Remove mutex protection on nodes_ member access

24ac2b6

It was being used only inconsistently, and on reflection it wasn't adding any protection beyond what the GIL already provides. Signed-off-by: Brad Martin <[email protected]>

Fix deadlock during shutdown()

7273573

Needed to release the GIL while blocking on another lock. Signed-off-by: Brad Martin <[email protected]>

A little further on using C++ _rclpy types instead of Python interface

27da9c7

Signed-off-by: Brad Martin <[email protected]>

Fix issue with iterating through incomplete Tasks

b416145

Never bool-test a py::object::attr() return value without an explicit py::cast<bool>. Signed-off-by: Brad Martin <[email protected]>

Add support for coroutines to timer handling

252e6e6

Signed-off-by: Brad Martin <[email protected]>

Fix test_not_lose_callback() test to destroy entities properly

14dae32

EventsExecutor was exploding because the test was leaving destroyed entities in the node by using an incorrect API to destroy them. Signed-off-by: Brad Martin <[email protected]>

Warn on every timer added over the threshold, not just the first

f90f2dd

Co-authored-by: Janosch Machowinski <[email protected]> Signed-off-by: Brad Martin <[email protected]>

Protect against deferred method calls happening against a deleted Clo…

7d765d4

…ckManager Signed-off-by: Brad Martin <[email protected]>

Add support for new TimerInfo data passed to timer handlers

58731ab

Signed-off-by: Brad Martin <[email protected]>

Simplify spin_once() implementation

a4cbf0e

This both reduces duplicate code now, and simplifies the asio interface used which would need replacing. Signed-off-by: Brad Martin <[email protected]>

Fix stale Future done callbacks with spin_until_future_complete()

5b5ed30

This method can't be allowed to leave its Future done callback outstanding in case the method is returning for a reason other than the Future being done. Signed-off-by: Brad Martin <[email protected]>

Use existing rclpy signal handling instead of asio

adfd629

Signed-off-by: Brad Martin <[email protected]>

Replace asio timers with a dedicated timer wait thread

5e62835

This is dumb on its own, but it helps me move towards eliminating asio. Signed-off-by: Brad Martin <[email protected]>

Replace asio::io_context with a new EventsQueue object

02d265b

Signed-off-by: Brad Martin <[email protected]>

Add EventsExecutor to new test_executor test from merge

2b5e5e2

Signed-off-by: Brad Martin <[email protected]>

Brad Martin added 7 commits March 23, 2025 17:37

Swap 'pragma once' for ifndef include guards

4e0db37

Signed-off-by: Brad Martin <[email protected]>

Add test coverage for Node.destroy_subscription()

16f5a8d

Signed-off-by: Brad Martin <[email protected]>

Use 'auto' in place of explicit return type on hash

c85cecf

pybind11::hash() is documented as returning ssize_t, but this seems to be a lie because MSVC doesn't understand that type; so, let's just return whatever we do get. Signed-off-by: Brad Martin <[email protected]>

Change initialization of struct members

2dd61dd

MSVC didn't like the more concise method. Signed-off-by: Brad Martin <[email protected]>

Use subTest in test_executor to distinguish which executor type failed

c5ee683

Signed-off-by: Brad Martin <[email protected]>

Use time.perf_counter() instead of time.monotonic() in executor test

9d93f27

time.monotonic() has a resolution of 16ms, which is way too coarse for the intervals this test is trying to measure. Signed-off-by: Brad Martin <[email protected]>

Flova marked this pull request as ready for review March 23, 2025 20:02

Flova force-pushed the jazzy branch from 63c94a0 to f46fb61 Compare March 23, 2025 20:23

alsora approved these changes Mar 25, 2025

View reviewed changes

bmartin427 approved these changes Mar 26, 2025

View reviewed changes

ahcorde merged commit a10ff3b into ros2:jazzy Mar 28, 2025
2 checks passed

Flova mentioned this pull request Apr 8, 2025

rclpy BufferClient lookup_transform not working, timeout ros2/geometry2#438

Open

n8hui mentioned this pull request May 3, 2025

Backport of the EventsExecutor (#1391) #1454

Open

Barry-Xu-2018 mentioned this pull request May 7, 2025

Performance issues with MultiThreadedExecutor #1452

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Backport of the Events Executor (#1391) #1435

Backport of the Events Executor (#1391) #1435

Uh oh!

Flova commented Mar 23, 2025 •

edited

Loading

Uh oh!

Flova commented Mar 23, 2025

Uh oh!

jmachowinski commented Mar 24, 2025

Uh oh!

jmachowinski commented Mar 24, 2025

Uh oh!

Flova commented Mar 24, 2025 •

edited

Loading

Uh oh!

Flova commented Mar 24, 2025

Uh oh!

fujitatomoya commented Mar 24, 2025

Uh oh!

alsora commented Mar 25, 2025

Uh oh!

ros-discourse commented Mar 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Backport of the Events Executor (#1391) #1435

Backport of the Events Executor (#1391) #1435

Uh oh!

Conversation

Flova commented Mar 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Flova commented Mar 23, 2025

Uh oh!

jmachowinski commented Mar 24, 2025

Uh oh!

jmachowinski commented Mar 24, 2025

Uh oh!

Flova commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Flova commented Mar 24, 2025

Uh oh!

fujitatomoya commented Mar 24, 2025

Uh oh!

alsora commented Mar 25, 2025

Uh oh!

ros-discourse commented Mar 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Flova commented Mar 23, 2025 •

edited

Loading

Flova commented Mar 24, 2025 •

edited

Loading