rclcpp icon indicating copy to clipboard operation
rclcpp copied to clipboard
verified publisher icon ros2

fix: Fixed expiring of goals if events executor is used

Open jmachowinski opened this issue 1 year ago • 9 comments

This commit is only relevant if the events executor is used.

This commit adds a background thread, to create events for the expire timer of the action. Without this the action server will not expire old goal results leading to memory and performance issues.

For now, this commit is a draft. There is still a (minor) issue, that the clock used in the background thread does not work correct in the use_sim_time case (it still runs on system time).

This is an alternative to https://github.com/ros2/rclcpp/pull/2661

In contrast to #2661, it reuses the rcl internal timer and therefore existing code to recompute the expire times.

@alsora @mauropasse @mjcarroll thoughts ?

jmachowinski avatar Nov 14 '24 17:11 jmachowinski

I don't think creating a new thread per ActionServer is good solution, the TimersManager of the EventsExecutor is already creating a thread to handle timers, so I think that approach should be used. Threads can be expensive on resource-constrained platforms. It has been discussed here that maybe the best approach is removing the need for goals to expire, by not having servers storing results.

mauropasse avatar Nov 18 '24 09:11 mauropasse

@mauropasse Note, this is a backport to jazzy. The timer interface is not exposed, therefore there is no way to register a timer at the executor without breaking ABI/API.

jmachowinski avatar Nov 18 '24 10:11 jmachowinski

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

https://discourse.ros.org/t/next-client-library-wg-meeting-friday-22nd-november-2024/40703/1

ros-discourse avatar Nov 19 '24 16:11 ros-discourse

@jmachowinski just checking that this is jazzy of https://github.com/ros2/rclcpp/pull/2699 (rolling) to keep the ABI/API, right?

fujitatomoya avatar Jan 30 '25 19:01 fujitatomoya

@fujitatomoya yes, this is the jazzy version, as the rolling version was merged, I'll try to polish it within the next days...

jmachowinski avatar Feb 04 '25 16:02 jmachowinski

@jmachowinski sounds great! ping me whenever it is ready, happy to review. (trading reviews 👍 )

fujitatomoya avatar Feb 04 '25 17:02 fujitatomoya

@fujitatomoya ready for review. Note, this builds up on top of the code from https://github.com/ros2/rclcpp/pull/2691

jmachowinski avatar Feb 07 '25 14:02 jmachowinski

Pulls: ros2/rclcpp#2674 Gist: https://gist.githubusercontent.com/jmachowinski/4e36b98ee3c6bdaf6f68fe559d84654c/raw/58c792ad266e5313e16d44ee0d1d0a12f9655843/ros2.repos BUILD args: TEST args: ROS Distro: jazzy Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15207

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

jmachowinski avatar Feb 17 '25 10:02 jmachowinski

Pulls: ros2/rclcpp#2674 Gist: https://gist.githubusercontent.com/jmachowinski/f487917d0539ee71b95347bf2f16f41a/raw/58c792ad266e5313e16d44ee0d1d0a12f9655843/ros2.repos BUILD args: TEST args: ROS Distro: jazzy Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15209

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

jmachowinski avatar Feb 17 '25 12:02 jmachowinski