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

Allow to config the behavior on CTRL+C #1493

Merged
merged 1 commit into from Jan 9, 2020
Merged

Conversation

@sileht
Copy link
Contributor

sileht commented Jan 8, 2020

By default, we run SIGKILL after 0.5 seconds. Most of the time is
enough. But if the interrupted command have a complex processes tree,
it might not be enough to propagate the signal.

In such case processes are left behind and never killed.
If theses processes use static network port or keep file open. Next
call of tox will fail until the all processes left behind are manually
killed.

This change adds some configuration to be able to config the timeout
before signals are sent.

If the approach work for you, I will polish the PR (doc+test)

Contribution checklist:

(also see CONTRIBUTING.rst for details)

  • wrote descriptive pull request text
  • added/updated test(s)
  • updated/extended the documentation
  • added relevant issue keyword
    in message body
  • added news fragment in changelog folder
    • fragment name: <issue number>.<type>.rst for example (588.bugfix.rst)
    • <type> is must be one of bugfix, feature, deprecation,breaking, doc, misc
    • if PR has no issue: consider creating one first or change it to the PR number after creating the PR
    • "sign" fragment with "by :user:<your username>"
    • please use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files - by :user:superuser."
    • also see examples
  • added yourself to CONTRIBUTORS (preserving alphabetical order)
@sileht sileht force-pushed the sileht:ctrl+c branch from 7bc42ba to 39d4e2a Jan 9, 2020
Copy link
Member

gaborbernat left a comment

On the surface the idea looks ok; will need tests and documentation for it though 👍

src/tox/session/__init__.py Outdated Show resolved Hide resolved
src/tox/config/__init__.py Outdated Show resolved Hide resolved
@sileht sileht force-pushed the sileht:ctrl+c branch 4 times, most recently from 03e6153 to 34ee673 Jan 9, 2020
@sileht sileht force-pushed the sileht:ctrl+c branch from 34ee673 to b2830c2 Jan 9, 2020
@@ -0,0 +1 @@
Add ``interrupt_timeout`` and ``terminate_timeout`` that configure delay between SIGINT, SIGTERM and SIGKILL when tox is interrupted. - by :user:`sileht`

This comment has been minimized.

Copy link
@gaborbernat

gaborbernat Jan 9, 2020

Member

you can use the :conf:`interrupt_timeout` to make them references please do so

This comment has been minimized.

Copy link
@sileht

sileht Jan 9, 2020

Author Contributor

done

By default, we run SIGKILL after 0.5 seconds. Most of the time is
enough. But if the interrupted command have a complex processes tree,
it might not be enough to propagate the signal.

In such case processes are left behind and never killed.
If theses processes use static network port or keep file open. Next
call of tox will fail until the all processes left behind are manually
killed.

This change adds some configuration to be able to config the timeout
before signals are sent.

If the approach work for you, I will polish the PR (doc+test)
@sileht sileht force-pushed the sileht:ctrl+c branch from b2830c2 to eea285d Jan 9, 2020
@gaborbernat gaborbernat merged commit e639a54 into tox-dev:master Jan 9, 2020
2 checks passed
2 checks passed
Timeline protection Timeline protection: Good to go
Details
tox ci #tox ci_20200109.07 succeeded
Details
@gaborbernat

This comment has been minimized.

Copy link
Member

gaborbernat commented Jan 9, 2020

Thanks!

@stefanholek

This comment has been minimized.

Copy link

stefanholek commented Jan 13, 2020

Hi People,

I have been dealing with a similar issue the other day, and my solution is different. 😎

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 solutions is to make tox wait for processes to go away on their own before starting to send signals.

I see you are talking about SIGKILL, but maybe that's just a side-effect of tox SIGINT'ing immediately? Also, my issue is not fixed by the changes in #1493.

Anyway, here is my patch (which no longer applies now, 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:
@sileht

This comment has been minimized.

Copy link
Contributor Author

sileht commented Jan 13, 2020

#1493 does not change anything about signal handling by default. But just allow to configure different timeouts.

I got your double SIGINT issue. But my issue was a bit different.
My tooling don't care about receiving SIGINT/SIGTERM manytimes, any additional signals are just ignored during the processes shutdown procedure. My patch was to allow to increase the delay between the first SIGINT and the final deadly SIGKILL, so my test runner can cleanly shutdown everything, other signals sent between are ignored by my tooling.

I think your issue/use-case is valid too, feel free to open an issue to track it.

@stefanholek

This comment has been minimized.

Copy link

stefanholek commented Jan 13, 2020

Ok, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.