I noticed that the MultiThreadedExecutor was not operating in a multi-threaded manner, so I made some changes.
From my review of the rclcpp code, it appears that the current rclcpp::MultiThreadedExecutor only implements spin(), and spin_once() and spin_some() simply execute the implementation from the base class rclcpp::Executor.
Even after checking the implementation of rclcpp::Executor, I found no instances where the overridden spin is utilized. As a result, spin_once() and spin_some() seem to operate in a single-threaded mode. Indeed, in the header file of rclcpp::Executor, spin_some() and spin_once() are described as "suitable for a single-threaded model of execution."
Therefore, in this pull request, I changed the implementation to use spin() instead of spinonce() for MultiThreadedExecutor. Consequently, the stop flag became unnecessary and was removed. It has been confirmed that the spin thread terminates correctly with executor_->cancel(), without using the stop_ flag.
As a side note, I would like to mention that MultiThreadedExecutor::spin() is also used in the ros2_control_node.
This is an automatic backport of pull request #315 done by Mergify.
I noticed that the MultiThreadedExecutor was not operating in a multi-threaded manner, so I made some changes.
From my review of the rclcpp code, it appears that the current rclcpp::MultiThreadedExecutor only implements spin(), and spin_once() and spin_some() simply execute the implementation from the base class rclcpp::Executor.
Implementation of MultiThreadedExecutor::spin():
Even after checking the implementation of rclcpp::Executor, I found no instances where the overridden spin is utilized. As a result, spin_once() and spin_some() seem to operate in a single-threaded mode. Indeed, in the header file of rclcpp::Executor, spin_some() and spin_once() are described as "suitable for a single-threaded model of execution."
Additionally, similar observations were made in a related pull request for rclcpp. https://github.com/ros2/rclcpp/pull/2454
There was also an old issue related to this problem, but it seems to have been closed without appropriate changes. https://github.com/ros2/rclcpp/issues/85
Therefore, in this pull request, I changed the implementation to use spin() instead of spinonce() for MultiThreadedExecutor. Consequently, the stop flag became unnecessary and was removed. It has been confirmed that the spin thread terminates correctly with executor_->cancel(), without using the stop_ flag.
As a side note, I would like to mention that MultiThreadedExecutor::spin() is also used in the ros2_control_node.
This is an automatic backport of pull request #315 done by Mergify.