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

should we be more aggressive about reaping child processes? #879

Closed
njsmith opened this issue Jan 25, 2019 · 4 comments
Closed

should we be more aggressive about reaping child processes? #879

njsmith opened this issue Jan 25, 2019 · 4 comments

Comments

@njsmith
Copy link
Member

njsmith commented Jan 25, 2019

Right now, I believe that we don't reap child processes until the user calls .wait or .poll. (or, if that never happens, then until the backstop code in subprocess starts trying to reap old processes when a new one is spawned.)

Of course on Windows this is fine because zombies aren't a thing. But maybe we should reap more aggressively on posix? If we were using SIGCHLD like most async libraries do, then we'd be reaping processes as soon as they exit.

E.g. on Linux, maybe the wait logic should be something like:

def _thread_wait(token, process):
    process._popen.wait()
    token.call_soon_threadsafe(process._waited.set()

?

(Note that Popen.wait is thread-safe, so this is legal: https://bugs.python.org/issue21291)

CC: @oremanj

@oremanj
Copy link
Member

oremanj commented Feb 3, 2019

Interesting idea! I think the main practical benefit would be that if some code using a trio.Process gets cancelled, and the process wasn't wrapped in an async with, we won't leave a zombie behind when it does finally exit. And I guess it would let us stop using waitid() on Linux, which is hairy on the pypy side of things.

Our current reaping behavior mirrors subprocess.Popen: processes don't get reaped unless you ask for them to be reaped. This implies for example that proc.returncode doesn't change if you don't call poll() or wait(). I'm not sure there's a specific reason to keep that beyond "might as well be consistent if we don't have a reason not to be". I can't particularly think of any case where one would want to keep a zombie lying around. If we always auto-wait in the background, poll() stops being a meaningful function, and users who want to check if the process has exited can just inspect proc.returncode.

@njsmith
Copy link
Member Author

njsmith commented Jul 27, 2019

The distinction between returncode and poll has always bothered me. returncode ought to be a property that automatically polls if necessary...

@oremanj
Copy link
Member

oremanj commented May 12, 2020

#1315 gives another reason for doing this.

@njsmith
Copy link
Member Author

njsmith commented May 12, 2020

I think the conclusion here is that we should present the user with the illusion that returncode is always up to date, so we'll auto-reap the process if the user looks at all (see #1315, #1514). But, I don't think we really need to get any more aggressive than that – the stdlib subprocess doesn't and they do OK, our high-level run_process/start_process APIs will always reap (see #1104), and implementing automagic background reaping is non-trivial, especially on kqueue platforms. So closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants