Timers will cancel on shutdown

launch/launch/actions/execute_process.py

@@ -308,27 +308,35 @@ def printer(context, msg, timeout_substitutions):
             ('float(', *self.__sigterm_timeout, ') + float(', *self.__sigkill_timeout, ')')
         )]
         # Setup a timer to send us a SIGTERM if we don't shutdown quickly.
-        self.__sigterm_timer = TimerAction(period=sigterm_timeout, actions=[
-            OpaqueFunction(
-                function=printer,
-                args=(base_msg.format('{}', '{}', 'SIGINT', 'SIGTERM'), sigterm_timeout)
-            ),
-            EmitEvent(event=SignalProcess(
-                signal_number=signal.SIGTERM,
-                process_matcher=matches_action(self)
-            )),
-        ])
+        self.__sigterm_timer = TimerAction(
+            period=sigterm_timeout,
+            actions=[
+                OpaqueFunction(
+                    function=printer,
+                    args=(base_msg.format('{}', '{}', 'SIGINT', 'SIGTERM'), sigterm_timeout)
+                ),
+                EmitEvent(event=SignalProcess(
+                    signal_number=signal.SIGTERM,
+                    process_matcher=matches_action(self)
+                )),
+            ],
+            cancel_on_shutdown=False,
+        )
         # Setup a timer to send us a SIGKILL if we don't shutdown after SIGTERM.
