subprocess leaks open file descriptors between Popen instances causing hangs

[milkokrachounov]

BPO	7213
Nosy	@birkenfeld, @gpshead, @pfmoore, @vstinner, @jwilk, @ned-deily
Files	subprocess-cloexec-py3k.patch: [obsolete] Simple patch to enable CLOEXEC on subprocess pipes subprocess-00-subprocess_test_utils.patch: Utils required by the tests of the CLOEXEC patches (issue 6559 could also use them for tests) subprocess-01-atomic_cloexec_pipe2.patch: Create subprocess pipes with FD_CLOEXEC (atomically when possible) subprocess-02-cloexec_tests.patch: Tests for FD_CLOEXEC on the subprocess pipes

^{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 = 'https://github.com/gpshead'
closed_at = <Date 2010-12-14.13:47:59.664>
created_at = <Date 2009-10-26.23:14:01.036>
labels = ['type-bug', 'library']
title = 'subprocess leaks open file descriptors between Popen instances causing hangs'
updated_at = <Date 2010-12-14.13:48:51.110>
user = 'https://bugs.python.org/milkokrachounov'

bugs.python.org fields:

activity = <Date 2010-12-14.13:48:51.110>
actor = 'gregory.p.smith'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2010-12-14.13:47:59.664>
closer = 'gregory.p.smith'
components = ['Library (Lib)']
creation = <Date 2009-10-26.23:14:01.036>
creator = 'milko.krachounov'
dependencies = []
files = ['20000', '20017', '20018', '20020']
hgrepos = []
issue_num = 7213
keywords = ['patch']
message_count = 28.0
messages = ['94538', '100372', '123408', '123737', '123741', '123742', '123743', '123744', '123746', '123748', '123754', '123763', '123766', '123788', '123792', '123801', '123802', '123804', '123806', '123817', '123818', '123832', '123837', '123863', '123864', '123869', '123940', '123941']
nosy_count = 8.0
nosy_names = ['georg.brandl', 'gregory.p.smith', 'paul.moore', 'vstinner', 'jwilk', 'ned.deily', 'milko.krachounov', 'Giovanni.Bajo']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue7213'
versions = ['Python 2.7', 'Python 3.2']

[milkokrachounov]

Currently, close_fds defaults to False. The are few cases in which one
would want to leave the fds open, in all the rest leaving them open can
lead to unpleasant side effects. For example, the following doesn't work:

>> p1 = Popen(['cat'], stdin=PIPE, stdout=PIPE)
>> p2 = Popen(['grep', 'a'], stdin=p1.stdout, stdout=PIPE)
>> p1.stdin.write("aaaaaaaaaaaaaaaa\n")
>> p1.stdin.close()
>> p2.stdout.read()

It would block forever, and it is not obvious that p1.stdin remains
open, because p2 was created without close_fds=True. On the other hand,
in each and every case where close_fds=True is required, the programmer
is aware that he needs to leave some fds open (and usually knows which fds).

The current default is harmful, because in each case where the file
descriptors are not explicitly needed in the child process, they can
cause problems hard to debug. It seems that only about 10% of the
current uses of Popen have close_fds=True.

I propose that the close_fds default is changed to True. Alternatively,
this could be combined with bug bpo-6559 by completely removing the
close_fds argument, and leaving only pass_fds, which could accept
subprocess.ALL_FDS as a value.

There are two issues with my proposal:

1. close_fds would have to be changed to give a warning each time it is
   not specified before the change of the default can be adopted. Otherwise
   it would break any programs relying on close_fds being False by default.
2. Closing fds has a slight performance impact.

However, I think that close_fds=True is much more sensible default. I
think it will be a good idea if at least py3k adopts the change.

[gpshead]

bpo-2320 is somewhat related to this.

[pfmoore]

This bug appears to be Unix-only. On Windows:

>>> from subprocess import *
>>> p1 = Popen(['cat'], stdin=PIPE, stdout=PIPE)
>>> p2 = Popen(['grep', 'a'], stdin=p1.stdout, stdout=PIPE)
>>> p1.stdin.write("aaaaaaaaaaaaaaaa\n")
>>> p1.stdin.close()
>>> p2.stdout.read()
'aaaaaaaaaaaaaaaa\n'
>>>

So there's no clear reason why the default should change on Windows. (It's not possible to specify close_fds explicitly on Windows for this case:

>>> p1 = Popen(['cat'], stdin=PIPE, stdout=PIPE, close_fds=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "D:\Apps\Python27\lib\subprocess.py", line 630, in __init__
    raise ValueError("close_fds is not supported on Windows "
ValueError: close_fds is not supported on Windows platforms if you redirect stdin/stdout/stderr

which may imply that False is not only reasonable, but necessary on Windows. I haven't dug into this enough to know for sure if this is the case, though).

[GiovanniBajo]

Hi Gregory, I saw your commit here:
http://code.activestate.com/lists/python-checkins/91914/

This basically means that in 3.2 it is mandatory to specify close_fds to avoid a DeprecationWarning. *BUT* there is no good value that works both on Windows and Linux if you redirect stdout/stderr, as shown in this bug.

So basically in 3.2 to avoid a warning, each and every usage of Popen() with a redirection should be guarded by an if that checks the platform. I don't think this is acceptable.

Have I misunderstood something? Also: can you please explain how the behaviour is going to change in 3.3? I assume that you are planning to change the default to True; but would that also cover Windows' singularity in redirection cases?

[milkokrachounov]

I'd offer two ideas.

1. Add a constant DISREGARD_FDS to the subprocess module could help. It would allow the user to specify his intent, and let the implementation choose the best action. Popen(..., close_fds=subprocess.DISREGARD_FDS) would mean that any fds additional fds are not needed, and can be closed if it is determined that it is the best course of action.

So, if DISREGARD_FDS gets passed on Windows assume close_fds=False, while on POSIX assume close_fds=True (although this has its downsides, see at the bottom).
2. Set the CLOEXEC flag on the other side of the pipes after the fork. With the attached patch, I don't get the issue I reported:

Python 3.2b1 (py3k:87158, Dec 10 2010, 20:13:57) 
[GCC 4.4.5] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from subprocess import *
>>> p1 = Popen(['cat'], stdin=PIPE, stdout=PIPE, close_fds=False)
>>> p2 = Popen(['grep', 'a'], stdin=p1.stdout, stdout=PIPE, close_fds=False)
>>> p1.stdin.write(b"aaaaaaaaaaaaaaaa\n")
17
>>> p1.stdin.close()
>>> p2.stdout.read()
b'aaaaaaaaaaaaaaaa\n'
>>> 

Additional problem with close_fds=True is that if MAXFD is huge, it can cause a huge problem with performance, so perhaps the default being True isn't all that great. On Linux you can close only the open fds, but I'm not sure if that's possible on other POSIX systems.

[milkokrachounov]

The cloexec approach still doesn't help with bpo-2320. In fact, with threading and people calling subprocess from multiple threads, *this* issue wouldn't be fixed with my patch either unless mutexes are used. It's impossible to avoid a race here with threads and no mutex as far as I can tell. It would mean that the creation of the pipe, the fork and the setting of the CLOEXEC flag needs to be done atomically... yikes.

[GiovanniBajo]

Setting CLOEXEC on the pipes seems like a very good fix for this bug. I'm +1 on it, but I think it should be the default; instead, your proposed patch adds a new argument to the public API. Why do you think it's necessary to do so?

At the same time, we need a solution to handle close_fds, because the current status of the 3.2 with the DeprecationWarning (on 90% of subprocess uses in the world, if it ever gets backported to 2.7) and no way to fix it in a multi-platform way is really sad.

I don't think a new constant DISREGARD_FDS is necessary. I think we can just use "None" as intermediate default (just like the current 3.2 does), and later switch it to True. The only further required action is to make "True" always work on Windows and never error out (just make it do nothing if there are some redirections), which is an obviously good thing to do to increase portability of subprocess.

Otherwise, if this can't make 3.2, I think the DeprecationWarning should be reverted until we agree on a different solution.

[milkokrachounov]

I'm +1 on it, but I think it should be the default; instead,
your proposed patch adds a new argument to the public API. Why do you
think it's necessary to do so?

I don't think it's necessary. I put it there because when I was testing I thought it might help. For example, you might want to keep the pipes open and then open another process with close_fds=False, thus changing the current default might cause some regressions in some software and an argument would allow an easier transition. I updated the patch removing anything unnecessary. Though it still has a huge race when used with threading.

[GiovanniBajo]

Would you mind elaborating on where is the race condition?

[milkokrachounov]

It's almost exactly the same race condition as the one described in bpo-2320. The pipes are created and stay without the CLOEXEC flag for a while (until the process has been forked and fcntl has been called). During that time another thread can launch a subprocess, the subprocess will inherit the pipe, and there might be a hang similar to the one described here (or in bpo-2320).

[ned-deily]

(Adding the 3.2 release manager: a potential release blocker?)

[milkokrachounov]

I created another patch that attempts to create the pipes atomically.

On GNU/Linux, if pipe2 is available, it uses it to create the pipes, and there is no race. On other POSIX platforms, pipe and fcntl are called without releasing the GIL - relying on the fact that _posixsubprocess.fork_exec doesn't release the GIL either, so the two can't run at the same time (bonus: os.fork doesn't release the GIL either). I can't reproduce neither bpo-7213 nor bpo-2320 with either implementation, so the patch seems to fix them.

Issues:

1. If the _posixsubprocess module isn't compiled, the race still exists (well, without it subprocess isn't safe to use with threads anyway).
2. On GNU/Linux systems where glibc was compiled with kernel headers >=2.6.27, but the running kernel is <2.6.27, the subprocess module won't work. (There should be a fix for that?)
3. I have no way to tell that the non-Linux implementation works for sure. I've been running it in an endless loop, and so far there have been no hangs (*), but that doesn't mean that it doesn't have a rare race that's beyond my comprehension. With pipe2() you can be certain, but I have my doubts about the other implementation.

All unit tests seem to pass.

() Actually, I *thought it hang on my first attempt, but I interrupted the process too soon to tell for sure. No hangs after that. :(