subprocesses and waitpid()

[Charles-François Natali]

Hi,

While browsing the code, I noticed this in unix_events.py:
"""
def _sig_chld(self):
try:
try:
pid, status = os.waitpid(0, os.WNOHANG)
except ChildProcessError:
return
"""

The current code will only wait children in the same process group, so
if a child process called setpgrp() or setsid() (which is common e.g.
for a daemon), the above code won't work as expected: is this wanted?

Otherwise, what's the rationale behind this code?
I'm thinking about a couple things:
1. registering a signal handler for SIGCHLD makes it much more likely
to have syscalls failing with EINTR
2. by waiting for all children indiscriminately, this will make some
user code calling waitpid() on child processes they spawned fail with
ECHILD (subprocess is guarded against this, but I expect some
third-party code might not be)

I guess the reason is to avoid having to call waitpid() on multiple
PIDs at a periodic interval...

cf

[Guido van Rossum]

On Sat, Oct 19, 2013 at 5:17 AM, Charles-François Natali <cf.n...@gmail.com> wrote:

While browsing the code, I noticed this in unix_events.py:
"""
    def _sig_chld(self):
        try:
            try:
                pid, status = os.waitpid(0, os.WNOHANG)
            except ChildProcessError:
                return
"""

The current code will only wait children in the same process group, so
if a child process called setpgrp() or setsid() (which is common e.g.
for a daemon), the above code won't work as expected: is this wanted?

It sounds like nobody thought about this yet -- do you have a suggestio on how to fix it?

The subprocess code is definitely not as baked as I'd like it to be -- I haven't had enough time to review and try it. See e.g. http://code.google.com/p/tulip/issues/detail?id=68, that issue light be related.

 

Otherwise, what's the rationale behind this code?

It just sounds odd to have to do explicit regular polling for every known subprocess when the SIGCHLD feature was designed to notify you when you should poll.

 

I'm thinking about a couple things:
1. registering a signal handler for SIGCHLD makes it much more likely
to have syscalls failing with EINTR
2. by waiting for all children indiscriminately, this will make some
user code calling waitpid() on child processes they spawned fail with
ECHILD (subprocess is guarded against this, but I expect some
third-party code might not be)

I guess the reason is to avoid having to call waitpid() on multiple
PIDs at a periodic interval...

1. Syscalls failing with EINTR *shouldn't* be a problem when using Tulip, the send/receive code deals with this, all calls have an except (BlockingIOError, InterruptedError) clause.

2. I guess this is an interoperability issue between Tulip and code that manually spawns subprocesses (and bypassing the subprocess module). I expect that would rarely be an issue. Perhaps we can design the subprocess handling so that you can turn off the SIGCHLD handler and in that case let the event loop poll for specific children repeatedly?

--
--Guido van Rossum (python.org/~guido)

[Glyph]

On Oct 19, 2013, at 9:34 AM, Guido van Rossum <gu...@python.org> wrote:

Otherwise, what's the rationale behind this code?

It just sounds odd to have to do explicit regular polling for every known subprocess when the SIGCHLD feature was designed to notify you when you should poll.

Twisted used to do this because we had bugs where it looked like processes were getting "lost" and not getting reaped.  Eventually we managed to track down all the subtle race conditions there, and it seems to be pretty reliable, as long as you deal with signal coalescing by always continuing to call waitpid until it finds no further processes to reap.  (You may only get one SIGCHLD even if multiple processes exit.)

-glyph

[Glyph]

On Oct 19, 2013, at 9:34 AM, Guido van Rossum <gu...@python.org> wrote:

1. Syscalls failing with EINTR *shouldn't* be a problem when using Tulip, the send/receive code deals with this, all calls have an except (BlockingIOError, InterruptedError) clause.

But you *do* still have people calling write() and read() though, right?  Like, for log files and stuff?  And in database drivers?  Or maybe when importing a module?  I don't believe the import system is signal safe :-).  You will definitely see explosions all over the place from library code if you don't address this problem.

Luckily it's super easy to address in these modern times!  Just call signal.siginterrupt(signal.SIGCHLD, False) and it's solved.

Actually I think I'd recommend making this the default for most signal handlers unless otherwise specified (maybe for one of SIGINT/SIGTERM so you can get something like KeyboardInterrupt to wake up a stuck process).  The main reason _not_ to do that is that if you're doing lots of slow, blocking I/O, you don't know when your Python program is going to wake up next; but with set_wakeup_fd you can be pretty sure that receipt of the signal will trigger a timely wakeup.

2. I guess this is an interoperability issue between Tulip and code that manually spawns subprocesses (and bypassing the subprocess module). I expect that would rarely be an issue.

Speaking from Twisted's experience, people _love_ to write libraries that wrap one-off subprocess spawning with os.popen() or whatever.  Maybe that's less common these days, but I doubt it; just grepping my own local pile of libraries for the absolutely most obvious cases, I found non-subprocess-based spawning calls in epydoc, IPython, docutils, PIL, and py2app.  I'm sure there's more out there in the wild.

Perhaps we can design the subprocess handling so that you can turn off the SIGCHLD handler and in that case let the event loop poll for specific children repeatedly?

I think it's safest to go with a design where you call waitpid on specific pids in the SIGCHLD handler.  Other systems which actually want to register their own SIGCHLD handler, like Twisted, can somehow integrate with that (although asyncio/tulip's handler would then have to be public).  One-off things that call popen() et. al. will work just fine as long as you've made the signal non-interrupting with siginterrupt (so that reading from the popen'd pipe doesn't always fail with an EINTR right at EOF) and you don't call waitpid(0); they generally don't have a handler registered, they just spawn the process and then call waitpid immediately.

It might also be cool to have a high-performance/low-safety mode where you can call waitpid(0), but applications should have to opt in to that when they know the libraries they're calling aren't going to cause issues.

-glyph

[Antoine Pitrou]

On Sat, 19 Oct 2013 09:34:49 -0700

Guido van Rossum <gu...@python.org> wrote:

> On Sat, Oct 19, 2013 at 5:17 AM, Charles-François Natali <
> cf.n...@gmail.com> wrote:
>
> > While browsing the code, I noticed this in unix_events.py:
> > """
> > def _sig_chld(self):
> > try:
> > try:
> > pid, status = os.waitpid(0, os.WNOHANG)
> > except ChildProcessError:
> > return
> > """
> >
> > The current code will only wait children in the same process group, so
> > if a child process called setpgrp() or setsid() (which is common e.g.
> > for a daemon), the above code won't work as expected: is this wanted?
> >
>
> It sounds like nobody thought about this yet -- do you have a suggestio on
> how to fix it?

waitpid(-1, os.WNOHANG) perhaps?

I also don't understand why _sig_chld() reschedules itself using
call_soon (it should probably use call_soon_threadsafe(), by the way).

I also wonder how this signal handler interacts with the
ProcessExecutor in concurrent.futures (or with multiprocessing in
general).

How about another approach, such as spawning a dedicated thread that
will block on os.waitpid(<known child pid>) and will wake up the event
loop when it returns?
(if you're spawning a process, you can probably bear the cost of also
spawning an additional thread)

Regards

Antoine.

[Glyph]

On Oct 19, 2013, at 12:51 PM, Antoine Pitrou <soli...@pitrou.net> wrote:

How about another approach, such as spawning a dedicated thread that
will block on os.waitpid(<known child pid>) and will wake up the event
loop when it returns?
(if you're spawning a process, you can probably bear the cost of also
spawning an additional thread)

This is a bad idea for a number of reasons:

1. subprocess/thread interaction is subtle and tricky and you shouldn't rely on it if you can avoid it.  Some platforms have straight-up bugs in this area and it's best not to tickle them.  (Probably most relevant platforms are somewhat obsolete by now, but nevertheless, people have lots of old computers...)
2. A wedged / dead subprocess would now mean a wedged/dead super-process with a resource it can't get rid of.  Again with platform bugs: ever seen an oddly persistent <defunct> or <zombie> in 'ps' output?  That would then mean "dead, unjoinable thread forever", which can easily lead to deadlocks during shutdown; very unpleasant if your daemon's purpose in life is being the super-reliable component that spawns and manages other processes.
3. If you're writing a coordination daemon that spawns as many processes as it can manage, depending on ulimits, this may as much as halve the number of processes you can spawn.  On embedded devices it might be even worse than that.

-glyph

[Charles-François Natali]

2013/10/19 Antoine Pitrou <soli...@pitrou.net>:

>
> I also don't understand why _sig_chld() reschedules itself using
> call_soon (it should probably use call_soon_threadsafe(), by the way).

The current code is broken: if waitpid() returns 0 (some children are
not exited yet), it will keep rescheduling itself endlessly until all
children have exited. It's pretty easy to reproduce by spawning a
short-lived process and a long-lived one, asyncio will busy-loop.

I'll open an issue on the tracker.

[Charles-François Natali]

2013/10/19 Glyph <gl...@twistedmatrix.com>:

>
> On Oct 19, 2013, at 9:34 AM, Guido van Rossum <gu...@python.org> wrote:
>
> 1. Syscalls failing with EINTR *shouldn't* be a problem when using Tulip,
> the send/receive code deals with this, all calls have an except
> (BlockingIOError, InterruptedError) clause.
>
>
> But you *do* still have people calling write() and read() though, right?
> Like, for log files and stuff? And in database drivers? Or maybe when
> importing a module? I don't believe the import system is signal safe :-).
> You will definitely see explosions all over the place from library code if
> you don't address this problem.
>
> Luckily it's super easy to address in these modern times! Just call
> signal.siginterrupt(signal.SIGCHLD, False) and it's solved.

Except that some syscalls are not restartable:
"""
>>> signal(SIGALRM, lambda *args: None)
0
>>> siginterrupt(SIGALRM, False)
>>> alarm(10)
0
>>> select([], [], [], 60)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
InterruptedError: [Errno 4] Interrupted system call
"""

Really, EINTR should be handled by the interpreter/stdlib (yeah, I
know I signed for that, I've just been really busy recently...), but
that could help in the meantime for restartable syscalls.

[Glyph]

On Oct 20, 2013, at 3:53 AM, Charles-François Natali <cf.n...@gmail.com> wrote:

Really, EINTR should be handled by the interpreter/stdlib (yeah, I
know I signed for that, I've just been really busy recently...), but
that could help in the meantime for restartable syscalls.

Just remember that as soon as you're done with that, you also need to go fix every C library that has ever called 'read' or 'write' (both proprietary and open source) which might be wrapped by Python...

Go ahead.  I'll wait.  ;-)

-glyph

[Antoine Pitrou]

On Mon, 21 Oct 2013 12:24:22 -0700
Glyph <gl...@twistedmatrix.com> wrote:

>
> On Oct 20, 2013, at 3:53 AM, Charles-François Natali <cf.natali-Re5JQE...@public.gmane.org> wrote:
>
> > Really, EINTR should be handled by the interpreter/stdlib (yeah, I
> > know I signed for that, I've just been really busy recently...), but
> > that could help in the meantime for restartable syscalls.
>
> Just remember that as soon as you're done with that, you also need to go fix every C library that has ever called 'read' or 'write' (both proprietary and open source) which might be wrapped by Python...
>
> Go ahead. I'll wait. ;-)

Don't worry, we'll interrupt you :)

Regards

Antoine.

[Charles-François Natali]

2013/10/21 Glyph <gl...@twistedmatrix.com>:

>
> Just remember that as soon as you're done with that, you also need to go fix
> every C library that has ever called 'read' or 'write' (both proprietary and
> open source) which might be wrapped by Python...
>
> Go ahead. I'll wait. ;-)

Well, we can't be held responsible for every piece of broken code out
there, and since as noted above not all syscalls are restartable,
that's basically a lost battle as soon as we setup a signal handler.

Anyway, that's indeed something to try to tackle in a not-too-distant future :-)

[Anthony Baire]

Le 19/10/2013 18:34, Guido van Rossum a écrit :

The subprocess code is definitely not as baked as I'd like it to be -- I haven't had enough time to review and try it. See e.g. http://code.google.com/p/tulip/issues/detail?id=68, that issue light be related.

 

Otherwise, what's the rationale behind this code?

It just sounds odd to have to do explicit regular polling for every known subprocess when the SIGCHLD feature was designed to notify you when you should poll 

There is an intermediate solution : polling every child upon SIGCHLD. I noticed that this is what glib does.
https://git.gnome.org/browse/glib/plain/glib/gmain.c (in dispatch_unix_signals())

Also, using waitpid(0) (or waitpid(-1)) implies dealing with a race condition when spawning a process from a secondary thread (different from issue 68). The _sig_chld() handler may be run before the pid is registered in self._subprocesses.

I'm thinking about a couple things:
1. registering a signal handler for SIGCHLD makes it much more likely
to have syscalls failing with EINTR
2. by waiting for all children indiscriminately, this will make some
user code calling waitpid() on child processes they spawned fail with
ECHILD (subprocess is guarded against this, but I expect some
third-party code might not be)

I guess the reason is to avoid having to call waitpid() on multiple
PIDs at a periodic interval...

1. Syscalls failing with EINTR *shouldn't* be a problem when using Tulip, the send/receive code deals with this, all calls have an except (BlockingIOError, InterruptedError) clause.

2. I guess this is an interoperability issue between Tulip and code that manually spawns subprocesses (and bypassing the subprocess module). I expect that would rarely be an issue.

There may be code spawning processes from C modules.

Also I don't know if it is relevant here, but there is an interoperability issue with other PEP 3156 implementations when they are mixed together in the same process (say you reuse code relying on different event loops (glib, qt, whatever)). The current code around the sigchld handler assumes that the main thread is running a tulip loop, and inter-loop communication (registering the child pid) is done only through a private attribute (loop._subprocesses).

Perhaps we can design the subprocess handling so that you can turn off the SIGCHLD handler and in that case let the event loop poll for specific children repeatedly?

I would opt for polling every child in the sigchld handler by default. This is safe and with a reduced overhead. Using waitpid(0) could be proposed as an option for those having a huge number of processes.


Still regarding the SIGCHLD, I noticed another issue. The _sig_chld() handler is registered on the fly when the first subprocess is started. If my understanding of add_signal_handler() is correct, this will fail if the first subprocess is started from a secondary thread.

Anthony

[Anthony Baire]

Hi all,

I am posting an attempt to isolate the SIGCHLD handler from the event loop.

https://codereview.appspot.com/16700044/

The purpose is twofold:
1. clarify the interface between different event loops (there can be
only one SIGCHLD handler)
2. allow reimplementing the sigchld handler using a different strategy
(waitpid(-1), polling, blocking waitpid in a separate thread, ...)

I included two implementations: one which calls waitpid(-1) and one
which avoids it.

Point 1. is needed to allow multiple event loop implementations to
interoperate within the same process. I wrote an implementation for the
GLib event loop too:
https://bitbucket.org/a_ba/gbulb/commits/2f409adad942107afb7bc96994235e30caa4c17c


Regards
Anthony

[Guido van Rossum]

Hey Anthony, this looks very promising. Unfortunately it looks like a bug in Rietveld made the code review unusable (there's no base rev) -- I think this is fixed now so you can just re-upload. Also your patch seems based on a week-old version of Tulip, and one chunk of your changes to unix_events.py doesn't apply (though I think you are just deleting the changed code :-).

Could you freshen your repo, merge/resolve, and create a new Rietveld issue? (Also you have some style issues like long lines but I'll point those out in the code review once it's been uploaded.)

I'm also interested in CF's view on your code.

[Anthony Baire]

I updated the patch. It should be better now.
Anthony