classification
Title: subprocess.Popen does not release process handles if process cannot be started
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.1, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: tim.golden Nosy List: brian.curtin, gjb1002, gregory.p.smith, markmentovai, ocean-city, terry.reedy, tim.golden, twhitema
Priority: normal Keywords: patch

Created on 2008-06-26 15:22 by gjb1002, last changed 2010-08-08 16:15 by tim.golden. This issue is now closed.

Files
File name Uploaded Description Edit
sp-3.py tim.golden, 2008-06-26 17:26 Reduced example of subprocess handle error
adhok.patch ocean-city, 2008-08-07 13:49
3210.py markmentovai, 2008-12-08 20:53
3210.r83741.patch tim.golden, 2010-08-05 10:21
3210.release31-maint.patch tim.golden, 2010-08-05 14:26
3210.release27-maint.patch tim.golden, 2010-08-05 14:27
issue3210_py3k.diff brian.curtin, 2010-08-05 14:55
Messages (21)
msg68787 - (view) Author: Geoffrey Bache (gjb1002) Date: 2008-06-26 15:22
Run the following code on Windows:

import subprocess, os

file = open("filename", "w")
try:
    proc = subprocess.Popen("nosuchprogram", stdout=file)
except OSError:
    file.close()
    os.remove("filename")

This produces the following exception:

Traceback (most recent call last):
  File "C:\processown.py", line 10, in <module>
    os.remove("filename")
WindowsError: [Error 32] The process cannot access the file because it
is being used by another process: 'filename'

When the CreateProcess call fails the subprocess module should release
the handles it provides. Unfortunately it seems to raise WindowsError
before doing this.

See also
http://groups.google.com/group/comp.lang.python/browse_thread/thread/6157691ea3324779/6274e9f8bc8a71ee?hl=en#6274e9f8bc8a71ee

As Tim Golden points out, this can be worked around by doing
os.close(file.fileno()) at the end instead of file.close()
msg68796 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2008-06-26 17:26
The attached file sp-3.py simulates what I think is happening within the
subprocess module. Note that the OS handle is duplicated to allow
inheritance and then left unclosed on failure. If it is explicitly
closed, the file can be removed.

There is no hope of closing the file handle since it was local to the
__init__ method which failed but was not closed before exit and is now
inaccessible.

On the surface, the obvious fix would be a try/except block around the
CreateProcess call (or its caller) which would then release whatever
handles were needed. I'll try to get the time to put a patch together,
but it would be useful to have confirmation of my theory.
msg68817 - (view) Author: Geoffrey Bache (gjb1002) Date: 2008-06-27 06:56
A note on workarounds, the garbage collector seems to release the
handles when the function exits, so removing the file in a caller works
for me. However Tim's proposed fix with os.close didn't do so.
msg70679 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2008-08-04 03:16
I tried closing -all- of the handles listed here (the std ones used as
input to _get_handles and the pipe read/write ones returned) on an
OSError during CreateProcess but the problem still occurs for me using
the test code in msg68787.

        (p2cread, p2cwrite,
         c2pread, c2pwrite,
         errread, errwrite) = self._get_handles(stdin, stdout, stderr)

someone with more windows willingness/knowledge/fu should take this one on.
msg70826 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2008-08-07 13:49
Hello. I had lecture about exception & frames on issue3515.

When Lib/subprocess.py (814)'s CreateProcess() raises exception,
p2cread will be freed by refcount GC, but it never happen before
os.remove() because sys.exc_traceback holds reference to frame
which has p2cread.

import subprocess, os

file = open("filename", "w")
try:
    proc = subprocess.Popen("nosuchprogram", stdout=file)
except OSError:
    pass

try:
    raise RuntimeError() # hack to clear previous exc_traceback
except:
    pass

file.close()
os.remove("filename") # runs fine

So, I think subprocess module's practice which relys on refcount GC
is not good. (p2cread is PC/_subprocess.c 's sp_handle_object, so
automatically freed by refcount GC) I don't think attached "adhok.patch"
is enough, but seems to fix this issue at least.
msg77340 - (view) Author: Mark Mentovai (markmentovai) Date: 2008-12-08 20:53
This is not limited to Windows.  I experience this bug on Mac OS X and
Linux as well.

http://mail.python.org/pipermail/python-list/2008-August/505753.html

Attachment 3210.py is a reduced testcase.  It attempts to execute nocmd,
which should not exist.  The expected behavior is for OSError to be
thrown each time, with ENOENT or EACCES set in the errno field,
depending on the environment.  Due to this bug, Python will hit the file
descriptor limit at some point instead.

For quick reproduction, set a low but reasonable limit on the number of
maximum open files:

mark@anodizer bash$ ulimit -n 256
mark@anodizer bash$ python 3210.py
250
Traceback (most recent call last):
  File "3210.py", line 11, in <module>
    raise e
OSError: [Errno 24] Too many open files
msg91989 - (view) Author: Todd Whiteman (twhitema) Date: 2009-08-26 23:29
Is this a duplicate of this already fixed issue: issue5179 ?
msg93787 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-10-09 12:14
No, this is not duplicate of issue5179. That issue described handle was
leaked when exception occurred. But this issue is not *leak*. See
following code.

import subprocess, os, sys
file = open("filename", "w")
try:
    proc = subprocess.Popen("nosuchprogram", stdout=file)
except OSError:
    file.close()
    sys.exc_clear() # here we go....
    os.remove("filename") # now we can delete file!

subprocess is implemented using sp_handle_type in PC/_subprocess.c
(windows). This object is in exception stack frame(?), so handle lives
until another exception occurs or explicitly sys.exc_clear() is called.
msg93788 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-10-09 12:26
Probably we can fix this issue by calling Close() of sp_handle_type
somewhere in Lib/subprocess.py, but I have no patch now.
msg112711 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2010-08-03 22:34
Same problem in 3.1.2
msg112968 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2010-08-05 10:21
Patch attached with code & test which fixes this within _subprocess.c at least for Windows
msg112984 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2010-08-05 14:26
Patch added for 31 branch
msg112985 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2010-08-05 14:27
Patch added for 27 branch
msg112986 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-08-05 14:55
Tim,
I updated your test to use some of the newer and preferred unittest features and made a change to do the common stuff in loops. The _subprocess.c changes look fine to me. See attached issue3210_py3k.diff.
msg112987 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2010-08-05 15:09
Brian, I'm not sure that the test as rewritten will exercise the error.

The key is that the traceback object will prevent the handles
from being finalised until it is itself finalised. After your
change I expect the handles to release anyway since the traceback
is already finalised.

In other words, I'm not testing that an exception is raised (which
is what the with assertRaises would suggest); I'm testing that
*within the lifetime of the exception* it's still possible to
remove the files which were passed as stdin/out/err where it wasn't
previously.
msg112988 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-08-05 15:12
Ah ok. I got hooked onto new unittest stuff and overdid it. Whoops.

In that case, I guess just the last lines converting your "assert_" to "assertFalse" would be my only suggestion.
msg113102 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2010-08-06 14:14
Committed in r83759 r83760 r83761
msg113106 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2010-08-06 15:57
Sorry for posting to closed entry, but I think handle should be closed in Lib/subprocess.py not in PC/_subprocess.c. I noticed following code showed strange error.

import subprocess

for _ in xrange(2):
    stdout = open("stdout.txt", "w")
    try:
        p = subprocess.Popen(["unknown"], stdout=stdout)
    except WindowsError:
        pass

// error

close failed in file object destructor:
IOError: [Errno 9] Bad file descriptor
msg113107 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2010-08-06 16:19
Blast. Thanks; I'll have to rework those patches then.
msg113249 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2010-08-08 11:16
OK, the issue identified by Hirokazu Yamamoto in msg113106 only actually affects the 2.x series, because of the awkwardly multiple-level interaction between file handles. The io rewrite in 3.x seems not to suffer the same way. Nonetheless, the proposed adhok.patch (slightly updated to match the current codebase) seems to cover all the cases, and is rather more elegant. So I'll revert the C module changes and apply that instead to all branches.
msg113277 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2010-08-08 16:15
Committed in r83815, r83816, r83817
History
Date User Action Args
2010-08-08 16:15:14tim.goldensetstatus: open -> closed

messages: + msg113277
stage: patch review -> resolved
2010-08-08 11:16:36tim.goldensetmessages: + msg113249
2010-08-06 16:19:24tim.goldensetstatus: closed -> open

messages: + msg113107
2010-08-06 15:57:28ocean-citysetmessages: + msg113106
2010-08-06 14:14:23tim.goldensetstatus: open -> closed
resolution: fixed
messages: + msg113102
2010-08-05 15:12:42brian.curtinsetmessages: + msg112988
2010-08-05 15:09:09tim.goldensetmessages: + msg112987
2010-08-05 14:55:48brian.curtinsetstage: test needed -> patch review
2010-08-05 14:55:25brian.curtinsetfiles: + issue3210_py3k.diff
nosy: + brian.curtin
messages: + msg112986

2010-08-05 14:27:16tim.goldensetfiles: + 3210.release27-maint.patch

messages: + msg112985
2010-08-05 14:26:44tim.goldensetfiles: + 3210.release31-maint.patch

messages: + msg112984
2010-08-05 10:21:25tim.goldensetfiles: + 3210.r83741.patch
assignee: tim.golden
messages: + msg112968
2010-08-03 22:34:26terry.reedysetversions: + Python 3.1, Python 2.7, Python 3.2, - Python 2.6, Python 2.5
nosy: + terry.reedy

messages: + msg112711

stage: test needed
2009-10-09 12:26:11ocean-citysetmessages: + msg93788
2009-10-09 12:14:05ocean-citysetmessages: + msg93787
2009-08-26 23:29:21twhitemasetnosy: + twhitema
messages: + msg91989
2008-12-08 20:53:25markmentovaisetfiles: + 3210.py
nosy: + markmentovai
messages: + msg77340
2008-08-07 13:55:21ocean-citysetmessages: - msg70827
2008-08-07 13:52:24ocean-citysetmessages: + msg70827
2008-08-07 13:49:14ocean-citysetfiles: + adhok.patch
keywords: + patch
messages: + msg70826
2008-08-07 07:14:36ocean-citysetnosy: + ocean-city
2008-08-04 03:16:23gregory.p.smithsetassignee: gregory.p.smith -> (no value)
messages: + msg70679
2008-07-07 04:56:39gregory.p.smithsetpriority: normal
assignee: gregory.p.smith
nosy: + gregory.p.smith
components: + Library (Lib), - Extension Modules
versions: + Python 2.6
2008-06-27 06:56:43gjb1002setmessages: + msg68817
2008-06-26 17:26:02tim.goldensetfiles: + sp-3.py
nosy: + tim.golden
messages: + msg68796
2008-06-26 15:22:42gjb1002create