Repository: theacodes/nox (python)

Issue:
CTRL-C always results in an error
If `nox -s foo` encounters a `KeyboardInterrupt` but the currently running script catches it and handles it gracefully, `nox` will still report a failure. For example:

```
$ nox -s app -r
nox > Running session app-3.7
nox > Re-using existing virtualenv at .../.nox/app-3-7.
nox > pip install --upgrade --requirement .../requirements/app.txt
nox > cd .../src
nox > python -m flask run
 * Serving Flask app "app.py" (lazy loading)
...
127.0.0.1 - - [20/Sep/2018 15:43:27] "GET /foo HTTP/1.1" 200 -
127.0.0.1 - - [20/Sep/2018 15:43:31] "GET /bar HTTP/1.1" 200 -
127.0.0.1 - - [20/Sep/2018 15:43:33] "GET /baz HTTP/1.1" 404 -
^Cnox > Interrupted...
nox > Session app-3.7 interrupted.
$ echo $?
130
```

However, if the same application runs outside of `nox` there is no error (i.e. `flask` exits gracefully):

```
$ cd src/
$ FLASK_APP=app.py ../.nox/app-3-7/bin/python -m flask run
 * Serving Flask app "app.py"
...
127.0.0.1 - - [20/Sep/2018 15:46:09] "GET /foo HTTP/1.1" 200 -
127.0.0.1 - - [20/Sep/2018 15:46:12] "GET /baz HTTP/1.1" 404 -
127.0.0.1 - - [20/Sep/2018 15:46:13] "GET /bar HTTP/1.1" 200 -
^C
$ echo $?
0
$ cd ..
```

What should the correct `nox` behavior be here?

Pull request:
Gracefully shutdown child processes
Fixes #139

This PR changes Nox's behavior when receiving a keyboard interrupt (CTRL-C, `SIGINT`) while executing a command with `session.run` or `session.install`.

The current behavior is to terminate the child immediately (`SIGTERM` on Posix, `TerminateProcess` on Windows) and to reraise the `KeyboardInterrupt`, ultimately exiting with failure.

The new behavior is to shut down the child gracefully and only raise if the child did not exit with success. The shutdown sequence consists of these four steps:

1. Check if the child has terminated, with a 300ms timeout. If it did, skip to the last step. Note that the OS delivers the signal to the child as well.
2. Send `SIGTERM` and check again, with a 200ms timeout. If it did, skip to the last step.
3. Send `SIGKILL` and wait without timeout.
4. If the exit status of the child process is non-zero, reraise the `KeyboardInterrupt`. Otherwise, we consider the keyboard interrupt handled and return normally.
   
A few points of note:

- We should probably move `logger.error("Interrupted...")` from `command.run` to `popen.popen`, so the user gets immediate feedback before the shutdown sequence takes course. ~~We'd need to pass `logger` to `popen.popen`, though.~~ What do you think?

- The timeouts correspond to tox's defaults for its `interrupt_timeout` and `terminate_timeout` settings. For commands that need longer to shut down, it may be a good idea to make this configurable in Nox as well. These could be keyword arguments for `session.run`. I don't think this needs to be addressed by this PR though, right? (Interestingly, tox4 no longer seems to have these configurable.)

- tox does the shutdown slightly differently. It first checks if the child has terminated (with a timeout of zero), and sends a duplicate `SIGINT` if it hasn't. I believe the reason is that the shutdown protocol is also used in cases where there was no initial `SIGINT`. The rest of the shutdown sequence is the same. tox always reraises the interrupt, even if the child exited with success. (tox4 essentially behaves like tox3, but busy-waits and skips step 3 on Windows where `Popen.kill` is an alias for `Popen.terminate`.)

