From 5999f601866bca64c5ceca9cd37c14710c033017 Mon Sep 17 00:00:00 2001 From: mahendrapaipuri Date: Tue, 27 Jun 2023 15:44:18 +0200 Subject: [PATCH 01/10] Take existing signal handlers into account Ensure that we do not override the existing signal handlers registered in Python --- simpervisor/atexitasync.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/simpervisor/atexitasync.py b/simpervisor/atexitasync.py index 65aa954..3840aed 100644 --- a/simpervisor/atexitasync.py +++ b/simpervisor/atexitasync.py @@ -7,6 +7,7 @@ import sys _handlers = [] +_existing_handlers = {} signal_handler_set = False @@ -14,6 +15,14 @@ def add_handler(handler): global signal_handler_set if not signal_handler_set: + # First get existing signal handlers for SIGINT and SIGTERM + # If no handlers are defined in Python, getsignal will return None + _existing_handlers.update( + { + signal.SIGINT: signal.getsignal(signal.SIGINT), + signal.SIGTERM: signal.getsignal(signal.SIGTERM), + } + ) signal.signal(signal.SIGINT, _handle_signal) signal.signal(signal.SIGTERM, _handle_signal) signal_handler_set = True @@ -31,4 +40,9 @@ def _handle_signal(signum, *args): signum = signal.CTRL_C_EVENT for handler in _handlers: handler(signum) - sys.exit(0) + # Finally execute any existing handler that were registered in Python + existing_handler = _existing_handlers.get(signum, None) + if existing_handler is not None: + existing_handler(signum, None) + else: + sys.exit(0) From b52fd8d6a4392673dc67b6b75574967dcb8d6a05 Mon Sep 17 00:00:00 2001 From: mahendrapaipuri Date: Wed, 28 Jun 2023 12:01:26 +0200 Subject: [PATCH 02/10] Ensure existing handlers are not default ones We check if the existing handlers for SIGINT and SIGTERM are different from default handlers. Only when they are custom handlers, we execute them in our custom handler --- simpervisor/atexitasync.py | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/simpervisor/atexitasync.py b/simpervisor/atexitasync.py index 3840aed..625ef76 100644 --- a/simpervisor/atexitasync.py +++ b/simpervisor/atexitasync.py @@ -15,18 +15,31 @@ def add_handler(handler): global signal_handler_set if not signal_handler_set: - # First get existing signal handlers for SIGINT and SIGTERM - # If no handlers are defined in Python, getsignal will return None + # Add handler of SIGINT only if it is not default + _existing_sigint_handler = signal.getsignal(signal.SIGINT) + if _existing_sigint_handler.__qualname__ == 'default_int_handler': + _existing_sigint_handler = None _existing_handlers.update( { - signal.SIGINT: signal.getsignal(signal.SIGINT), - signal.SIGTERM: signal.getsignal(signal.SIGTERM), + signal.SIGINT: _existing_sigint_handler, } ) + + # Add handler of SIGTERM only if it is not default + _existing_sigterm_handler = signal.getsignal(signal.SIGTERM) + if _existing_sigterm_handler == signal.Handlers.SIG_DFL: + _existing_sigterm_handler = None + _existing_handlers.update( + { + signal.SIGTERM: _existing_sigterm_handler, + } + ) + signal.signal(signal.SIGINT, _handle_signal) signal.signal(signal.SIGTERM, _handle_signal) signal_handler_set = True _handlers.append(handler) + # print(_existing_handlers) def remove_handler(handler): @@ -41,8 +54,9 @@ def _handle_signal(signum, *args): for handler in _handlers: handler(signum) # Finally execute any existing handler that were registered in Python + # Bail if existing handler is not a callable existing_handler = _existing_handlers.get(signum, None) - if existing_handler is not None: + if existing_handler is not None and callable(existing_handler): existing_handler(signum, None) else: sys.exit(0) From 7e17032d9f9d635edeb67ab6bf6e809d3435e636 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 28 Jun 2023 10:02:27 +0000 Subject: [PATCH 03/10] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- simpervisor/atexitasync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/simpervisor/atexitasync.py b/simpervisor/atexitasync.py index 625ef76..7f63359 100644 --- a/simpervisor/atexitasync.py +++ b/simpervisor/atexitasync.py @@ -17,7 +17,7 @@ def add_handler(handler): if not signal_handler_set: # Add handler of SIGINT only if it is not default _existing_sigint_handler = signal.getsignal(signal.SIGINT) - if _existing_sigint_handler.__qualname__ == 'default_int_handler': + if _existing_sigint_handler.__qualname__ == "default_int_handler": _existing_sigint_handler = None _existing_handlers.update( { From 7b3dd3048f71edf41127db628ff15539a99614fa Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 30 Jan 2024 09:07:30 +0100 Subject: [PATCH 04/10] Cleanup commented out print statement --- simpervisor/atexitasync.py | 1 - 1 file changed, 1 deletion(-) diff --git a/simpervisor/atexitasync.py b/simpervisor/atexitasync.py index 7f63359..57df49f 100644 --- a/simpervisor/atexitasync.py +++ b/simpervisor/atexitasync.py @@ -39,7 +39,6 @@ def add_handler(handler): signal.signal(signal.SIGTERM, _handle_signal) signal_handler_set = True _handlers.append(handler) - # print(_existing_handlers) def remove_handler(handler): From 15304ee0c91dc83408ee3d5cc2dcc5d5fed690e3 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 30 Jan 2024 09:33:13 +0100 Subject: [PATCH 05/10] Refactor for readability --- simpervisor/atexitasync.py | 70 ++++++++++++++++++++++---------------- 1 file changed, 40 insertions(+), 30 deletions(-) diff --git a/simpervisor/atexitasync.py b/simpervisor/atexitasync.py index 57df49f..76af8d6 100644 --- a/simpervisor/atexitasync.py +++ b/simpervisor/atexitasync.py @@ -7,55 +7,65 @@ import sys _handlers = [] -_existing_handlers = {} +_prev_handlers = {} signal_handler_set = False def add_handler(handler): + """ + Adds a signal handler function that will be called when the Python process + receives either a SIGINT (on windows CTRL_C_EVENT) or SIGTERM signal. + """ + _ensure_signal_handlers_set() + _handlers.append(handler) + + +def remove_handler(handler): + """Removes previously added signal handler.""" + _handlers.remove(handler) + + +def _ensure_signal_handlers_set(): + """ + Ensures _handle_signal is registered as a top level signal handler for + SIGINT and SIGTERM and saves previously registered non-default Python + callable signal handlers. + """ global signal_handler_set if not signal_handler_set: - # Add handler of SIGINT only if it is not default - _existing_sigint_handler = signal.getsignal(signal.SIGINT) - if _existing_sigint_handler.__qualname__ == "default_int_handler": - _existing_sigint_handler = None - _existing_handlers.update( - { - signal.SIGINT: _existing_sigint_handler, - } - ) - - # Add handler of SIGTERM only if it is not default - _existing_sigterm_handler = signal.getsignal(signal.SIGTERM) - if _existing_sigterm_handler == signal.Handlers.SIG_DFL: - _existing_sigterm_handler = None - _existing_handlers.update( - { - signal.SIGTERM: _existing_sigterm_handler, - } - ) + # save previously registered non-default Python callable signal handlers + prev_sigint = signal.getsignal(signal.SIGINT) + prev_sigterm = signal.getsignal(signal.SIGTERM) + if callable(prev_sigint) and prev_sigint.__qualname__ != "default_int_handler": + _prev_handlers[signal.SIGINT] = prev_sigint + if callable(prev_sigterm) and prev_sigterm != signal.Handlers.SIG_DFL: + _prev_handlers[signal.SIGTERM] = prev_sigint + # let _handle_signal handle SIGINT and SIGTERM signal.signal(signal.SIGINT, _handle_signal) signal.signal(signal.SIGTERM, _handle_signal) signal_handler_set = True - _handlers.append(handler) - - -def remove_handler(handler): - _handlers.remove(handler) def _handle_signal(signum, *args): + """ + Calls functions added by add_handler, and then calls the previously + registered non-default Python callable signal handler if there were one. + """ # Windows doesn't support SIGINT. Replacing it with CTRL_C_EVENT so that it # can used with subprocess.Popen.send_signal if signum == signal.SIGINT and sys.platform == "win32": signum = signal.CTRL_C_EVENT + for handler in _handlers: handler(signum) - # Finally execute any existing handler that were registered in Python - # Bail if existing handler is not a callable - existing_handler = _existing_handlers.get(signum, None) - if existing_handler is not None and callable(existing_handler): - existing_handler(signum, None) + + # call previously registered non-default Python callable handler or exit + # FIXME: This is currently incorrectly implemented for windows as there are + # no _prev_handlers entry for signal.CTRL_C_EVENT! + prev_handler = _prev_handlers.get(signum) + if prev_handler: + prev_handler(signum, None) else: sys.exit(0) From 0c1be62ca5e77e3c381ef04b8f3742fce9550a2c Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 30 Jan 2024 12:34:19 +0100 Subject: [PATCH 06/10] Fix calling of previous signal handlers for windows --- simpervisor/atexitasync.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/simpervisor/atexitasync.py b/simpervisor/atexitasync.py index 76af8d6..ff6dcb9 100644 --- a/simpervisor/atexitasync.py +++ b/simpervisor/atexitasync.py @@ -35,6 +35,12 @@ def _ensure_signal_handlers_set(): global signal_handler_set if not signal_handler_set: # save previously registered non-default Python callable signal handlers + # + # windows note: signal.getsignal(signal.CTRL_C_EVENT) would error with + # "ValueError: signal number out of range", and + # signal.signal(signal.CTRL_C_EVENT, _handle_signal) would error with + # "ValueError: invalid signal value". + # prev_sigint = signal.getsignal(signal.SIGINT) prev_sigterm = signal.getsignal(signal.SIGTERM) if callable(prev_sigint) and prev_sigint.__qualname__ != "default_int_handler": @@ -53,6 +59,8 @@ def _handle_signal(signum, *args): Calls functions added by add_handler, and then calls the previously registered non-default Python callable signal handler if there were one. """ + prev_handler = _prev_handlers.get(signum) + # Windows doesn't support SIGINT. Replacing it with CTRL_C_EVENT so that it # can used with subprocess.Popen.send_signal if signum == signal.SIGINT and sys.platform == "win32": @@ -62,9 +70,6 @@ def _handle_signal(signum, *args): handler(signum) # call previously registered non-default Python callable handler or exit - # FIXME: This is currently incorrectly implemented for windows as there are - # no _prev_handlers entry for signal.CTRL_C_EVENT! - prev_handler = _prev_handlers.get(signum) if prev_handler: prev_handler(signum, None) else: From 4d2e774a4ce0ef62c127a239325a5961b8733a8c Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 30 Jan 2024 12:44:09 +0100 Subject: [PATCH 07/10] Pass args to previous registered signal handlers I'm not confident this is right, but not confident on the passing None either. What is correct here? --- simpervisor/atexitasync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/simpervisor/atexitasync.py b/simpervisor/atexitasync.py index ff6dcb9..ebb047d 100644 --- a/simpervisor/atexitasync.py +++ b/simpervisor/atexitasync.py @@ -71,6 +71,6 @@ def _handle_signal(signum, *args): # call previously registered non-default Python callable handler or exit if prev_handler: - prev_handler(signum, None) + prev_handler(signum, *args) else: sys.exit(0) From 3461767a6b02947e6f1b7e5ca8f67ad825d57020 Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Tue, 30 Jan 2024 17:27:02 +0100 Subject: [PATCH 08/10] Fix critical typo --- simpervisor/atexitasync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/simpervisor/atexitasync.py b/simpervisor/atexitasync.py index ebb047d..d4869cb 100644 --- a/simpervisor/atexitasync.py +++ b/simpervisor/atexitasync.py @@ -46,7 +46,7 @@ def _ensure_signal_handlers_set(): if callable(prev_sigint) and prev_sigint.__qualname__ != "default_int_handler": _prev_handlers[signal.SIGINT] = prev_sigint if callable(prev_sigterm) and prev_sigterm != signal.Handlers.SIG_DFL: - _prev_handlers[signal.SIGTERM] = prev_sigint + _prev_handlers[signal.SIGTERM] = prev_sigterm # let _handle_signal handle SIGINT and SIGTERM signal.signal(signal.SIGINT, _handle_signal) From e1265cd588244a59cb9b5197fac47fcad4e6e01b Mon Sep 17 00:00:00 2001 From: Mahendra Paipuri Date: Wed, 31 Jan 2024 10:54:08 +0100 Subject: [PATCH 09/10] test: Add a unit test for non default handlers * Test will check if registered non default handlers are called * Cast port number to int in simplehttpserver.py script Signed-off-by: Mahendra Paipuri --- tests/child_scripts/signalprinter.py | 12 +++++++ tests/child_scripts/simplehttpserver.py | 2 +- tests/test_atexitasync.py | 45 +++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 1 deletion(-) diff --git a/tests/child_scripts/signalprinter.py b/tests/child_scripts/signalprinter.py index 61b583c..e6fe7af 100644 --- a/tests/child_scripts/signalprinter.py +++ b/tests/child_scripts/signalprinter.py @@ -3,17 +3,29 @@ """ import asyncio import sys +import signal from functools import partial from simpervisor.atexitasync import add_handler +def _non_default_handle(sig, frame): + # Print the received signum and then exit + print(f"non default handler received {sig}", flush=True) + sys.exit(0) + + def _handle_sigterm(number, received_signum): # Print the received signum so we know our handler was called print(f"handler {number} received", int(received_signum), flush=True) handlercount = int(sys.argv[1]) +# Add non default handler if arg true is passed +if len(sys.argv) == 3: + if bool(sys.argv[2]): + signal.signal(signal.SIGINT, _non_default_handle) + signal.signal(signal.SIGTERM, _non_default_handle) for i in range(handlercount): add_handler(partial(_handle_sigterm, i)) diff --git a/tests/child_scripts/simplehttpserver.py b/tests/child_scripts/simplehttpserver.py index cc460ff..44668cc 100644 --- a/tests/child_scripts/simplehttpserver.py +++ b/tests/child_scripts/simplehttpserver.py @@ -23,4 +23,4 @@ async def hello(request): app = web.Application() app.add_routes(routes) -web.run_app(app, port=PORT) +web.run_app(app, port=int(PORT)) diff --git a/tests/test_atexitasync.py b/tests/test_atexitasync.py index 94e2fe1..866c6d2 100644 --- a/tests/test_atexitasync.py +++ b/tests/test_atexitasync.py @@ -49,3 +49,48 @@ def test_atexitasync(signum, handlercount): # The code should exit cleanly retcode = proc.wait() assert retcode == 0 + + +@pytest.mark.parametrize( + "signum, handlercount", + [ + (signal.SIGTERM, 1), + (signal.SIGINT, 1), + (signal.SIGTERM, 5), + (signal.SIGINT, 5), + ], +) +@pytest.mark.skipif( + sys.platform == "win32", + reason="Testing signals on Windows doesn't seem to be possible", +) +def test_atexitasync_with_non_default_handlers(signum, handlercount): + """ + Test signal handlers receive signals properly and handler existing default handlers + correctly + """ + signalprinter_file = os.path.join( + os.path.dirname(os.path.abspath(__file__)), "child_scripts", "signalprinter.py" + ) + proc = subprocess.Popen( + [sys.executable, signalprinter_file, str(handlercount), "true"], + stdout=subprocess.PIPE, + text=True, + ) + + # Give the process time to register signal handlers + time.sleep(1) + proc.send_signal(signum) + + # Make sure the signal is handled by our handler in the code + stdout, stderr = proc.communicate() + expected_output = ( + "\n".join([f"handler {i} received {signum}" for i in range(handlercount)]) + + f"\nnon default handler received {signum}\n" + ) + + assert stdout == expected_output + + # The code should exit cleanly + retcode = proc.wait() + assert retcode == 0 From 0788834cd6e15dffed3c7859cc94efa7506e4ed6 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 31 Jan 2024 09:54:23 +0000 Subject: [PATCH 10/10] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/child_scripts/signalprinter.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/child_scripts/signalprinter.py b/tests/child_scripts/signalprinter.py index e6fe7af..480f048 100644 --- a/tests/child_scripts/signalprinter.py +++ b/tests/child_scripts/signalprinter.py @@ -2,8 +2,8 @@ Print received SIGTERM & SIGINT signals """ import asyncio -import sys import signal +import sys from functools import partial from simpervisor.atexitasync import add_handler