-        self.__sigkill_timer = TimerAction(period=sigkill_timeout, actions=[
-            OpaqueFunction(
-                function=printer,
-                args=(base_msg.format('{}', '{}', 'SIGTERM', 'SIGKILL'), sigkill_timeout)
-            ),
-            EmitEvent(event=SignalProcess(
-                signal_number='SIGKILL',
-                process_matcher=matches_action(self)
-            ))
-        ])
+        self.__sigkill_timer = TimerAction(
+            period=sigkill_timeout,
+            actions=[
+                OpaqueFunction(
+                    function=printer,
+                    args=(base_msg.format('{}', '{}', 'SIGTERM', 'SIGKILL'), sigkill_timeout)
+                ),
+                EmitEvent(event=SignalProcess(
+                    signal_number='SIGKILL',
+                    process_matcher=matches_action(self)
+                ))
+            ],
+            cancel_on_shutdown=False,
+        )
         return [
             cast(Action, self.__sigterm_timer),
             cast(Action, self.__sigkill_timer),

launch/launch/actions/timer_action.py

@@ -29,6 +29,7 @@
 
 from ..action import Action
 from ..event_handler import EventHandler
+from ..events import Shutdown
 from ..events import TimerEvent
 from ..launch_context import LaunchContext
 from ..launch_description_entity import LaunchDescriptionEntity
@@ -40,6 +41,7 @@
 from ..utilities import is_a_subclass
 from ..utilities import normalize_to_list_of_substitutions
 from ..utilities import perform_substitutions
+from .opaque_function import OpaqueFunction
 
 _logger = logging.getLogger('launch.timer_action')
 
@@ -56,6 +58,7 @@ def __init__(
         *,
         period: Union[float, SomeSubstitutionsType],
         actions: Iterable[LaunchDescriptionEntity],
+        cancel_on_shutdown: bool = True,
         **kwargs
     ) -> None:
         """Constructor."""
@@ -72,6 +75,7 @@ def __init__(
         self.__completed_future = None  # type: Optional[asyncio.Future]
         self.__canceled = False
         self.__canceled_future = None  # type: Optional[asyncio.Future]
+        self.__cancel_on_shutdown = cancel_on_shutdown
 
     async def __wait_to_fire_event(self, context):
         done, pending = await asyncio.wait(
@@ -136,7 +140,6 @@ def execute(self, context: LaunchContext) -> Optional[List['Action']]:
 
         # Once per context, install the general purpose OnTimerEvent event handler.
         if not hasattr(context, '_TimerAction__event_handler_has_been_installed'):
-            from ..actions import OpaqueFunction
             context.register_event_handler(EventHandler(
                 matcher=lambda event: is_a_subclass(event, TimerEvent),
                 entities=OpaqueFunction(
@@ -150,6 +153,17 @@ def execute(self, context: LaunchContext) -> Optional[List['Action']]:
         # Capture the current context locals so the yielded actions can make use of them too.
         self.__context_locals = dict(context.get_locals_as_dict())  # Capture a copy
         context.asyncio_loop.create_task(self.__wait_to_fire_event(context))
+
+        # By default, the 'shutdown' event will cause timers to cancel so they don't hold up the
+        # launch process
+        if self.__cancel_on_shutdown:
+            context.register_event_handler(
+                EventHandler(
+                    matcher=lambda event: is_a_subclass(event, Shutdown),
+                    entities=OpaqueFunction(function=lambda context: self.cancel())
+                )
+            )
+
         return None
 
     def get_asyncio_future(self) -> Optional[asyncio.Future]:

launch/test/launch/test_timer_action.py

@@ -13,8 +13,12 @@
 # limitations under the License.
 
 
+"""Tests for the TimerAction Action."""
+import sys
+
 import launch
 import launch.actions
+import launch.event_handlers
 
 
 def test_multiple_launch_with_timers():
@@ -24,6 +28,11 @@ def test_multiple_launch_with_timers():
 
     def generate_launch_description():
         return launch.LaunchDescription([
+
+            launch.actions.ExecuteProcess(
+                cmd=[sys.executable, '-c', 'while True: pass'],
+            ),
+
             launch.actions.TimerAction(
                 period='1',
                 actions=[
@@ -34,9 +43,113 @@ def generate_launch_description():
 
     ls = launch.LaunchService()
     ls.include_launch_description(generate_launch_description())
-    assert 0 == ls.run(shutdown_when_idle=False)  # Always works
+    assert 0 == ls.run()  # Always works
 
     ls = launch.LaunchService()
     ls.include_launch_description(generate_launch_description())
     # Next line hangs forever before https://github.com/ros2/launch/issues/183 was fixed.
-    assert 0 == ls.run(shutdown_when_idle=False)
+    assert 0 == ls.run()
+
+
+def _shutdown_listener_factory(reasons_arr):
+    return launch.actions.RegisterEventHandler(
+        launch.event_handlers.OnShutdown(
+            on_shutdown=lambda event, context: reasons_arr.append(event)
+        )
+    )
+
+
+def test_timer_action_sanity_check():
+    """Test that timer actions work (sanity check)."""
+    # This test is structured like test_shutdown_preempts_timers and
+    # test_timer_can_block_preemption as a sanity check that the shutdown listener
+    # and other launch related infrastructure works as expected
+    shutdown_reasons = []
+
+    ld = launch.LaunchDescription([
+        launch.actions.ExecuteProcess(
+            cmd=[sys.executable, '-c', 'while True: pass'],
+        ),
+
+        launch.actions.TimerAction(
+            period='1',
+            actions=[
+                launch.actions.Shutdown(reason='One second timeout')
+            ]
+        ),
+
+        _shutdown_listener_factory(shutdown_reasons),
+    ])
+
+    ls = launch.LaunchService()
+    ls.include_launch_description(ld)
+    assert 0 == ls.run()
+    assert shutdown_reasons[0].reason == 'One second timeout'
+
+
+def test_shutdown_preempts_timers():
+    shutdown_reasons = []
+
+    ld = launch.LaunchDescription([
+
+        launch.actions.ExecuteProcess(
+            cmd=[sys.executable, '-c', 'while True: pass'],
+        ),
+
+        launch.actions.TimerAction(
+            period='1',
+            actions=[
+                launch.actions.Shutdown(reason='fast shutdown')
+            ]
+        ),
+
+        launch.actions.TimerAction(
+            period='2',
+            actions=[
+                launch.actions.Shutdown(reason='slow shutdown')
+            ]
+        ),
+
+        _shutdown_listener_factory(shutdown_reasons),
+    ])
+
+    ls = launch.LaunchService()
+    ls.include_launch_description(ld)
+    assert 0 == ls.run()
+    assert len(shutdown_reasons) == 1
+    assert shutdown_reasons[0].reason == 'fast shutdown'
+
+
+def test_timer_can_block_preemption():
+    shutdown_reasons = []
+
+    ld = launch.LaunchDescription([
+
+        launch.actions.ExecuteProcess(
+            cmd=[sys.executable, '-c', 'while True: pass'],
+        ),
+
+        launch.actions.TimerAction(
+            period='1',
+            actions=[
+                launch.actions.Shutdown(reason='fast shutdown')
+            ]
+        ),
+
+        launch.actions.TimerAction(
+            period="2",
+            actions=[
+                launch.actions.Shutdown(reason='slow shutdown')
+            ],
+            cancel_on_shutdown=False  # Preempted in test_shutdown_preempts_timers, but not here
+        ),
+
+        _shutdown_listener_factory(shutdown_reasons),
+    ])
+
+    ls = launch.LaunchService()
+    ls.include_launch_description(ld)
+    assert 0 == ls.run()
+    assert len(shutdown_reasons) == 2  # Should see 'shutdown' event twice because
+    assert shutdown_reasons[0].reason == 'fast shutdown'
+    assert shutdown_reasons[1].reason == 'slow shutdown'