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

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

Merged
merged 6 commits into from
Aug 24, 2017

Conversation

benjastudio
Copy link
Contributor

@benjastudio benjastudio commented Aug 23, 2017

This option forces simpleflow to kill child processes in simpleflow.execute, on exit.

Note: I wasn't be able to use atexit correctly (at least with Pypy).

Related issue: #291

This option force simpleflow to kill child processes in simpleflow.execute, on exit.
@benjastudio benjastudio force-pushed the enhancement/291/option-to-kill-child-processes branch from 8d24b9e to 59c8bc8 Compare August 23, 2017 14:48
'--kill-children',
action='store_const',
const=True,
default=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: you can use

action='store_true'

instead of the 3 lines

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

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 👌

@ybastide
Copy link
Contributor

👍

@jbbarth
Copy link
Collaborator

jbbarth commented Aug 23, 2017

@ybastide or @benjastudio : I let one of you merge and try the new release script so we confirm it works for you? if you want to do it @benjastudio you'd need credentials on the pypi package, signup there and give me or @ybastide your nickname so we add you.

@benjastudio benjastudio merged commit b349e4b into master Aug 24, 2017
@jbbarth jbbarth deleted the enhancement/291/option-to-kill-child-processes branch August 24, 2017 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants