Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Immediately forwarding Ctrl+C to child processes considered harmful #1497

Closed
stefanholek opened this issue Jan 13, 2020 · 3 comments · Fixed by #1588
Closed

Immediately forwarding Ctrl+C to child processes considered harmful #1497

stefanholek opened this issue Jan 13, 2020 · 3 comments · Fixed by #1588
Labels
bug:normal pr-merged

Comments

@stefanholek
Copy link
Contributor

stefanholek commented Jan 13, 2020

I have a test runner which install unittests's signal handlers. When hitting Ctrl+C in tox, the test runner sometimes receives a SIGINT while in the process of shutting down. This seemed wrong, and I traced it down to tox unconditionally forwarding any received SIGINT. The situation however is that all foreground processes receive the SIGINT, not just tox, and when tox forwards it the test runner receives the SIGINT twice. My solution is to make tox wait for processes to go away on their own before starting to send signals.

Anyway, here is my patch (which no longer applies after #1493, but you get the idea):

diff --git a/src/tox/action.py b/src/tox/action.py
index 10707b4..9f826a0 100644
--- a/src/tox/action.py
+++ b/src/tox/action.py
@@ -18,6 +18,7 @@ from tox.reporter import Verbosity
 from tox.util.lock import get_unique_file
 from tox.util.stdlib import is_main_thread
 
+WAIT_SUICIDE = 0.5
 WAIT_INTERRUPT = 0.3
 WAIT_TERMINATE = 0.2
 
@@ -177,7 +178,7 @@ class Action(object):
     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, WAIT_SUICIDE) 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, WAIT_INTERRUPT) is None:
@@ -193,7 +194,7 @@ class Action(object):
         if sys.version_info >= (3, 3):
             # python 3 has timeout feature built-in
             try:
-                process.communicate(timeout=WAIT_INTERRUPT)
+                process.communicate(timeout=timeout)
             except subprocess.TimeoutExpired:
                 pass
         else:
@stefanholek stefanholek added the bug:normal label Jan 13, 2020
@stefanholek
Copy link
Contributor Author

stefanholek commented Jan 17, 2020

I have looked further into this and it appears that in response to Marius' freak accident with the zope-testrunner (#1172) the code was changed from process.wait() to basically process.murder().

That's quite radical IMO and prone to issues like the above. I guess the only reason it did not come up earlier is that few test runners actually have to perform any cleanups.

I am happy to turn this into a PR à la #1493 but would appreciate some feedback first.

@blueyed
Copy link
Contributor

blueyed commented Mar 26, 2020

I think the patch makes sense - just tried it again after having upvoted it before already.
Needs adjustments after #1493 however.

My use case is running pytest in tox, where it often looks like this then (and often much worse/longer):

testing/test_config.py ............^C
=================================================================================== short test summary info ====================================================================================

SKIPPED [1] testing/test_capture.py:1395: only py3.6+ on windows
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! KeyboardInterrupt !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
/tmp/tox…/Vcs/pytest/py38-coverage/lib/python3.8/site-packages/_pytest/mark/structures.py:386: KeyboardInterrupt
(to show a full traceback on KeyboardInterrupt use --full-trace)
================================================================================ 194 passed, 1 skipped in 3.65s ================================================================================
ERROR: got KeyboardInterrupt signal
Traceback (most recent call last):
  File "/tmp/tox…/Vcs/pytest/py38-coverage/lib/python3.8/site-packages/coverage/cmdline.py", line 703, in do_run
    runner.run()
  File "/tmp/tox…/Vcs/pytest/py38-coverage/lib/python3.8/site-packages/coverage/execfile.py", line 247, in run
    exec(code, main_mod.__dict__)
  File "/tmp/tox…/Vcs/pytest/py38-coverage/lib/python3.8/site-packages/pytest/__main__.py", line 8, in <module>
    raise SystemExit(pytest.main())
SystemExit: ExitCode.INTERRUPTED

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/tmp/tox…/Vcs/pytest/py38-coverage/bin/coverage", line 8, in <module>
    sys.exit(main())
  File "/tmp/tox…/Vcs/pytest/py38-coverage/lib/python3.8/site-packages/coverage/cmdline.py", line 827, in main
    status = CoverageScript().command_line(argv)
  File "/tmp/tox…/Vcs/pytest/py38-coverage/lib/python3.8/site-packages/coverage/cmdline.py", line 555, in command_line
    return self.do_run(options, args)
  File "/tmp/tox…/Vcs/pytest/py38-coverage/lib/python3.8/site-packages/coverage/cmdline.py", line 710, in do_run
    self.coverage.save()
  File "/tmp/tox…/Vcs/pytest/py38-coverage/lib/python3.8/site-packages/coverage/control.py", line 642, in save
    data = self.get_data()
  File "/tmp/tox…/Vcs/pytest/py38-coverage/lib/python3.8/site-packages/coverage/control.py", line 696, in get_data
    if self._collector and self._collector.flush_data():
  File "/tmp/tox…/Vcs/pytest/py38-coverage/lib/python3.8/site-packages/coverage/collector.py", line 423, in flush_data
    self.covdata.add_arcs(self.mapped_file_dict(self.data))
  File "/tmp/tox…/Vcs/pytest/py38-coverage/lib/python3.8/site-packages/coverage/sqldata.py", line 477, in add_arcs
    con.executemany(
  File "/tmp/tox…/Vcs/pytest/py38-coverage/lib/python3.8/site-packages/coverage/sqldata.py", line 1089, in executemany
    return self.con.executemany(sql, data)
KeyboardInterrupt
___________________________________________________________________________________________ summary ____________________________________________________________________________________________
ERROR:   py38-coverage: keyboardinterrupt

@gaborbernat
Copy link
Member

gaborbernat commented Apr 19, 2020

Feel free to open a PR against master and fix it. My available efforts at the moment are aimed at fixing this as part of #1394, but that probably will take a while (ETA September).

jhesketh added a commit to jhesketh/tox that referenced this issue May 21, 2020
Fixes tox-dev#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 <stefan@epy.co.at>
jhesketh added a commit to jhesketh/tox that referenced this issue May 21, 2020
Fixes tox-dev#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 <stefan@epy.co.at>
@helpr helpr bot added the pr-available label May 21, 2020
jhesketh added a commit to jhesketh/tox that referenced this issue May 21, 2020
Fixes tox-dev#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 <stefan@epy.co.at>
jhesketh added a commit to jhesketh/tox that referenced this issue May 21, 2020
Fixes tox-dev#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 <stefan@epy.co.at>
jhesketh added a commit to jhesketh/tox that referenced this issue May 21, 2020
Fixes tox-dev#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 <stefan@epy.co.at>
jhesketh added a commit to jhesketh/tox that referenced this issue Jun 2, 2020
Fixes tox-dev#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 <stefan@epy.co.at>
@helpr helpr bot added pr-merged and removed pr-available labels Jun 3, 2020
@tox-dev tox-dev locked and limited conversation to collaborators Jan 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug:normal pr-merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants