make fork + exec non blocking on unix

[Martiusweb]

Dear all,

Following the bug #414, here is a patch which makes the fork + exec operation non blocking on Unix. The patch is probably not ready to land, but I believe it's in pretty good shape.

This patch require a few changes in cpython (subprocess.Popen), which are included in this patch for discussion (in tmp_subprocess, as a child class of subprocess.Popen). Those changes have been tested with cpython unittests on my laptop. The whole patch has been tested with Python 3.5.2 on Linux, so the CI may report undetected errors for other configurations.

The problem: subprocess.Popen() will fork and synchronously read on a pipe until it is closed as a way to wait and check the result of the exec() call in the new child process. All of this is done in the constructor of Popen. This is an issue when the preexec_fn (user function called before exec) or exec() is slow.

This PR does the following:

• breaks the private Popen._execute_child() method into several methods so we can isolate the blocking reads (on the fd errpipe_read) in a (temporary) child class tmp_subprocess._Popen.
• introduces _NonBlockingPopen, a version of _Popen which uses the loop and a future to signal the caller when the exec() is done (or failed). It means that an error will usually be reported by the future instead of raised when the _NonBlockingPopen object is created. It also means that to follow the (implicit) contract of Popen, _NonBlockingPopen is responsible of cleaning resources it opened if an error occurs before exec(). Note that _NonBlockingPopen also uses sockets instead of pipes (following what was done in _UnixSubprocessTransport._start()).
• since it is now required to wait on a future (but only on unix so far), _BaseSubprocessTransport._start() can be a coroutine. The implementation of the transport has been modified to support this change: the fork is now done in a task scheduled by the constructor. This doesn't change much for the caller (_make_subprocess_transport()) which awaited a future reporting errors during the transport initialization or is done once the subprocess transport is in a stable condition (_connect_pipes() finished). Special care has been taken to possible races conditions due to cancellations (usually when loop.subprocess_exec/shell() are canceled).
• loop._make_subprocess_transport() doesn't change much. The biggest change is that the child_watcher is handled by the transport itself. This is required because we don't want this method to handle the process termination if something failed before the subprocess exec-ed, while now _NonBlockingPopen still have to handle this asynchronously.
• Two tests have been added: one handling a new possible race condition (cancellation before exec), and the other one ensuring that _NonBlockingPopen will eventually raise an exception if something wrong happens in preexec_fn (this was previously the responsibility of Popen).
• One test have been modified (test_proc_exited): it assumed that the transport was ready before the waiter was done. I believe this assumption is incorrect, and doesn't hold true now that the _BaseSubprocessTransport._proc attribute is set asynchronously (instead of in the constructor).

This is my biggest PR for asyncio so far, and my first attempt at patching cpython, I'm eager to receive feedback and comments about the patch and about how it can successfully land in the codebase (for instance, should I get in touch with the owner of cpython's subprocess?).

Thanks for your time, Martin

[gvanrossum]

@gpshead This seems your territory -- do you have time to look into this? The ideas seem solid.

[gvanrossum]

Thank you @Martiusweb for this patch. There are a lot of changes here and there are many concerns. For example the asyncio library should work with Python 3.3 and up (at least the version from this repo -- the one in the CPython repo only needs to work with the version it's part of, but we try to keep the source differences minimal). My other overarching concern is that we're already in feature freeze for Python 3.6 (3.6b1 went out a few weeks ago) and while this is technically not a new feature it certainly is a complex thing to land close to a release. You should probably also create an issue in bugs.python.org with at least a patch for subprocess.py, and a reference here.

[1st1]

+1 to what @gvanrossum said. I can take a look at the patch in a couple of weeks (assuming you fix the CI). We can probably aim for 3.6.1 to have this.

[Martiusweb]

Thank you for your feedback.

I pushed a commit which adds compatibility with Python 3.3. It adds a tmp_subprocess33 module which is the Popen patch backported for python 3.3. The modules tmp_subprocess and tmp_subprocess33 can probably be merged, but I guess that it can wait until we know how things will evolve on the cpython side.

The CI is not yet fixed on windows (I don't have a windows setup to work on yet). I expect (...hope) that the bug which make the tests hang will be easy to fix as there is not much change in the windows code path.

I also opened an issue on bugs.python.org about the changes in Popen: http://bugs.python.org/issue28287

I'm all in favor for not rushing the patch: I prefer if I can make it in python 3.6.1 or later instead of seeing it reverted because something was missing.

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

Unfortunately we couldn't find an account corresponding to your GitHub username at bugs.python.org (b.p.o). If you don't already have an account at b.p.o, please create one and make sure to add your GitHub username. If you do already have an account at b.p.o then please go there and under "Your Details" add your GitHub username.

And in case you haven't already, please make sure to sign the PSF contributor agreement (CLA); we can't legally look at your contribution until you have signed the CLA.

Once you have done everything that's needed, please reply here and someone will verify everything is in order.

[Martiusweb]

Hi,

I updated the PR: if my patch on Popen (submitted on b.p.o) is merged, asyncio will detect that it's able to make a NonBlockingPopen and use it, else, it will use Popen and block, but work as it used to. When it's done, I will remove the module tmp_subprocess (which monkey patches Popen) so the PR will be ready fro review.

@the-knights-who-say-ni My account on b.p.o is martius, I add my github username there. I signed the CLA. Thanks !

[gvanrossum]

Can you link to the specific issue you opened for your CPython patch?

[Martiusweb]

Yes, this is this one: http://bugs.python.org/issue28287

[gvanrossum]

Oh, so that's scheduled for Python 3.7.