API Changes for Multihtreaded Events Executor

[jmachowinski]

These are the API changes needed in order to implement an external Multihtreaded Events Executor

@mjcarroll @alsora @fujitatomoya

[jmachowinski]

Writing tests was a good suggestion, I screwed up the clock interface, it's not thread safe, I will redesign it...

[jmachowinski]

Redesigned the clock sleep interruption. Can the reviewers have a good lock at the mutex/lock/wait construct, its hard to get it right without data races....

[jmachowinski]

I'll need to add a way to terminate the clock and all sleeps as soon as the shutdown call was done.

[jmachowinski]

rebased to rolling

[mjcarroll]

Waiting for this to get in before running CI: https://github.com/ros2/rosidl_typesupport_fastrtps/pull/117

[mjcarroll]

• Linux
• Linux-aarch64
• Windows

[jmachowinski]

rebased and (hopefully) fixed windows compile error

[alsora]

• Linux
• Linux-aarch64
• Windows

[jmachowinski]

Dropped the test refactoring.

[mjcarroll]

• Linux
• Linux-aarch64
• Windows

[mjcarroll]

@jmachowinski this needs a merge from rolling because of the added API

[jmachowinski]

rebased. test_executors_timer_cancel_behavior is flaky on my machine. I hope this gets fixed by #2500

[mjcarroll]

• Linux
• Linux-aarch64
• Windows

[mjcarroll]

Looks like a failure in test_actions cli, which is unrelated to these changes, is this test known-flaky? https://ci.ros2.org/job/ci_linux/20835/testReport/junit/ros2action.ros2action.test/test_cli/test_cli/

[wjwwood]

I merged https://github.com/ros2/rclcpp/pull/2500, so we can see if that addressed what you hoped it did @jmachowinski. Also, I have some questions about the changes to executor.hpp and executors.hpp, which I'd like to understand before giving the 👍.

[wjwwood]

I'm highly suspect of the changes to the Executor API to make it so you don't need to inherit from rclcpp::Executor. I don't think we should go this route until it's explained better why that's necessary.

[jmachowinski]

rebased to rolling

[jmachowinski]

I reworked this PR. Now everything in rclcpp::Executor is made virtual.

I changed two function, that were template function, without any reason to normal functions.

I reworked the template function spin_until_future_complete, so that it can be fully overwritten by a subclass.

I added an constructor, that will not touch anything in the system.

@wjwwood can you do a review ?

[jmachowinski]

Pretty close, had a few small style things and a suggestion to scope down the changes.

Also, I think there might still be some style issues? please run ament_uncrustify path/to/rclcpp --reformat if you haven't already (saw some if( that should be if ().

I did that, uncrustify doesn't fix this (?). BTW, why are we not using clang-format ? In my experience it works way better then the uncrustify / cpplint construct.

[wjwwood]

I did that, uncrustify doesn't fix this (?).

I did this and if found the issues (and auto-formatted the fix when I used --reformat):

william@markook-ubuntu-2204:~/ros2_ws/src/ros2/rclcpp (git:wjwwood/executor_features:163f727)
$ ament_uncrustify .
...
Code style divergence in file 'rclcpp/test/rclcpp/test_clock.cpp':

--- rclcpp/test/rclcpp/test_clock.cpp
+++ rclcpp/test/rclcpp/test_clock.cpp.uncrustify
@@ -116,4 +116,4 @@
-        Clocks,
-        TestClockWakeup,
-        ::testing::Values(
-            RCL_SYSTEM_TIME, RCL_ROS_TIME, RCL_STEADY_TIME
+  Clocks,
+  TestClockWakeup,
+  ::testing::Values(
+    RCL_SYSTEM_TIME, RCL_ROS_TIME, RCL_STEADY_TIME
@@ -185,2 +185,3 @@
-  for(size_t nr = 0; nr < thread_finished.size(); nr++) {
-    threads.push_back(std::thread(
+  for (size_t nr = 0; nr < thread_finished.size(); nr++) {
+    threads.push_back(
+      std::thread(
@@ -189 +190 @@
-        // make sure the thread starts sleeping late
+          // make sure the thread starts sleeping late
@@ -192 +193 @@
-      }));
+        }));
@@ -198 +199 @@
-  for(const bool & finished : thread_finished) {
+  for (const bool & finished : thread_finished) {
@@ -209,2 +210,2 @@
-    for(const bool finished : thread_finished) {
-      if(!finished) {
+    for (const bool finished : thread_finished) {
+      if (!finished) {
@@ -219 +220 @@
-  for(const bool finished : thread_finished) {
+  for (const bool finished : thread_finished) {
@@ -223 +224 @@
-  for(auto & thread : threads) {
+  for (auto & thread : threads) {
...

Are you running it correctly?

[clalancette]

BTW, why are we not using clang-format ? In my experience it works way better then the uncrustify / cpplint construct.

We've had many long, detailed conversations about this, some as recently as a month ago. While I won't rehash all of the arguments, the short of it is:

1. It is not possible to configure clang-format to meet our preferred style.
2. Even if we were to change our preferred style to meet what clang-format wants, we have literally hundreds of packages in the core that would need to be changed. And they would all have to be changed at once. And it would mean that we could no longer cleanly backport from Rolling to stable branches.

We have many real problems in the code, this doesn't seem to be one of them (in my opinion).

[wjwwood]

Closing in favor of https://github.com/ros2/rclcpp/pull/2506

[wjwwood]

better then the uncrustify / cpplint construct.

Also, for what it's worth, I think clang-tidy might overlap a bit with cpplint, but it wouldn't completely replace it. most projects that use cpplint also use clang-tidy or something similar along side it.

[jmachowinski]

Are you running it correctly?

Here is a paste of my system, it is clearly not working, but why ?

https://pastebin.com/gGbPLdaA

[clalancette]

Here is a paste of my system, it is clearly not working, but why ?

Oh, you know, we've recently updated to a new version of uncrustify (0.78.1), that does things slightly differently than the old way. What version of uncrustify are you using?

[jmachowinski]

Oh, you know, we've recently updated to a new version of uncrustify (0.78.1), that does things slightly differently than the old way. What version of uncrustify are you using?

Recent installation, uncrustify_vendor installs 0.78.1f

$ uncrustify --version
Uncrustify-0.78.1_f

I even patched ament_uncrustify to check if it picked up the right version, and this one also reports Uncrustify-0.78.1_f

I have an Uncrustify-0.72.0_f installed by apt, but even if I uninstall this version, my system still reports no style divergence.