Author milko.krachounov
Recipients Giovanni.Bajo, georg.brandl, gregory.p.smith, jwilk, milko.krachounov, ned.deily, paul.moore, vstinner
Date 2010-12-11.22:26:07
SpamBayes Score 1.11022e-16
Marked as misclassified No
Message-id <1292106369.86.0.673925197274.issue7213@psf.upfronthosting.co.za>
In-reply-to
Content
OK, I have created new updated patches. I haven't combined them in one patch because some of the changes can be applied independently, the three patches can be cat'ed together if anyone sees separate patches a problem. ;)

I. Changes:
* Now pipe2 would be used on all systems where available.
* I don't depend on external grep and cat, but use utils shipped with the tests.

II. The patches:
1. subprocess-00-subprocess_test_utils.patch
This contains the fd_status.py and input_reader.py utilities that are needed by the tests. A separate file because they can be used for creating unit tests for issue 6559. In case these patches aren't accepted, it could be useful there.

2. subprocess-01-atomic_cloexec_pipe2.patch
This file contains the changes that make the subprocess pipes created with the FD_CLOEXEC flag (atomically through pipe2) where possible. Can be used independently of the rest
of the patches.

3. subprocess-02-cloexec_tests.patch
This file contains the tests that verify that CLOEXEC is set on the pipes created by subprocess and this allows complex pipes between apps to work correctly. Requires subprocess-00-subprocess_test_utils.patch. It can be used for another implementation of CLOEXEC in case this implementation isn't accepted. With slight modification it can be used to test that pipes work fine with close_fds=True and no CLOEXEC.

III. Atomicity and issue 2320:

> I don't understand why do you talk about "atomicity". Do you test add non-atomic 
> operations? Was subprocess atomic?
>
> If I understood correctly, you are fixing a specific issue which can be called 
> something like "subprocess: close pipes on exec(), set FD_CLOEXEC flag to all pipes", 
> and no more changing the default value of close_fds. Can you update the title please?

The CLOEXEC flag needs to be set atomically (or at least in a way that another subprocess won't start in the middle of it) so that issue 2320 is also resolved. This is why I suggested the use of pipe2. On systems where pipe2 isn't available, the patches rely on the coincidence that the GIL during both process execution and the pipe creation. I was wondering if a tests that verify for things like issue 2320 are wanted in general (there's no way to reliably test such, but there could be a test that does it with a certain probability).

In short, yes, the suggested patches attempt to solve this issue without a change to the default, but also to resolve issue 2320 (changing the default would also resolve it).

III. More issues:

1. What other possible situations exist causing the same issue that can be solved with close_fds=True and are caused by resources coming from other modules? Are any of those common? Maybe something involving os.openpty() and subprocess? Does anyone use those together? Are such issues worth worrying about (in other words, is changing the default still necessary if the subprocess pipes have the FD_CLOEXEC flag?)
2. What possible side-effects could the FD_CLOEXEC flag have? Any common use-case that is broken by the fact that the pipes are now with FD_CLOEXEC?
3. Can any changes be done to Python 2.7's subprocess? Can the issue be fixed there too, or it will have to remain as it is now?
History
Date User Action Args
2010-12-11 22:26:09milko.krachounovsetrecipients: + milko.krachounov, georg.brandl, gregory.p.smith, paul.moore, vstinner, jwilk, ned.deily, Giovanni.Bajo
2010-12-11 22:26:09milko.krachounovsetmessageid: <1292106369.86.0.673925197274.issue7213@psf.upfronthosting.co.za>
2010-12-11 22:26:07milko.krachounovlinkissue7213 messages
2010-12-11 22:26:07milko.krachounovcreate