From 0d265e22c04b500196e64bf09a959f500bef1532 Mon Sep 17 00:00:00 2001 From: Joshua Hesketh Date: Thu, 21 May 2020 15:52:39 +1000 Subject: [PATCH] Add option to delay propegating SIGINT to child process Fixes #1497 Ctrl+C sends a SIGTERM to both Tox and the running process, causing the running process to receive the signal twice once tox passes it along. This can often cancel or interfer with the process's cleanup. Instead, add an option to allow a process to suicide before sending the SIGTERM. Co-Authored-By: Stefan H. Holek --- CONTRIBUTORS | 1 + docs/changelog/1497.bugfix.rst | 1 + docs/config.rst | 23 +++++++++++++++++------ src/tox/action.py | 4 +++- src/tox/config/__init__.py | 8 ++++++++ src/tox/session/__init__.py | 3 ++- src/tox/venv.py | 1 + tests/unit/config/test_config.py | 5 ++++- 8 files changed, 37 insertions(+), 9 deletions(-) create mode 100644 docs/changelog/1497.bugfix.rst diff --git a/CONTRIBUTORS b/CONTRIBUTORS index e9488628c3..80dda65ca4 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -47,6 +47,7 @@ Johannes Christ Jon Dufresne Josh Smeaton Josh Snyder +Joshua Hesketh Julian Krause Jurko Gospodnetić Krisztian Fekete diff --git a/docs/changelog/1497.bugfix.rst b/docs/changelog/1497.bugfix.rst new file mode 100644 index 0000000000..dce157a1dc --- /dev/null +++ b/docs/changelog/1497.bugfix.rst @@ -0,0 +1 @@ +Add an option to allow a process to suicide before sending the SIGTERM. - by :user:`jhesketh` diff --git a/docs/config.rst b/docs/config.rst index 76c3a5828c..2a9ba05dbb 100644 --- a/docs/config.rst +++ b/docs/config.rst @@ -581,21 +581,32 @@ Complete list of settings that you can put into ``testenv*`` sections: via the ``-e`` tox will only run those three (even if ``coverage`` may specify as ``depends`` other targets too - such as ``py27, py35, py36, py37``). +.. conf:: suicide_timeout ^ float ^ 0.5 + + .. versionadded:: 3.15.2 + + When an interrupt is sent via Ctrl+C, the SIGINT is sent to all foreground + processes. The :conf:``suicide_timeout`` gives the running process time to + cleanup and exit before receiving (in some cases, a duplicate) SIGINT from + tox. + .. conf:: interrupt_timeout ^ float ^ 0.3 .. versionadded:: 3.15.0 - When tox is interrupted, it propagates the signal to the child process, - waits :conf:``interrupt_timeout`` seconds, and sends it a SIGTERM if it hasn't - exited. + When tox is interrupted, it propagates the signal to the child process + after :conf:``suicide_timeout`` seconds. If the process still hasn't exited + after :conf:``interrupt_timeout`` seconds, its sends a SIGTERM. .. conf:: terminate_timeout ^ float ^ 0.2 .. versionadded:: 3.15.0 - When tox is interrupted, it propagates the signal to the child process, - waits :conf:``interrupt_timeout`` seconds, sends it a SIGTERM, waits - :conf:``terminate_timeout`` seconds, and sends it a SIGKILL if it hasn't exited. + When tox is interrupted, after waiting :conf:``interrupt_timeout`` seconds, + it propagates the signal to the child process, waits + :conf:``interrupt_timeout`` seconds, sends it a SIGTERM, waits + :conf:``terminate_timeout`` seconds, and sends it a SIGKILL if it hasn't + exited. Substitutions ------------- diff --git a/src/tox/action.py b/src/tox/action.py index a7f022e2f6..74eec55aa9 100644 --- a/src/tox/action.py +++ b/src/tox/action.py @@ -32,6 +32,7 @@ def __init__( command_log, popen, python, + suicide_timeout, interrupt_timeout, terminate_timeout, ): @@ -45,6 +46,7 @@ def __init__( self.command_log = command_log self._timed_report = None self.python = python + self.suicide_timeout = suicide_timeout self.interrupt_timeout = interrupt_timeout self.terminate_timeout = terminate_timeout @@ -188,7 +190,7 @@ def evaluate_cmd(self, input_file_handler, process, redirect): def handle_interrupt(self, process): """A three level stop mechanism for children - INT -> TERM -> KILL""" msg = "from {} {{}} pid {}".format(os.getpid(), process.pid) - if process.poll() is None: + if self._wait(process, self.suicide_timeout) is None: self.info("KeyboardInterrupt", msg.format("SIGINT")) process.send_signal(signal.CTRL_C_EVENT if sys.platform == "win32" else signal.SIGINT) if self._wait(process, self.interrupt_timeout) is None: diff --git a/src/tox/config/__init__.py b/src/tox/config/__init__.py index fd3cb07f04..f4ca76d7a3 100644 --- a/src/tox/config/__init__.py +++ b/src/tox/config/__init__.py @@ -53,6 +53,7 @@ WITHIN_PROVISION = os.environ.get(str("TOX_PROVISION")) == "1" +SUICIDE_TIMEOUT = 0.5 INTERRUPT_TIMEOUT = 0.3 TERMINATE_TIMEOUT = 0.2 @@ -827,6 +828,13 @@ def develop(testenv_config, value): parser.add_testenv_attribute_obj(DepOption()) + parser.add_testenv_attribute( + name="suicide_timeout", + type="float", + default=SUICIDE_TIMEOUT, + help="timeout to allow process to exit before sending SIGINT", + ) + parser.add_testenv_attribute( name="interrupt_timeout", type="float", diff --git a/src/tox/session/__init__.py b/src/tox/session/__init__.py index 22c32bd34c..27d6310e7c 100644 --- a/src/tox/session/__init__.py +++ b/src/tox/session/__init__.py @@ -19,7 +19,7 @@ import tox from tox import reporter from tox.action import Action -from tox.config import INTERRUPT_TIMEOUT, TERMINATE_TIMEOUT, parseconfig +from tox.config import INTERRUPT_TIMEOUT, SUICIDE_TIMEOUT, TERMINATE_TIMEOUT, parseconfig from tox.config.parallel import ENV_VAR_KEY_PRIVATE as PARALLEL_ENV_VAR_KEY_PRIVATE from tox.config.parallel import OFF_VALUE as PARALLEL_OFF from tox.logs.result import ResultLog @@ -170,6 +170,7 @@ def newaction(self, name, msg, *args): self.resultlog.command_log, self.popen, sys.executable, + SUICIDE_TIMEOUT, INTERRUPT_TIMEOUT, TERMINATE_TIMEOUT, ) diff --git a/src/tox/venv.py b/src/tox/venv.py index bf91e19023..6524296be0 100644 --- a/src/tox/venv.py +++ b/src/tox/venv.py @@ -130,6 +130,7 @@ def new_action(self, msg, *args): command_log, self.popen, self.envconfig.envpython, + self.envconfig.suicide_timeout, self.envconfig.interrupt_timeout, self.envconfig.terminate_timeout, ) diff --git a/tests/unit/config/test_config.py b/tests/unit/config/test_config.py index 911be6e36b..05c1a990b6 100644 --- a/tests/unit/config/test_config.py +++ b/tests/unit/config/test_config.py @@ -175,11 +175,12 @@ def test_is_same_dep(self): assert DepOption._is_same_dep("pkg_hello-world3==1.0", "pkg_hello-world3<=2.0") assert not DepOption._is_same_dep("pkg_hello-world3==1.0", "otherpkg>=2.0") - def test_interrupt_terminate_timeout_set_manually(self, newconfig): + def test_suicide_interrupt_terminate_timeout_set_manually(self, newconfig): config = newconfig( [], """ [testenv:dev] + suicide_timeout = 30.0 interrupt_timeout = 5.0 terminate_timeout = 10.0 @@ -187,10 +188,12 @@ def test_interrupt_terminate_timeout_set_manually(self, newconfig): """, ) envconfig = config.envconfigs["other"] + assert 0.5 == envconfig.suicide_timeout assert 0.3 == envconfig.interrupt_timeout assert 0.2 == envconfig.terminate_timeout envconfig = config.envconfigs["dev"] + assert 30.0 == envconfig.suicide_timeout assert 5.0 == envconfig.interrupt_timeout assert 10.0 == envconfig.terminate_timeout