asyncio: child process exit isn't detected if its stdin/stdout/stderr FDs have been inherited by a child process

[neologix]

BPO	19326
Nosy	@gvanrossum

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2013-10-21.17:50:28.441>
created_at = <Date 2013-10-21.10:15:57.484>
labels = ['type-bug']
title = "asyncio: child process exit isn't detected if its stdin/stdout/stderr FDs have been inherited by a child process"
updated_at = <Date 2013-10-21.17:50:28.440>
user = 'https://bugs.python.org/neologix'

bugs.python.org fields:

activity = <Date 2013-10-21.17:50:28.440>
actor = 'neologix'
assignee = 'none'
closed = True
closed_date = <Date 2013-10-21.17:50:28.441>
closer = 'neologix'
components = []
creation = <Date 2013-10-21.10:15:57.484>
creator = 'neologix'
dependencies = []
files = []
hgrepos = []
issue_num = 19326
keywords = []
message_count = 3.0
messages = ['200744', '200812', '200816']
nosy_count = 2.0
nosy_names = ['gvanrossum', 'neologix']
pr_nums = []
priority = 'normal'
resolution = 'rejected'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue19326'
versions = ['Python 3.4']

[neologix]

(This is a spinoff from http://bugs.python.org/issue19293#msg200598)

When SIGCHLD is received, _sig_chld() is executed:

    def _sig_chld(self):
        [...]
        transp = self._subprocesses.get(pid)
        if transp is not None:
            transp._process_exited(returncode)

Then, here's _process_exited():

    def _process_exited(self, returncode):
        assert returncode is not None, returncode
        assert self._returncode is None, self._returncode
        self._returncode = returncode
        self._loop._subprocess_closed(self)
        self._call(self._protocol.process_exited)
        self._try_finish()

And here's _try_finish():

    def _try_finish(self):
        assert not self._finished
        if self._returncode is None:
            return
        if all(p is not None and p.disconnected
               for p in self._pipes.values()):
            self._finished = True
            self._loop.call_soon(self._call_connection_lost, None)

Thus, _UnixSubprocessTransport protocol's connection_lost is only
called if the all() expression is true:
and it's true only if all the subprocess pipes have been disconnected
(or if we didn't setup any pipe).

Unfortunately, this might very well never happen: imagine that the
subprocess forks a process: this grand-child process inherits the
child process's pipes, so when the child process exits, we won't
receive any notification, since this grand-child process still has
open FDs pointing to the original child's stdin/stdout/stderr.

The following simple test will hang until the background 'sleep' exits:
"""
diff -r 47618b00405b Lib/test/test_asyncio/test_events.py
--- a/Lib/test/test_asyncio/test_events.py Sat Oct 19 10:45:48 2013 +0300
+++ b/Lib/test/test_asyncio/test_events.py Sun Oct 20 19:32:37 2013 +0200
@@ -1059,6 +1059,23 @@

 @unittest.skipIf(sys.platform == 'win32',
                  "Don't support subprocess for Windows yet")

+ def test_subprocess_inherit_fds(self):
+ proto = None
+
+ @tasks.coroutine
+ def connect():
+ nonlocal proto
+ transp, proto = yield from self.loop.subprocess_shell(
+ functools.partial(MySubprocessProtocol, self.loop),
+ 'sleep 60 &')
+ self.assertIsInstance(proto, MySubprocessProtocol)
+
+ self.loop.run_until_complete(connect())
+ self.loop.run_until_complete(proto.completed)
+ self.assertEqual(0, proto.returncode)
+
+ @unittest.skipIf(sys.platform == 'win32',
+ "Don't support subprocess for Windows yet")
def test_subprocess_close_after_finish(self):
proto = None
transp = None
"""

If waitpid() returns a process's PID, then the process is done,
there's no reason to further wait for pipe's disconnection: they can
be used as a hint that the process terminated, but there's definitely
not required...

[gvanrossum]

This is by design. Let me try to defend the design.

As long as one of the pipes is still open the parent might be interested in it. The Protocol object does get notified of the process's exit, via process_exited(), and if at that point it wants to be done, it can close the pipes itself. (To do that, call transport.get_pipe_transport(FD).close().) Once that's done the subprocess protocol's connection_lost() method will be called.

I suppose an alternative approach might be to assume that when the subprocess exist the parent should close the pipes and be done, but there's a race condition there: there may still be data in one of the pipes (stdout or stderr) that should be processed before calling connection_lost(), similar to how we delay the socket connection_lost() call until we have processed all data read from it.

So I don't think that alternative is viable.

[neologix]

This is by design. Let me try to defend the design.

OK, if that's a know limitation, then that's fine.
It would be nice to add maybe a note somewhere in the documentation, so that people don't get bitten by this (and also probably add a test for process_exited).