feat(MultiThreadedExecutor): Added ability to handle exceptions from threads #2382

jmachowinski · 2023-12-06T16:53:56Z

This commit adds external exception handling for the worker threads, allowing application code to implement custom exception handling.

jmachowinski · 2024-01-29T08:46:28Z

@wjwwood @mjcarroll @fujitatomoya Ping, I would like to see this merged for jazzy

fujitatomoya

@jmachowinski thanks for the PR. i think this is useful! a couple of minor comments. (if we merge this, i think it would be nice to update the demos/example aligned with this.)

rclcpp/include/rclcpp/executors/multi_threaded_executor.hpp

rclcpp/src/rclcpp/executors/multi_threaded_executor.cpp

fujitatomoya

lgtm with green CI.

@alsora friendly ping.

rclcpp/test/rclcpp/executors/test_multi_threaded_executor.cpp

rclcpp/include/rclcpp/experimental/executors/events_executor/events_executor.hpp

fujitatomoya · 2024-03-29T16:44:39Z

@alsora can you check and resolve your comments above?

fujitatomoya · 2024-03-29T17:18:36Z

@jmachowinski can you rebase to rolling? i would like to make sure CI is green with current HEAD especially at this release moment.

after rebase, i need to start CI with https://gist.githubusercontent.com/fujitatomoya/fef2fe491b4f10fd5120cc3907fab525/raw/a05837c1b3c2ab1de927d222ef4fcb68fcd82850/ros2.repos

alsora · 2024-04-01T19:09:42Z

Looks good to me

jmachowinski · 2024-04-03T18:35:53Z

@jmachowinski can you rebase to rolling? i would like to make sure CI is green with current HEAD especially at this release moment.

rebased, note this currently fails the pipline, as it uncovered the bug reported in #2474

fujitatomoya · 2024-04-12T18:39:12Z

@jmachowinski this is ready for CI with ros2/demos#670? just chekcing.

mjcarroll · 2024-04-12T18:41:50Z

Linux
Linux-aarch64
Windows

…threads This commit adds external exception handling for the worker threads, allowing application code to implement custom exception handling. feat(Executor): Added spin API with exception handler. Signed-off-by: Janosch Machowinski <[email protected]>

Signed-off-by: Janosch Machowinski <[email protected]>

jmachowinski · 2024-04-13T11:12:02Z

rebased to rolling

I don't know why the CI failed, it builds for me using this file:
https://github.com/jmachowinski/build_desc/raw/exeception_handling/ros2.repos

fujitatomoya · 2024-04-15T16:34:11Z

CI:

Linux
Linux-aarch64
Windows

wjwwood · 2024-04-16T00:04:03Z

I need to make some changes before merging this, but I cannot push to this branch, so I'll be opening a new pr. Please don't merge this one.

wjwwood

I am going to fix up this pr and run CI on it as an option to merge, but I really think that it would be preferable to have an exception handler callback as an optional part of the ExecutorOptions instead, passing it during construction and/or setting it after construction with a new method, rather than modifying all of these spin functions and causing down stream changes.

Please don't merge this version of the pr, see: #2505

wjwwood · 2024-04-16T02:20:14Z

rclcpp/include/rclcpp/executor.hpp

+  /**
+   * \sa rclcpp::Executor:spin() for more details
+   * \throws std::runtime_error when spin() called while already spinning
+   * @param exception_handler will be called for every exception in the processing threads
+   *
+   * The exception_handler can be called from multiple threads at the same time.
+   * The exception_handler shall rethrow the exception it if wants to terminate the program.
+   */
+  RCLCPP_PUBLIC
+  virtual void
+  spin(const std::function<void(const std::exception & e)> & exception_handler) = 0;


I would have liked to have had a non-virtual spin(... exception_handler = nullptr), that called a virtual and protected spin_impl(... exception_handler) = 0, so that we didn't need two signatures. It would be easier to document and wouldn't require every executor to implement two functions, one of which is pretty much always be boilerplate calling the other.

However, I don't know if I have time to do that before the freeze, so I won't block this on it, but I wanted to make the comment somewhere.

wjwwood · 2024-04-16T02:21:07Z

rclcpp/include/rclcpp/executor.hpp

