Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Fixed expiring of goals if events executor is used #2674

Merged
merged 2 commits into from
Feb 21, 2025
Merged

fix: Fixed expiring of goals if events executor is used #2674

merged 2 commits into from
Feb 21, 2025

Conversation

jmachowinski
Copy link
Contributor

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 #2661

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

@alsora @mauropasse @mjcarroll thoughts ?

@mauropasse
Copy link
Collaborator

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.

@jmachowinski
Copy link
Contributor Author

@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.

@ros-discourse
Copy link

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

@fujitatomoya
Copy link
Collaborator

@jmachowinski just checking that this is jazzy of #2699 (rolling) to keep the ABI/API, right?

@jmachowinski jmachowinski marked this pull request as draft February 4, 2025 16:02
@jmachowinski
Copy link
Contributor Author

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

@fujitatomoya
Copy link
Collaborator

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

@jmachowinski jmachowinski force-pushed the action_leak_fix_jazzy branch from 6ea3c59 to f51a729 Compare February 7, 2025 13:52
@jmachowinski jmachowinski marked this pull request as ready for review February 7, 2025 14:15
@jmachowinski
Copy link
Contributor Author

@fujitatomoya ready for review. Note, this builds up on top of the code from #2691

@jmachowinski jmachowinski force-pushed the action_leak_fix_jazzy branch from f51a729 to 7f2ad63 Compare February 7, 2025 14:33
@jmachowinski jmachowinski force-pushed the action_leak_fix_jazzy branch from 7f2ad63 to 805700c Compare February 7, 2025 19:32
fujitatomoya
fujitatomoya approved these changes Feb 12, 2025
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmachowinski thanks overall, lgtm. minor comments only.

@fujitatomoya fujitatomoya mentioned this pull request Feb 12, 2025
Open
feat: Add ClockWaiter and ClockConditionalVariable
2048aba 
Added two new synchronization primitives to perform waits
using an rclcpp::Clock.

Signed-off-by: Janosch Machowinski <[email protected]>
@jmachowinski
Copy link
Contributor Author

Pulls: #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

fix: Fixed expiring of goals if events executor is used
2de6ce4 
This commit is only relevant if the events executor is used.

This commit starts 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.

Signed-off-by: Janosch Machowinski <[email protected]>
@jmachowinski
Copy link
Contributor Author

Pulls: #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

@alsora alsora merged commit 4c239d3 into ros2:jazzy Feb 21, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alsora alsora alsora approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

@hidmic hidmic Awaiting requested review from hidmic

@wjwwood wjwwood Awaiting requested review from wjwwood

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jmachowinski @mauropasse @ros-discourse @fujitatomoya @alsora