From 0f41a1f4a1a6828613874d32c61208d6871a99b6 Mon Sep 17 00:00:00 2001 From: Randil Fernando Date: Wed, 14 Oct 2026 11:34:27 +0530 Subject: [PATCH] Destroy the old timer when the updater period changes 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 --- diagnostic_updater/CMakeLists.txt | 1 + .../diagnostic_updater/_diagnostic_updater.py | 2 +- .../test/test_updater_period.py | 35 +++++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 diagnostic_updater/test/test_updater_period.py diff --git a/diagnostic_updater/CMakeLists.txt b/diagnostic_updater/CMakeLists.txt index 46c2ecc66..f7ab56a78 100644 --- a/diagnostic_updater/CMakeLists.txt +++ b/diagnostic_updater/CMakeLists.txt @@ -96,6 +96,7 @@ if(BUILD_TESTING) find_package(ament_cmake_pytest REQUIRED) ament_add_pytest_test(diagnostic_updater_test.py "test/diagnostic_updater_test.py") ament_add_pytest_test(test_DiagnosticStatusWrapper.py "test/test_diagnostic_status_wrapper.py") + ament_add_pytest_test(test_updater_period.py "test/test_updater_period.py") # ament_add_pytest_test(status_msg_test.py "test/status_msg_test.py") endif() diff --git a/diagnostic_updater/diagnostic_updater/_diagnostic_updater.py b/diagnostic_updater/diagnostic_updater/_diagnostic_updater.py index e05ba6de1..067efd7bf 100644 --- a/diagnostic_updater/diagnostic_updater/_diagnostic_updater.py +++ b/diagnostic_updater/diagnostic_updater/_diagnostic_updater.py @@ -313,7 +313,7 @@ def period(self): @period.setter def period(self, period): self.__period = period - self.timer.reset() + self.node.destroy_timer(self.timer) self.timer = self.node.create_timer(self.__period, self.update) def force_update(self): diff --git a/diagnostic_updater/test/test_updater_period.py b/diagnostic_updater/test/test_updater_period.py new file mode 100644 index 000000000..25e6e6eca --- /dev/null +++ b/diagnostic_updater/test/test_updater_period.py @@ -0,0 +1,35 @@ +#!/usr/bin/env python +# -*- coding: utf-8 -*- + +"""Tests for the Updater period setter.""" + +import unittest + +from diagnostic_updater import Updater + +import rclpy + + +class TestUpdaterPeriod(unittest.TestCase): + + def setUp(self): + rclpy.init() + self.node = rclpy.create_node('test_updater_period') + + def tearDown(self): + self.node.destroy_node() + rclpy.shutdown() + + def test_period_setter_replaces_timer(self): + updater = Updater(self.node, period=1.0) + self.assertEqual(len(list(self.node.timers)), 1) + old_timer = updater.timer + + updater.period = 0.5 + + self.assertEqual(updater.period, 0.5) + # The old timer must be destroyed rather than left running next to + # the new one, otherwise update() gets called by both of them. + self.assertEqual(len(list(self.node.timers)), 1) + self.assertIsNot(updater.timer, old_timer) + self.assertEqual(updater.timer.timer_period_ns, 500000000)