+  /**
+   * \sa rclcpp::Executor:spin() for more details
+   * \throws std::runtime_error when spin() called while already spinning
+   * @param exception_handler will be called for every exception in the processing threads


We don't use \ everywhere (some other @ have slipped through reviews), but we should tend towards the desired state when possible, which is \ everywhere. I'll fix these up in my version of this pr.

wjwwood · 2024-04-16T02:21:37Z

rclcpp/include/rclcpp/executor.hpp

+  /**
+   * \sa rclcpp::Executor:spin() for more details


This needs a "brief", with a /// ... line before the block comment.

wjwwood · 2024-04-16T02:23:56Z

rclcpp/include/rclcpp/experimental/timers_manager.hpp

@@ -125,9 +125,14 @@ class TimersManager
  /**
   * @brief Starts a thread that takes care of executing the timers stored in this object.
   * Function will throw an error if the timers thread was already running.
+   *
+   * @param exception_handler if valid, the execution of the timer will be done in a try catch block,


I'll leave this @, because this file uses that style, but the whole file should be updated (it mostly got a review pass for being in the experimental namespace.

wjwwood · 2024-04-16T02:25:06Z

rclcpp/src/rclcpp/executor.cpp

@@ -395,6 +395,69 @@ Executor::execute_any_executable(AnyExecutable & any_exec)
  any_exec.callback_group->can_be_taken_from().store(true);
 }

+template<typename Function>
+void execute_guarded(
+  const Function & fun,


nitpick: use full variable names

Suggested change

const Function & fun,

const Function & function,

wjwwood · 2024-04-16T02:26:59Z

rclcpp/src/rclcpp/executors/static_single_threaded_executor.cpp

+
+void
+StaticSingleThreadedExecutor::spin(
+  const std::function<void(const std::exception & e)> & exception_handler)
+{
+  if (spinning.exchange(true)) {
+    throw std::runtime_error("spin() called while already spinning");
+  }
+  RCPPUTILS_SCOPE_EXIT(this->spinning.store(false); );
+
+  // This is essentially the contents of the rclcpp::Executor::wait_for_work method,
+  // except we need to keep the wait result to reproduce the StaticSingleThreadedExecutor
+  // behavior.
+  while (rclcpp::ok(this->context_) && spinning.load()) {
+    this->spin_once_impl(std::chrono::nanoseconds(-1), exception_handler);
+  }
+}
+


We should not copy-paste like this, we should have one call the other...

wjwwood · 2024-04-16T02:28:03Z

rclcpp_lifecycle/test/benchmark/benchmark_lifecycle_client.cpp

-    spinner_ = std::thread(&rclcpp::executors::SingleThreadedExecutor::spin, executor);
+    spinner_ = std::thread([this]() {executor->spin();});


Why is this change needed?

I see it doesn't compile... this is the kind of change that suggests we should rethink the approach. The old style of calling it should still work I think. I'll leave it, but it's another point against this approach in my mind.

fujitatomoya · 2024-04-16T03:01:53Z

but I really think that it would be preferable to have an exception handler callback as an optional part of the ExecutorOptions instead

+1 for this.

wjwwood · 2024-04-16T03:28:04Z

Another concern I've got after trying to shape this up for merge is that there's no way to indicate if an exception should cause the executor to stop or not. You can re-throw, but doing that in a thread will trigger termination, which by default will head towards std::abort, but can be overridden see: https://en.cppreference.com/w/cpp/error/set_terminate

Instead, I'd like to have seen either a return value from the exception handler that can tell the executor to stop or throw from the main thread or something, and/or pass the executor into the exception handler so that it can cancel it if desired.

wjwwood · 2024-04-16T04:28:53Z

Closing in favor of the updated version #2505

jmachowinski requested review from ivanpauno, hidmic and wjwwood as code owners December 6, 2023 16:53

jmachowinski force-pushed the exception_handling branch from daa95df to dee34bc Compare January 29, 2024 08:44

jmachowinski force-pushed the exception_handling branch from dee34bc to 80ecbd8 Compare January 29, 2024 15:00

fujitatomoya reviewed Feb 12, 2024

View reviewed changes

rclcpp/include/rclcpp/executors/multi_threaded_executor.hpp Outdated Show resolved Hide resolved

rclcpp/src/rclcpp/executors/multi_threaded_executor.cpp Outdated Show resolved Hide resolved

jmachowinski force-pushed the exception_handling branch 2 times, most recently from 51c3b85 to 7fd9c1a Compare February 13, 2024 17:14

jmachowinski force-pushed the exception_handling branch 2 times, most recently from 68979e9 to e62d7f6 Compare March 7, 2024 14:19

jmachowinski requested review from fujitatomoya and alsora March 7, 2024 14:19

jmachowinski force-pushed the exception_handling branch from e62d7f6 to 070d71e Compare March 7, 2024 14:36

fujitatomoya approved these changes Mar 7, 2024

View reviewed changes

alsora reviewed Mar 7, 2024

View reviewed changes

rclcpp/test/rclcpp/executors/test_multi_threaded_executor.cpp Outdated Show resolved Hide resolved

alsora reviewed Mar 7, 2024

View reviewed changes

rclcpp/include/rclcpp/experimental/executors/events_executor/events_executor.hpp Outdated Show resolved Hide resolved

jmachowinski force-pushed the exception_handling branch from 070d71e to d557d7b Compare March 11, 2024 12:28

jmachowinski requested a review from alsora March 11, 2024 12:28

alsora approved these changes Apr 1, 2024

View reviewed changes

jmachowinski force-pushed the exception_handling branch from d557d7b to cc6c0bc Compare April 3, 2024 18:34

jmachowinski force-pushed the exception_handling branch from cc6c0bc to 2a499bb Compare April 3, 2024 19:08

This was referenced Apr 3, 2024

Compile fix for upcomming changes to rclcpp::Executor ros2/geometry2#668

Merged

fix: Compilefix for changes to rclcpp::Executor ros2/demos#670

Closed

jmachowinski force-pushed the exception_handling branch 2 times, most recently from 9d19aa8 to 9d20c70 Compare April 12, 2024 14:55

jmachowinski force-pushed the exception_handling branch from 9d20c70 to 8611bbf Compare April 12, 2024 17:36

Janosch Machowinski and others added 2 commits April 13, 2024 12:29

chore: Fixes for rebase

8e6c9e3

Signed-off-by: Janosch Machowinski <[email protected]>

jmachowinski force-pushed the exception_handling branch from 8611bbf to 8e6c9e3 Compare April 13, 2024 10:45

wjwwood mentioned this pull request Apr 16, 2024

[wjwwood] feat(MultiThreadedExecutor): Added ability to handle exceptions from threads #2505

Open

wjwwood requested changes Apr 16, 2024

View reviewed changes

wjwwood closed this Apr 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(MultiThreadedExecutor): Added ability to handle exceptions from threads #2382

feat(MultiThreadedExecutor): Added ability to handle exceptions from threads #2382

jmachowinski commented Dec 6, 2023

jmachowinski commented Jan 29, 2024

fujitatomoya left a comment

fujitatomoya left a comment

fujitatomoya commented Mar 29, 2024

fujitatomoya commented Mar 29, 2024 •

edited

Loading

alsora commented Apr 1, 2024

jmachowinski commented Apr 3, 2024

fujitatomoya commented Apr 12, 2024

mjcarroll commented Apr 12, 2024

jmachowinski commented Apr 13, 2024

fujitatomoya commented Apr 15, 2024

wjwwood commented Apr 16, 2024

wjwwood left a comment

wjwwood Apr 16, 2024

wjwwood Apr 16, 2024

wjwwood Apr 16, 2024

wjwwood Apr 16, 2024

wjwwood Apr 16, 2024

wjwwood Apr 16, 2024

wjwwood Apr 16, 2024

wjwwood Apr 16, 2024

fujitatomoya commented Apr 16, 2024

wjwwood commented Apr 16, 2024

wjwwood commented Apr 16, 2024

		spinner_ = std::thread(&rclcpp::executors::SingleThreadedExecutor::spin, executor);
		spinner_ = std::thread([this]() {executor->spin();});

feat(MultiThreadedExecutor): Added ability to handle exceptions from threads #2382

feat(MultiThreadedExecutor): Added ability to handle exceptions from threads #2382

Conversation

jmachowinski commented Dec 6, 2023

jmachowinski commented Jan 29, 2024

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya commented Mar 29, 2024

fujitatomoya commented Mar 29, 2024 • edited Loading

alsora commented Apr 1, 2024

jmachowinski commented Apr 3, 2024

fujitatomoya commented Apr 12, 2024

mjcarroll commented Apr 12, 2024

jmachowinski commented Apr 13, 2024

fujitatomoya commented Apr 15, 2024

wjwwood commented Apr 16, 2024

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujitatomoya commented Apr 16, 2024

wjwwood commented Apr 16, 2024

wjwwood commented Apr 16, 2024

fujitatomoya commented Mar 29, 2024 •

edited

Loading