Repository: theacodes/nox
Pull request #565 (2022-02-01 22:13:16)

feat: allow configurable child shutdown timeouts
Thank you  for the work done in https://github.com/theacodes/nox/pull/393! It did fix some odd issues but brought to light one more. We have a few tests that spin up docker containers and then clean them up after the pytest session exits but these take upwards of 10-15 seconds to fully clean up. I think that making the timeout configurable may be acceptable although I'm not sure if this is the best approach. Let me know what you think.

The goal of this change is to allow you to set the timeout on the CLI, in the `noxfile.py`, or individually on each `session.run` invocation. If this is acceptable I'll work on updating docs and tests. Thanks!

raddessi: Thanks for the feedback!

> Generally, it's preferred to open an issue first.

I'm sorry, I would if I had known. I don't think there is a PR template on this project is there? I'll maybe open an issue about adding one of those with some guidelines then. 

> Have you considered using pytest fixtures for your Docker containers?

Yes, actually that is what is going on :) Pytest spins up and down containers, but when it tries to shut them down after receiving SIGINT noxs end up SIGKILLing pytest and we get containers left around on out CI hosts. This has become apparent after I switched the project in question over to a matrix strategy build on github so after the first test failure a SIGTERM gets sent to the other running matrix tests as I'm sure you know.

> While you can always argue for more configurability, every little knob we add increases maintainance effort permanently, so I'd like to first consider alternative solutions.

I 100% understand and agree. If there is a workaround I'm willing to implement it, but I think this will probably effect more than a few other users of nox unless I happen to be doing things in a strange way (which I hope I'm not)

> Also, have you considered modifying the Docker containers to improve their shutdown time?

I'm not really sure what all I could speed up, I think it's just a problem of combined IO load on the CI hosts at some points. It can be as fast as 1-2 seconds to clean it up sometimes but it's always longer than 0.3 seconds. Docker-compose with databases always seems a little sluggish to me on cleanup for some reason.

> The option help says that it's for the delay between SIGTERM and SIGKILL. But the implementation is for the delay between SIGINT and SIGTERM. Possible confusion?

Oh you're totally right, I think in this case the configurable timeout should be moved to the second one? I was reading the code wrong.

> There are two timeouts involved in the existing shutdown process, the delay before SIGTERM and the delay before SIGKILL. In tox 3, both are configurable, and have names that make their purpose clear (interrupt_timeout and terminate_timeout). You chose the generic name shutdown_timeout, and wired that to the interrupt timeout. Why did you select this one of the two timeouts, and how should we handle future feature requests for configuring the other timeout?

Good to know about tox, I should have looked there for naming standards to follow. In this case because tox has the functionality already then I would personally want to match tox's naming and probably provide both as options to the user. If you would only want one, this should probably be named `terminate_timeout` and have control over the timeout before the SIGKILL do you agree?

---

I should have set this to draft mode to begin with, I didn't mean for you to accept this without a discussion :) Thanks again

raddessi: I do see you have a CONTRIBUTING file however, I apologize I should have looked for one.
raddessi: Should I start a related Issue for this?
cjolowicz: Thanks for explaining your use case. And no worries about opening an issue now, I think we can take it from here.

What are your thoughts about adding parameters `interrupt_timeout` and `terminate_timeout` to `session.run`, and omitting the CLI options? I feel that the command-line interface isn't achieving much here, in addition to the parameters.

We'll also need docs and a test. For the docs, you can update the docstrings of `session.run` and `session.run_always`.
cjolowicz: > I don't think there is a PR template on this project is there? I'll maybe open an issue about adding one of those with some guidelines then.

I agree, that would be helpful.
raddessi: > What are your thoughts about adding parameters interrupt_timeout and terminate_timeout to session.runpopen, and omitting the CLI options? I feel that the command-line interface isn't achieving much here, in addition to the parameters.

That actually makes a lot more sense, CLi options aren't probably needed for this and having separate variables that match `tox` naming also makes sense. Should a default also be read from the config options so it could be set in the noxfile as `nox.options.<option_name>` or should it only be allowed as a kwarg to `session.run` that then gets passed to `popen`?

> We'll also need docs and a test. For the docs, you can update the docstrings of session.run and session.run_always.

For sure! Thank you for keeping the test coverage high, nox has been incredibly stable for me.
cjolowicz: > Should a default also be read from the config options so it could be set in the noxfile as `nox.options.<option_name>` or should it only be allowed as a kwarg to `session.run` that then gets passed to `popen`?

IMO it should be passed explicitly as a keyword argument. I don't think we need a configurable global default, and `nox.options` wouldn't be a good place for that as its attributes should mirror command-line options.
raddessi: Still need to check test coverage and update docs
raddessi: Sorry for the delay
raddessi: Nice! Yep, will do. Thanks!

Patch:
diff --git a/nox/popen.py b/nox/popen.py
index 6e88ee9..2d174ac 100644
--- a/nox/popen.py
+++ b/nox/popen.py
@@ -21,16 +21,19 @@ import sys
 from typing import IO, Mapping, Sequence
 
 
-def shutdown_process(proc: subprocess.Popen) -> tuple[bytes, bytes]:
+def shutdown_process(
+    proc: subprocess.Popen,
+    interrupt_timeout: float | None,
+    terminate_timeout: float | None,
+) -> tuple[bytes, bytes]:
     """Gracefully shutdown a child process."""
-
     with contextlib.suppress(subprocess.TimeoutExpired):
-        return proc.communicate(timeout=0.3)
+        return proc.communicate(timeout=interrupt_timeout)
 
     proc.terminate()
 
     with contextlib.suppress(subprocess.TimeoutExpired):
-        return proc.communicate(timeout=0.2)
+        return proc.communicate(timeout=terminate_timeout)
 
     proc.kill()
 
@@ -60,6 +63,8 @@ def popen(
     silent: bool = False,
     stdout: int | IO | None = None,
     stderr: int | IO = subprocess.STDOUT,
+    interrupt_timeout: float | None = 0.3,
+    terminate_timeout: float | None = 0.2,
 ) -> tuple[int, str]:
     if silent and stdout is not None:
         raise ValueError(
@@ -77,7 +82,7 @@ def popen(
         sys.stdout.flush()
 
     except KeyboardInterrupt:
-        out, err = shutdown_process(proc)
+        out, err = shutdown_process(proc, interrupt_timeout, terminate_timeout)
         if proc.returncode != 0:
             raise
 
diff --git a/nox/sessions.py b/nox/sessions.py
index 6d1cd83..4bea8e8 100644
--- a/nox/sessions.py
+++ b/nox/sessions.py
@@ -270,6 +270,17 @@ class Session:
                 'bash', '-c', 'echo $SOME_ENV',
                 env={'SOME_ENV': 'Hello'})
 
+        You can extend the shutdown timeout to allow long-running cleanup tasks to
+        complete before being terminated. For example, if you wanted to allow ``pytest``
+        extra time to clean up large projects in the case that nox receives an
+        interrupt signal from your build system and needs to terminate its child
+        processes::
+
+            session.run(
+                'pytest', '-k', 'long_cleanup',
+                interrupt_timeout=10.0,
+                terminate_timeout=2.0)
+
         You can also tell nox to treat non-zero exit codes as success using
         ``success_codes``. For example, if you wanted to treat the ``pytest``
         "tests discovered, but none selected" error as success::
@@ -308,6 +319,16 @@ class Session:
             ``--error-on-external-run``. This has no effect for sessions that
             do not have a virtualenv.
         :type external: bool
+        :param interrupt_timeout: The timeout (in seconds) that nox should wait after it
+            and its children receive an interrupt signal before sending a terminate
+            signal to its children. Set to ``None`` to never send a terminate signal.
+            Default: ``0.3``
+        :type interrupt_timeout: float or None
+        :param terminate_timeout: The timeout (in seconds) that nox should wait after it
+            sends a terminate signal to its children before sending a kill signal to
+            them. Set to ``None`` to never send a kill signal.
+            Default: ``0.2``
+        :type terminate_timeout: float or None
         """
         if not args:
             raise ValueError("At least one argument required to run().")
@@ -348,6 +369,16 @@ class Session:
             ``--error-on-external-run``. This has no effect for sessions that
             do not have a virtualenv.
         :type external: bool
+        :param interrupt_timeout: The timeout (in seconds) that nox should wait after it
+            and its children receive an interrupt signal before sending a terminate
+            signal to its children. Set to ``None`` to never send a terminate signal.
+            Default: ``0.3``
+        :type interrupt_timeout: float or None
+        :param terminate_timeout: The timeout (in seconds) that nox should wait after it
+            sends a terminate signal to its children before sending a kill signal to
+            them. Set to ``None`` to never send a kill signal.
+            Default: ``0.2``
+        :type terminate_timeout: float or None
         """
         if (
             self._runner.global_config.no_install


Test patch:
diff --git a/tests/test_sessions.py b/tests/test_sessions.py
index a71d1f9..688b8f1 100644
--- a/tests/test_sessions.py
+++ b/tests/test_sessions.py
@@ -283,7 +283,10 @@ class TestSession:
             session.run(sys.executable, "--version")
 
         run.assert_called_once_with(
-            (sys.executable, "--version"), external=True, env=mock.ANY, paths=None
+            (sys.executable, "--version"),
+            external=True,
+            env=mock.ANY,
+            paths=None,
         )
 
     def test_run_external_condaenv(self):
@@ -344,6 +347,56 @@ class TestSession:
 
         assert session.run_always(operator.add, 23, 19) == 42
 
+    @pytest.mark.parametrize(
+        (
+            "interrupt_timeout_setting",
+            "terminate_timeout_setting",
+            "interrupt_timeout_expected",
+            "terminate_timeout_expected",
+        ),
+        [
+            ("default", "default", 0.3, 0.2),
+            (None, None, None, None),
+            (0, 0, 0, 0),
+            (1, 2, 1, 2),
+        ],
+    )
+    def test_run_shutdown_process_timeouts(
+        self,
+        interrupt_timeout_setting,
+        terminate_timeout_setting,
+        interrupt_timeout_expected,
+        terminate_timeout_expected,
+    ):
+        session, runner = self.make_session_and_runner()
+
+        runner.venv = nox.virtualenv.ProcessEnv()
+
+        subp_popen_instance = mock.Mock()
+        subp_popen_instance.communicate.side_effect = KeyboardInterrupt()
+        with mock.patch(
+            "nox.popen.shutdown_process", autospec=True
+        ) as shutdown_process, mock.patch(
+            "nox.popen.subprocess.Popen",
+            new=mock.Mock(return_value=subp_popen_instance),
+        ):
+            shutdown_process.return_value = ("", "")
+
+            timeout_kwargs = {}
+            if interrupt_timeout_setting != "default":
+                timeout_kwargs["interrupt_timeout"] = interrupt_timeout_setting
+            if terminate_timeout_setting != "default":
+                timeout_kwargs["terminate_timeout"] = terminate_timeout_setting
+
+            with pytest.raises(KeyboardInterrupt):
+                session.run(sys.executable, "--version", **timeout_kwargs)
+
+        shutdown_process.assert_called_once_with(
+            proc=mock.ANY,
+            interrupt_timeout=interrupt_timeout_expected,
+            terminate_timeout=terminate_timeout_expected,
+        )
+
     @pytest.mark.parametrize(
         ("no_install", "reused", "run_called"),
         [