Skip to content

fix: Prevent short time endless loop in expire_timer#1303

Merged
jmachowinski merged 2 commits intoros2:rollingfrom
cellumation:expire_fixes
Mar 23, 2026
Merged

fix: Prevent short time endless loop in expire_timer#1303
jmachowinski merged 2 commits intoros2:rollingfrom
cellumation:expire_fixes

Conversation

@jmachowinski
Copy link
Copy Markdown
Contributor

There were multiple bugs in this area:

  • The expire of goals and the re computation of the expire period had a < vs <= issue, leading to goals not expiring, while the period for the next expire was compute as 0 leading to a direct recall of the expire timer. In combination with sim time this lead to 100% CPU spikes.
  • The period of the expire timer was changed after the reset of the timer, leading to an incorrect next call time of the timer.
  • The timer was not canceled after the expire event was issued

There were multiple bugs in this area:
- The expire of goals and the re computation of the expire period
  had a < vs <= issue, leading to goals not expiring, while the
  period for the next expire was compute as 0 leading to a direct
  recall of the expire timer. In combination with sim time this lead
  to 100% CPU spikes.
- The period of the expire timer was changed after the reset of the timer,
  leading to an incorrect next call time of the timer.
- The timer was not canceled after the expire event was issued

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
@jmachowinski jmachowinski requested a review from skyegalaxy March 20, 2026 15:25
Copy link
Copy Markdown
Member

@skyegalaxy skyegalaxy left a comment

Choose a reason for hiding this comment

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

lgtm with green CI

@jmachowinski
Copy link
Copy Markdown
Contributor Author

Pulls: #1303
Gist: https://gist.githubusercontent.com/jmachowinski/2d5014c70c932362b9dc2986b5f54926/raw/0e7cf4fccee571f613df3f3965dda11c49eccc50/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/18582

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

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
@fujitatomoya
Copy link
Copy Markdown
Collaborator

windows failure is unrelated.

  • Linux Build Status

@jmachowinski jmachowinski merged commit 927a33e into ros2:rolling Mar 23, 2026
2 of 3 checks passed
@jmachowinski
Copy link
Copy Markdown
Contributor Author

jmachowinski commented Mar 23, 2026

@Mergifyio backport jazzy

@mergify
Copy link
Copy Markdown

mergify bot commented Mar 23, 2026

backport jazzy

✅ Backports have been created

Details

Cherry-pick of 927a33e has failed:

On branch mergify/bp/jazzy/pr-1303
Your branch is up to date with 'origin/jazzy'.

You are currently cherry-picking commit 927a33e.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   rcl_action/src/rcl_action/action_server.c

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

mergify bot pushed a commit that referenced this pull request Mar 23, 2026
* fix: Prevent short time endless loop in expire_timer

There were multiple bugs in this area:
- The expire of goals and the re computation of the expire period
  had a < vs <= issue, leading to goals not expiring, while the
  period for the next expire was compute as 0 leading to a direct
  recall of the expire timer. In combination with sim time this lead
  to 100% CPU spikes.
- The period of the expire timer was changed after the reset of the timer,
  leading to an incorrect next call time of the timer.
- The timer was not canceled after the expire event was issued

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

* chore: Removed unneeded line

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

---------

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com>
(cherry picked from commit 927a33e)

# Conflicts:
#	rcl_action/src/rcl_action/action_server.c
Comment on lines +115 to +116
RCUTILS_LOG_ERROR_NAMED(
ROS_PACKAGE_NAME, "_enqueue_check_expired_goals cancel of timer failed !");
Copy link
Copy Markdown
Member

@wjwwood wjwwood Mar 24, 2026

Choose a reason for hiding this comment

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

nitpick: the function name is part of the log already, see: https://docs.ros2.org/foxy/api/rcutils/structrcutils__log__location__t.html
https://docs.ros.org/en/rolling/p/rcutils/generated/structrcutils__log__location__s.html#exhale-struct-structrcutils-log-location-s

Also, we should probably avoid punctuation, exclamation, and extra whitespace:

Suggested change
RCUTILS_LOG_ERROR_NAMED(
ROS_PACKAGE_NAME, "_enqueue_check_expired_goals cancel of timer failed !");
RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, "cancel of timer failed");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

#1307 if you think it's worth changing, otherwise feel free to close that pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants