Repository navigation
Destroy the old timer when the updater period changes - #673
Open
Randil-Fernando wants to merge 1 commit into
Open
Randil-Fernando wants to merge 1 commit into
Randil-Fernando wants to merge 1 commit into
Conversation
The period setter called self.timer.reset() and then rebound self.timer to a new timer. reset() only restarts a timer, so the old one stayed registered with the node and kept firing: rclpy's Node holds a strong reference to every timer it creates, so rebinding the attribute does not free it. Each period change therefore leaked a timer and added another caller of update() Destroy the old timer through the node instead, which removes it from the node's timer list before the replacement is created Fixes ros2/ros2#1883 Signed-off-by: Randil Fernando <randilfernando829@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes ros2/ros2#1883.
Updater.period's setter resets the old timer instead of destroying it:reset()only restarts a timer. rclpy'sNodeappends every timer it creates toself._timersand holds a strong reference, so rebindingself.timerdoesn't freethe old one — it stays registered and keeps firing. Every
periodassignment leaksa timer and adds another caller of
update().The fix destroys it through the node, which removes it from
_timersbefore thereplacement is created:
Added
test/test_updater_period.py, registered inCMakeLists.txtnext to theother pytest tests. It asserts one timer survives a period change, that it's a new
object, and that its period actually took effect. Against the old code it fails with
AssertionError: 2 != 1; with the fix it passes.The C++
Updateris not affected —reset_timer()reassigns arclcpp::TimerBase::SharedPtrand the callback group only holds a weak pointer, sothe old timer is cleaned up by RAII. This is rclpy-specific.
Reported against Jazzy, so a
backportlabel would be worth adding.