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.

Sign up for GitHub

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

Add a new option (and parameter) --kill-children #292

Merged
merged 6 commits into from
Aug 24, 2017
Merged

Add a new option (and parameter) --kill-children #292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3dbdc54
Add a new option (and parameter) --kill-children
benjastudio Aug 23, 2017
59c8bc8
Fix function name
benjastudio Aug 23, 2017
de55771
Use wait_procs
benjastudio Aug 23, 2017
41d862f
Use store_true
benjastudio Aug 23, 2017
33a8488
Handle NoSuchProcess
benjastudio Aug 23, 2017
3cab34a
Remove unused import
benjastudio Aug 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions simpleflow/execute.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

import time

import psutil

try:
import subprocess32 as subprocess
except ImportError:
Expand Down Expand Up @@ -172,7 +174,7 @@ def wait_subprocess(process, timeout=None, command_info=None):
return process.wait()


def python(interpreter='python', logger_name=__name__, timeout=None):
def python(interpreter='python', logger_name=__name__, timeout=None, kill_children=False):
"""
Execute a callable as an external Python program.

Expand Down Expand Up @@ -202,6 +204,8 @@ def execute(*args, **kwargs):
'--result-fd={}'.format(dup_result_fd),
'--error-fd={}'.format(dup_error_fd),
]
if kill_children:
full_command.append('--kill-children')
if compat.PY2: # close_fds doesn't work with python2 (using its C _posixsubprocess helper)
close_fds = False
pass_fds = ()
Expand All @@ -214,9 +218,7 @@ def execute(*args, **kwargs):
close_fds=close_fds,
pass_fds=pass_fds,
)

rc = wait_subprocess(process, timeout=timeout, command_info=full_command)

os.close(dup_result_fd)
os.close(dup_error_fd)
if rc:
Expand Down Expand Up @@ -409,7 +411,29 @@ def main():
metavar='N',
help='error file descriptor',
)
parser.add_argument(
'--kill-children',
action='store_true',
help='kill child processes on exit',
)
cmd_arguments = parser.parse_args()

def kill_child_processes():
process = psutil.Process(os.getpid())
children = process.children(recursive=True)

for child in children:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try ... except NoSuchProcess: pass around the loops?
Or do you think the caller should know this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the caller should now, +1 for catching

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's okay since children list come from https://pythonhosted.org/psutil/#psutil.Process.children call.
BTW: I just saw that and I think I'll use wait_procs instead of sleep it: https://pythonhosted.org/psutil/#psutil.wait_procs

Copy link
Contributor

@ybastide ybastide Aug 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er, your test catches it 🙂
EDIT: and if a child was terminated (as it should), kill won't find it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway you're right, wait_proc is the way to go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that even if they die by themselves it's ok since the references are kept (they are zombies).

In [1]: import subprocess, psutil, os, time

In [2]: sleeper = subprocess.Popen(['sleep', '5']) ; children = psutil.Process(os.getpid()).children(recursive=True)

In [3]: time.sleep(6)

In [4]: children
Out[4]: [<psutil.Process(pid=15234, name='sleep') at 140707464425808>]

In [5]: [c.status() for c in children]
Out[5]: ['zombie']

In [6]: [c.terminate() for c in children]
Out[6]: [None]

In [7]: [c.status() for c in children]
Out[7]: ['zombie']

In [8]: [c.kill() for c in children]
Out[8]: [None]

In [9]: [c.status() for c in children]
Out[9]: ['zombie']

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I can add it, may be there is a case where this won't work?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yet another reason to use psutil! No no, don't bother adding a useless try/except, I think @ybastide and I are too cautious but looks like psutil is smart there 👍 Thanks a lot for testing it!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, here's psutil:

        def _send_signal(self, sig):
            assert not self.pid < 0, self.pid
            if self.pid == 0:
                # see "man 2 kill"
                raise ValueError(
                    "preventing sending signal to process with PID 0 as it "
                    "would affect every process in the process group of the "
                    "calling process (os.getpid()) instead of PID 0")
            try:
                os.kill(self.pid, sig)
            except OSError as err:
                if err.errno == errno.ESRCH:
                    if OPENBSD and pid_exists(self.pid):
                        # We do this because os.kill() lies in case of
                        # zombie processes.
                        raise ZombieProcess(self.pid, self._name, self._ppid)
                    else:
                        self._gone = True
                        raise NoSuchProcess(self.pid, self._name)
                if err.errno in (errno.EPERM, errno.EACCES):
                    raise AccessDenied(self.pid, self._name)
                raise

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always look the code 👌

try:
child.terminate()
except psutil.NoSuchProcess:
pass
_, still_alive = psutil.wait_procs(children, timeout=0.3)
for child in still_alive:
try:
child.kill()
except psutil.NoSuchProcess:
pass

funcname = cmd_arguments.funcname
try:
arguments = format.decode(cmd_arguments.funcargs)
Expand Down Expand Up @@ -447,6 +471,8 @@ def main():
if not compat.PY2:
details = details.encode('utf-8')
os.write(cmd_arguments.error_fd, details)
if cmd_arguments.kill_children:
kill_child_processes()
sys.exit(1)

if cmd_arguments.result_fd == 1: # stdout (legacy)
Expand All @@ -456,6 +482,8 @@ def main():
if not compat.PY2:
result = result.encode('utf-8')
os.write(cmd_arguments.result_fd, result)
if cmd_arguments.kill_children:
kill_child_processes()


if __name__ == '__main__':
Expand Down
21 changes: 21 additions & 0 deletions tests/test_simpleflow/test_execute.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@
import platform
import threading

import psutil
import pytest
import time

import subprocess

from simpleflow import execute
from simpleflow.exceptions import ExecutionError, ExecutionTimeoutError

Expand Down Expand Up @@ -287,3 +290,21 @@ def test_timeout_execute_from_thread():
t = threading.Thread(target=test_timeout_execute)
t.start()
t.join()


def create_sleeper_subprocess():
pid = subprocess.Popen(['sleep', '600']).pid
return pid


def test_execute_dont_kill_children():
pid = execute.python()(create_sleeper_subprocess)()
subprocess = psutil.Process(pid)
assert subprocess.status() == 'sleeping'
subprocess.terminate() # cleanup


def test_execute_kill_children():
pid = execute.python(kill_children=True)(create_sleeper_subprocess)()
with pytest.raises(psutil.NoSuchProcess):
psutil.Process(pid)