- The use case with `pytest --pdb` is not covered here; Nox still insists on terminating the child. When I tried this, I did get to see pytest's traceback before Nox killed it, which is an improvement. One way to support this use case would be a command-line option to disable sending additional signals to the child. I don't think this needs to be addressed by this PR either. tox also does not support this currently (https://github.com/tox-dev/tox/issues/1791).

- When output is captured and the child handles the interrupt, we silently discard anything `Popen.communicate` buffered for us before it was interrupted. However, the only *documented* way to capture output from `session.run` is the `silent` flag, and in that case it doesn't matter.


Patch:
diff --git a/nox/popen.py b/nox/popen.py
index c152f7d..8e981e1 100644
--- a/nox/popen.py
+++ b/nox/popen.py
@@ -12,10 +12,27 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import contextlib
 import locale
 import subprocess
 import sys
-from typing import IO, Mapping, Sequence, Tuple, Union
+from typing import IO, Mapping, Optional, Sequence, Tuple, Union
+
+
+def shutdown_process(proc: subprocess.Popen) -> Tuple[Optional[bytes], Optional[bytes]]:
+    """Gracefully shutdown a child process."""
+
+    with contextlib.suppress(subprocess.TimeoutExpired):
+        return proc.communicate(timeout=0.3)
+
+    proc.terminate()
+
+    with contextlib.suppress(subprocess.TimeoutExpired):
+        return proc.communicate(timeout=0.2)
+
+    proc.kill()
+
+    return proc.communicate()
 
 
 def decode_output(output: bytes) -> str:
@@ -57,9 +74,9 @@ def popen(
         sys.stdout.flush()
 
     except KeyboardInterrupt:
-        proc.terminate()
-        proc.wait()
-        raise
+        out, err = shutdown_process(proc)
+        if proc.returncode != 0:
+            raise
 
     return_code = proc.wait()
 


Test patch:
diff --git a/tests/test_command.py b/tests/test_command.py
index 01fb06e..fec272b 100644
--- a/tests/test_command.py
+++ b/tests/test_command.py
@@ -12,9 +12,15 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import ctypes
 import logging
 import os
+import platform
+import signal
+import subprocess
 import sys
+import time
+from textwrap import dedent
 from unittest import mock
 
 import nox.command
@@ -23,6 +29,15 @@ import pytest
 
 PYTHON = sys.executable
 
+skip_on_windows_primary_console_session = pytest.mark.skipif(
+    platform.system() == "Windows" and "SECONDARY_CONSOLE_SESSION" not in os.environ,
+    reason="On Windows, this test must run in a separate console session.",
+)
+
+only_on_windows = pytest.mark.skipif(
+    platform.system() != "Windows", reason="Only run this test on Windows."
+)
+
 
 def test_run_defaults(capsys):
     result = nox.command.run([PYTHON, "-c", "print(123)"])
@@ -98,7 +113,7 @@ def test_run_env_unicode():
     result = nox.command.run(
         [PYTHON, "-c", 'import os; print(os.environ["SIGIL"])'],
         silent=True,
-        env={u"SIGIL": u"123"},
+        env={"SIGIL": "123"},
     )
 
     assert "123" in result
@@ -196,13 +211,153 @@ def test_fail_with_silent(capsys):
         assert "err" in err
 
 
-def test_interrupt():
-    mock_proc = mock.Mock()
-    mock_proc.communicate.side_effect = KeyboardInterrupt()
+@pytest.fixture
+def marker(tmp_path):
+    """A marker file for process communication."""
+    return tmp_path / "marker"
+
+
+def enable_ctrl_c(enabled):
+    """Enable keyboard interrupts (CTRL-C) on Windows."""
+    kernel32 = ctypes.WinDLL("kernel32", use_last_error=True)
+
+    if not kernel32.SetConsoleCtrlHandler(None, not enabled):
+        raise ctypes.WinError(ctypes.get_last_error())
+
+
+def interrupt_process(proc):
+    """Send SIGINT or CTRL_C_EVENT to the process."""
+    if platform.system() == "Windows":
+        # Disable Ctrl-C so we don't terminate ourselves.
+        enable_ctrl_c(False)
+
+        # Send the keyboard interrupt to all processes attached to the current
+        # console session.
+        os.kill(0, signal.CTRL_C_EVENT)
+    else:
+        proc.send_signal(signal.SIGINT)
+
+
+@pytest.fixture
+def command_with_keyboard_interrupt(monkeypatch, marker):
+    """Monkeypatch Popen.communicate to raise KeyboardInterrupt."""
+    if platform.system() == "Windows":
+        # Enable Ctrl-C because the child inherits the setting from us.
+        enable_ctrl_c(True)
+
+    communicate = subprocess.Popen.communicate
+
+    def wrapper(proc, *args, **kwargs):
+        # Raise the interrupt only on the first call, so Nox has a chance to
+        # shut down the child process subsequently.
+
+        if wrapper.firstcall:
+            wrapper.firstcall = False
+
+            # Give the child time to install its signal handlers.
+            while not marker.exists():
+                time.sleep(0.05)
+
+            # Send a real keyboard interrupt to the child.
+            interrupt_process(proc)
+
+            # Fake a keyboard interrupt in the parent.
+            raise KeyboardInterrupt
+
+        return communicate(proc, *args, **kwargs)
+
+    wrapper.firstcall = True
+
+    monkeypatch.setattr("subprocess.Popen.communicate", wrapper)
+
+
+def format_program(program, marker):
+    """Preprocess the Python program run by the child process."""
+    main = f"""
+    import time
+    from pathlib import Path
+
+    Path({str(marker)!r}).touch()
+    time.sleep(3)
+    """
+    return dedent(program).format(MAIN=dedent(main))
+
+
+def run_pytest_in_new_console_session(test):
+    """Run the given test in a separate console session."""
+    env = dict(os.environ, SECONDARY_CONSOLE_SESSION="")
+    creationflags = (
+        subprocess.CREATE_NO_WINDOW
+        if sys.version_info[:2] >= (3, 7)
+        else subprocess.CREATE_NEW_CONSOLE
+    )
+
+    subprocess.run(
+        [sys.executable, "-m", "pytest", f"{__file__}::{test}"],
+        env=env,
+        check=True,
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+        creationflags=creationflags,
+    )
+
+
+@skip_on_windows_primary_console_session
+@pytest.mark.parametrize(
+    "program",
+    [
+        """
+        {MAIN}
+        """,
+        """
+        import signal
+
+        signal.signal(signal.SIGINT, signal.SIG_IGN)
+
+        {MAIN}
+        """,
+        """
+        import signal
+
+        signal.signal(signal.SIGINT, signal.SIG_IGN)
+        signal.signal(signal.SIGTERM, signal.SIG_IGN)
+
+        {MAIN}
+        """,
+    ],
+)
+def test_interrupt_raises(command_with_keyboard_interrupt, program, marker):
+    """It kills the process and reraises the keyboard interrupt."""
+    with pytest.raises(KeyboardInterrupt):
+        nox.command.run([PYTHON, "-c", format_program(program, marker)])
+
+
+@only_on_windows
+def test_interrupt_raises_on_windows():
+    """It kills the process and reraises the keyboard interrupt."""
+    run_pytest_in_new_console_session("test_interrupt_raises")
+
+
+@skip_on_windows_primary_console_session
+def test_interrupt_handled(command_with_keyboard_interrupt, marker):
+    """It does not raise if the child handles the keyboard interrupt."""
+    program = """
+    import signal
+
+    def exithandler(sig, frame):
+        raise SystemExit()
+
+    signal.signal(signal.SIGINT, exithandler)
+
+    {MAIN}
+    """
+    nox.command.run([PYTHON, "-c", format_program(program, marker)])
+
 
-    with mock.patch("subprocess.Popen", return_value=mock_proc):
-        with pytest.raises(KeyboardInterrupt):
-            nox.command.run([PYTHON, "-c" "123"])
+@only_on_windows
+def test_interrupt_handled_on_windows():
+    """It does not raise if the child handles the keyboard interrupt."""
+    run_pytest_in_new_console_session("test_interrupt_handled")
 
 
 def test_custom_stdout(capsys, tmpdir):