From 4aedac1e2ec1bba8b20c54cbb1e43c084694d7ad Mon Sep 17 00:00:00 2001 From: dbwls99706 Date: Thu, 21 May 2026 08:03:46 +0900 Subject: [PATCH 1/4] Revert "Fix: deadlock when calling rclpy.shutdown() from callbacks (backport #947) (#1492)" This reverts commit 8190ea05322e391f92b08d439134e1ccce6244ee. Signed-off-by: dbwls99706 --- rclpy/rclpy/executors.py | 30 +++++++----------------------- rclpy/test/test_executor.py | 33 --------------------------------- 2 files changed, 7 insertions(+), 56 deletions(-) diff --git a/rclpy/rclpy/executors.py b/rclpy/rclpy/executors.py index c0ee37328..fc15d0d25 100644 --- a/rclpy/rclpy/executors.py +++ b/rclpy/rclpy/executors.py @@ -240,14 +240,10 @@ def shutdown(self, timeout_sec: float = None) -> bool: timeot expires before all outstanding work is done. """ with self._shutdown_lock: - if self._is_shutdown: - return True - self._is_shutdown = True - # Tell executor it's been shut down - try: + if not self._is_shutdown: + self._is_shutdown = True + # Tell executor it's been shut down self._guard.trigger() - except InvalidHandle: - pass if not self._work_tracker.wait(timeout_sec): return False @@ -469,20 +465,14 @@ async def handler(entity, gc, is_shutdown, work_tracker): if is_shutdown or not entity.callback_group.beginning_execution(entity): # Didn't get the callback, or the executor has been ordered to stop entity._executor_event = False - try: - gc.trigger() - except InvalidHandle: - pass + gc.trigger() return with work_tracker: arg = take_from_wait_list(entity) # Signal that this has been 'taken' and can be added back to the wait list entity._executor_event = False - try: - gc.trigger() - except InvalidHandle: - pass + gc.trigger() try: await call_coroutine(entity, arg) @@ -490,10 +480,7 @@ async def handler(entity, gc, is_shutdown, work_tracker): entity.callback_group.ending_execution(entity) # Signal that work has been done so the next callback in a mutually exclusive # callback group can get executed - try: - gc.trigger() - except InvalidHandle: - pass + gc.trigger() task = Task( handler, (entity, self._guard, self._is_shutdown, self._work_tracker), executor=self) @@ -581,10 +568,7 @@ def _wait_for_ready_callbacks( # retrigger a guard condition that was triggered but not handled for gc in node_guards: if gc._executor_triggered: - try: - gc.trigger() - except InvalidHandle: - pass + gc.trigger() guards.append(gc) if timeout_timer is not None: timers.append(timeout_timer) diff --git a/rclpy/test/test_executor.py b/rclpy/test/test_executor.py index 1dd1b2b2c..8ff54b2b7 100644 --- a/rclpy/test/test_executor.py +++ b/rclpy/test/test_executor.py @@ -576,39 +576,6 @@ def timer2_callback(): timer1.destroy() cli.destroy() - def test_shutdown_from_callback_no_deadlock(self): - test_context = rclpy.context.Context() - rclpy.init(context=test_context) - - try: - test_node = rclpy.create_node('test_shutdown_node', context=test_context) - shutdown_called = [False] - - def timer_callback(): - shutdown_called[0] = True - rclpy.shutdown(context=test_context) - - timer = test_node.create_timer(0.1, timer_callback) - - executor = SingleThreadedExecutor(context=test_context) - executor.add_node(test_node) - - start_time = time.monotonic() - while not shutdown_called[0] and time.monotonic() - start_time < 5.0: - executor.spin_once(timeout_sec=0.1) - - self.assertTrue(shutdown_called[0], 'Timer callback was not executed') - - test_node.destroy_timer(timer) - test_node.destroy_node() - executor.shutdown() - - finally: - try: - rclpy.shutdown(context=test_context) - except Exception: - pass - if __name__ == '__main__': unittest.main() From 6a217fa032b2761ad8427093fe782d27a9861b36 Mon Sep 17 00:00:00 2001 From: Brian Date: Tue, 14 Jun 2022 12:22:53 -0700 Subject: [PATCH 2/4] Merge pull request #947 from ros2/brianc/rclpy_callback_shutdown fix rclpy.shutdown() from hanging when triggered from callback Signed-off-by: dbwls99706 --- rclpy/rclpy/executors.py | 18 ++++++++++++------ rclpy/test/test_executor.py | 20 ++++++++++++++++++++ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/rclpy/rclpy/executors.py b/rclpy/rclpy/executors.py index fc15d0d25..82340c3c4 100644 --- a/rclpy/rclpy/executors.py +++ b/rclpy/rclpy/executors.py @@ -82,7 +82,7 @@ def __exit__(self, t, v, tb): self._num_work_executing -= 1 self._work_condition.notify_all() - def wait(self, timeout_sec=None): + def wait(self, timeout_sec: Optional[float] = None): """ Wait until all work completes. @@ -90,7 +90,7 @@ def wait(self, timeout_sec=None): :type timeout_sec: float or None :rtype: bool True if all work completed """ - if timeout_sec is not None and timeout_sec < 0: + if timeout_sec is not None and timeout_sec < 0.0: timeout_sec = None # Wait for all work to complete with self._work_condition: @@ -244,9 +244,9 @@ def shutdown(self, timeout_sec: float = None) -> bool: self._is_shutdown = True # Tell executor it's been shut down self._guard.trigger() - - if not self._work_tracker.wait(timeout_sec): - return False + if not self._is_shutdown: + if not self._work_tracker.wait(timeout_sec): + return False # Clean up stuff that won't be used anymore with self._nodes_lock: @@ -480,7 +480,13 @@ async def handler(entity, gc, is_shutdown, work_tracker): entity.callback_group.ending_execution(entity) # Signal that work has been done so the next callback in a mutually exclusive # callback group can get executed - gc.trigger() + + # Catch expected error where calling executor.shutdown() + # from callback causes the GuardCondition to be destroyed + try: + gc.trigger() + except InvalidHandle: + pass task = Task( handler, (entity, self._guard, self._is_shutdown, self._work_tracker), executor=self) diff --git a/rclpy/test/test_executor.py b/rclpy/test/test_executor.py index 8ff54b2b7..6ba8b26ce 100644 --- a/rclpy/test/test_executor.py +++ b/rclpy/test/test_executor.py @@ -543,6 +543,7 @@ def timer_callback(): executor.shutdown() self.node.destroy_timer(tmr) + def test_not_lose_callback(self): self.assertIsNotNone(self.node.handle) executor = SingleThreadedExecutor(context=self.context) @@ -576,6 +577,25 @@ def timer2_callback(): timer1.destroy() cli.destroy() + def shutdown_executor_from_callback(self): + """https://github.com/ros2/rclpy/issues/944: allow for executor shutdown from callback.""" + self.assertIsNotNone(self.node.handle) + timer_period = 0.1 + executor = SingleThreadedExecutor(context=self.context) + shutdown_event = threading.Event() + + def timer_callback(): + nonlocal shutdown_event, executor + executor.shutdown() + shutdown_event.set() + + tmr = self.node.create_timer(timer_period, timer_callback) + executor.add_node(self.node) + t = threading.Thread(target=executor.spin, daemon=True) + t.start() + self.assertTrue(shutdown_event.wait(120)) + self.node.destroy_timer(tmr) + if __name__ == '__main__': unittest.main() From 96578556bdd9f0c46d2ea75b4b41f7b0b3836cae Mon Sep 17 00:00:00 2001 From: dbwls99706 Date: Thu, 21 May 2026 08:23:18 +0900 Subject: [PATCH 3/4] Fix flake8 E303 in test_executor.py The cherry-pick of #947 introduced an extra blank line between test_not_lose_callback (Humble-only) and the preceding method, because the conflict resolution kept both methods. Reduce to a single blank line to satisfy flake8 E303. Signed-off-by: dbwls99706 --- rclpy/test/test_executor.py | 1 - 1 file changed, 1 deletion(-) diff --git a/rclpy/test/test_executor.py b/rclpy/test/test_executor.py index 6ba8b26ce..af02dde69 100644 --- a/rclpy/test/test_executor.py +++ b/rclpy/test/test_executor.py @@ -543,7 +543,6 @@ def timer_callback(): executor.shutdown() self.node.destroy_timer(tmr) - def test_not_lose_callback(self): self.assertIsNotNone(self.node.handle) executor = SingleThreadedExecutor(context=self.context) From 378e39e641727c589a8369c03f90be594d316efe Mon Sep 17 00:00:00 2001 From: dbwls99706 Date: Sat, 30 May 2026 18:01:30 +0900 Subject: [PATCH 4/4] Add test_ prefix to shutdown_executor_from_callback The test name from #947 lacked the 'test_' prefix, so pytest did not discover it. The rename was applied upstream as part of #1391; cherry-picking that change here so the test actually runs on Humble. Signed-off-by: dbwls99706 --- rclpy/test/test_executor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclpy/test/test_executor.py b/rclpy/test/test_executor.py index af02dde69..0a37b3a49 100644 --- a/rclpy/test/test_executor.py +++ b/rclpy/test/test_executor.py @@ -576,7 +576,7 @@ def timer2_callback(): timer1.destroy() cli.destroy() - def shutdown_executor_from_callback(self): + def test_shutdown_executor_from_callback(self): """https://github.com/ros2/rclpy/issues/944: allow for executor shutdown from callback.""" self.assertIsNotNone(self.node.handle) timer_period = 0.1