From 6b77d00c9e1c8f2c6c3da3ee5288c44172d24c3c Mon Sep 17 00:00:00 2001 From: Cian Donovan Date: Fri, 16 Jun 2023 23:38:57 +0100 Subject: [PATCH 1/7] Add default Python signal handler for all signals registered. By default, and only in certain circumstances, SIGINT is the only signal with a handler installed. All others will perform their function defined by SIG_DFL, which is to ignore is some cases, and to terminate in others. In either case, Python will never see them and be unable to pass them to the async handler. Signed-off-by: Signed-off-by: Cian Donovan --- launch/launch/utilities/signal_management.py | 1 + 1 file changed, 1 insertion(+) diff --git a/launch/launch/utilities/signal_management.py b/launch/launch/utilities/signal_management.py index a078c69d7..ee9ab1365 100644 --- a/launch/launch/utilities/signal_management.py +++ b/launch/launch/utilities/signal_management.py @@ -235,6 +235,7 @@ def handle( :return: previous handler if any, otherwise None """ signum = signal.Signals(signum) + signal.signal(signum, signal.default_int_handler) if handler is not None: if not callable(handler): raise ValueError('signal handler must be a callable') From 457159afb19927eda454a29d96a900b2e1fc4e4f Mon Sep 17 00:00:00 2001 From: Cian Donovan Date: Sat, 17 Jun 2023 01:01:15 +0100 Subject: [PATCH 2/7] Remove signal handler for SIGQUIT. SIGQUIT signals are often generated by the user hitting 'CTRL+\' in an interactive shell. However, when the terminal driver receives those characters, it doesn't just send a SIGQUIT to the running process, but to the entire foreground process group. By default, the ROS2 Client Libraries don't register a handler for SIGQUIT, as they do for SIGINT and SIGTERM, so the ROS2 node will be terminated immediately and its core dumpted on receipt. So there is little reason for ROS2 Launch to catch this signal for the purposes of forwarding it on to its children. Also, not catching SIGQUIT means that if there is a problem with the Python signal handling, then the user still has available a shortcut on their keyboards to unilaterally kill the process. Signed-off-by: Signed-off-by: Cian Donovan --- launch/launch/launch_service.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/launch/launch/launch_service.py b/launch/launch/launch_service.py index 2e1719edf..ef5e2013b 100644 --- a/launch/launch/launch_service.py +++ b/launch/launch/launch_service.py @@ -185,7 +185,7 @@ def _prepare_run_loop(self): this_task = asyncio.Task.current_task(this_loop) self.__this_task = this_task - # Setup custom signal handlers for SIGINT, SIGTERM and maybe SIGQUIT. + # Setup custom signal handlers for SIGINT & SIGTERM sigint_received = False def _on_sigint(signum): @@ -214,8 +214,6 @@ def _on_sigterm(signum): # Setup signal handlers manager.handle(signal.SIGINT, _on_sigint) manager.handle(signal.SIGTERM, _on_sigterm) - if platform.system() != 'Windows': - manager.handle(signal.SIGQUIT, _on_sigterm) # Yield asyncio loop and current task. yield this_loop, this_task finally: From 9367e655f0231c5329e836d26c8b5ba277de688a Mon Sep 17 00:00:00 2001 From: Cian Donovan Date: Sat, 17 Jun 2023 01:57:45 +0100 Subject: [PATCH 3/7] Replace individual handler functions with single generic one. Allows arbitrary signal handlers to easily be added in future without writing new functions, DRY. Also allows at least one signal of each type, printing a warning when more than one is received. This is to allow in future each signal type to be passed to the children of launch, so a user can manually escalate the kill signals. Signed-off-by: Cian Donovan --- launch/launch/launch_service.py | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/launch/launch/launch_service.py b/launch/launch/launch_service.py index ef5e2013b..078bfc73e 100644 --- a/launch/launch/launch_service.py +++ b/launch/launch/launch_service.py @@ -186,34 +186,25 @@ def _prepare_run_loop(self): self.__this_task = this_task # Setup custom signal handlers for SIGINT & SIGTERM - sigint_received = False + signals_received: dict = {} - def _on_sigint(signum): - nonlocal sigint_received - base_msg = 'user interrupted with ctrl-c (SIGINT)' - if not sigint_received: + def _on_signal(signum): + nonlocal signals_received + base_msg = 'caught ' + signal.Signals(signum).name + if signum not in signals_received or signals_received[signum] is not True: + signals_received[signum] = True self.__logger.warning(base_msg) ret = self._shutdown( - reason='ctrl-c (SIGINT)', due_to_sigint=True, force_sync=True + reason=base_msg, due_to_sigint=True, force_sync=True ) assert ret is None, ret - sigint_received = True else: self.__logger.warning('{} again, ignoring...'.format(base_msg)) - def _on_sigterm(signum): - signame = signal.Signals(signum).name - self.__logger.error( - 'user interrupted with ctrl-\\ ({}), terminating...'.format(signame)) - # TODO(wjwwood): try to terminate running subprocesses before exiting. - self.__logger.error('using {} can result in orphaned processes'.format(signame)) - self.__logger.error('make sure no processes launched are still running') - this_loop.call_soon(this_task.cancel) - with AsyncSafeSignalManager(this_loop) as manager: # Setup signal handlers - manager.handle(signal.SIGINT, _on_sigint) - manager.handle(signal.SIGTERM, _on_sigterm) + manager.handle(signal.SIGINT, _on_signal) + manager.handle(signal.SIGTERM, _on_signal) # Yield asyncio loop and current task. yield this_loop, this_task finally: From b1dfecc344e48a4770d669bf19acee01619bf2d1 Mon Sep 17 00:00:00 2001 From: Cian Donovan Date: Sun, 18 Jun 2023 21:45:44 +0100 Subject: [PATCH 4/7] Remove platform to pass code style check currently failing Signed-off-by: Cian Donovan --- launch/launch/launch_service.py | 1 - 1 file changed, 1 deletion(-) diff --git a/launch/launch/launch_service.py b/launch/launch/launch_service.py index 078bfc73e..87741ff56 100644 --- a/launch/launch/launch_service.py +++ b/launch/launch/launch_service.py @@ -18,7 +18,6 @@ import collections.abc import contextlib import logging -import platform import signal import threading import traceback From 63ebbe3db6b1c65c1e1789f142d94399eb47acae Mon Sep 17 00:00:00 2001 From: Cian Donovan Date: Sun, 18 Jun 2023 22:54:23 +0100 Subject: [PATCH 5/7] Replace test_signal_management.py __noop handler with standard default_int_handler to match how signals are now handled in Launch Signed-off-by: Cian Donovan --- launch/test/launch/utilities/test_signal_management.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/launch/test/launch/utilities/test_signal_management.py b/launch/test/launch/utilities/test_signal_management.py index d95d2a9c7..15ceffcd4 100644 --- a/launch/test/launch/utilities/test_signal_management.py +++ b/launch/test/launch/utilities/test_signal_management.py @@ -25,19 +25,19 @@ def cap_signals(*signals): - def _noop(*args): - pass - def _decorator(func): @functools.wraps(func) def _wrapper(*args, **kwargs): handlers = {} try: for s in signals: - handlers[s] = signal.signal(s, _noop) + handlers[s] = signal.signal(s, signal.default_int_handler) return func(*args, **kwargs) + except KeyboardInterrupt: + pass finally: - assert all(signal.signal(s, h) is _noop for s, h in handlers.items()) + assert all(signal.signal(s, h) is signal.default_int_handler + for s, h in handlers.items()) return _wrapper return _decorator From 8bd220adcf35475c34a5b8dfc12fc6e5ba88312c Mon Sep 17 00:00:00 2001 From: Cian Donovan Date: Thu, 2 May 2024 12:54:36 +0100 Subject: [PATCH 6/7] Set due_to_sigint correctly send_sigint? True : False +----------------+--------+---------+ | | SIGINT | SIGTERM | +----------------+--------+---------+ | interactive | False | True | | noninteractive | True | True | +----------------+--------+---------+ Launch either detects, through checking if there's a controlling terminal, or the value of the flag `--noninteractive`, to determine if launch was launched interactively or not. If it was launched interactively, then the receipt of SIGINT is implicitly assumed to have come from the user pressing Ctrl-C and their terminal sending a SIGINT to the entire foreground process group. In such case, it is redundant to also send SIGINT from launch to the child ROS nodes, however it should be noted that even if another signal was sent there would be no ill-effects. In all other cases, assume the child nodes were not themselves signalled, so manually send SIGINT, and later escalate to SIGTERM after timeout, to the nodes. Signed-off-by: Cian Donovan --- launch/launch/launch_service.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/launch/launch/launch_service.py b/launch/launch/launch_service.py index 87741ff56..09e26872b 100644 --- a/launch/launch/launch_service.py +++ b/launch/launch/launch_service.py @@ -193,8 +193,9 @@ def _on_signal(signum): if signum not in signals_received or signals_received[signum] is not True: signals_received[signum] = True self.__logger.warning(base_msg) + due_to_sigint = True if signal.Signals(signum).name == 'SIGINT' else False ret = self._shutdown( - reason=base_msg, due_to_sigint=True, force_sync=True + reason=base_msg, due_to_sigint=due_to_sigint, force_sync=True ) assert ret is None, ret else: From 213046df5054007c58387af23a009ba3b86a5f31 Mon Sep 17 00:00:00 2001 From: Cian Donovan Date: Fri, 3 May 2024 18:44:44 +0100 Subject: [PATCH 7/7] Use `set` over `dict` for `signals_received` store. Set is unordered and nonduplicative, better fits storing whether at least one of each unique signal has been received. Resolve `signum` into the signal's name as a string once instead of twice. Cleanup `due_to_sigint` ternary assignment. Signed-off-by: Cian Donovan --- launch/launch/launch_service.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/launch/launch/launch_service.py b/launch/launch/launch_service.py index 09e26872b..a25672079 100644 --- a/launch/launch/launch_service.py +++ b/launch/launch/launch_service.py @@ -185,15 +185,16 @@ def _prepare_run_loop(self): self.__this_task = this_task # Setup custom signal handlers for SIGINT & SIGTERM - signals_received: dict = {} + signals_received = set() def _on_signal(signum): nonlocal signals_received - base_msg = 'caught ' + signal.Signals(signum).name - if signum not in signals_received or signals_received[signum] is not True: - signals_received[signum] = True + signal_name = signal.Signals(signum).name + base_msg = 'caught ' + signal_name + if signal_name not in signals_received: + signals_received.add(signal_name) + due_to_sigint = True if signal_name == 'SIGINT' else False self.__logger.warning(base_msg) - due_to_sigint = True if signal.Signals(signum).name == 'SIGINT' else False ret = self._shutdown( reason=base_msg, due_to_sigint=due_to_sigint, force_sync=True )