| 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 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) *  |
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) * |
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 . (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) * |
Date: 2008-08-07 13:49 |
| Hello. I had lecture about exception & frames on . 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 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: ? |
|
|
| msg93787 - (view) |
Author: Hirokazu Yamamoto (ocean-city) * |
Date: 2009-10-09 12:14 |
| No, this is not duplicate of . 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) * |
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) * |
Date: 2010-08-03 22:34 |
| Same problem in 3.1.2 |
|
|
| msg112968 - (view) |
Author: Tim Golden (tim.golden) * |
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) * |
Date: 2010-08-05 14:26 |
| Patch added for 31 branch |
|
|
| msg112985 - (view) |
Author: Tim Golden (tim.golden) * |
Date: 2010-08-05 14:27 |
| Patch added for 27 branch |
|
|
| msg112986 - (view) |
Author: Brian Curtin (brian.curtin) * |
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) * |
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) * |
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) * |
Date: 2010-08-06 14:14 |
| Committed in r83759 r83760 r83761 |
|
|
| msg113106 - (view) |
Author: Hirokazu Yamamoto (ocean-city) * |
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) * |
Date: 2010-08-06 16:19 |
| Blast. Thanks; I'll have to rework those patches then. |
|
|
| msg113249 - (view) |
Author: Tim Golden (tim.golden) * |
Date: 2010-08-08 11:16 |
| OK, the issue identified by Hirokazu Yamamoto in 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) * |
Date: 2010-08-08 16:15 |
| Committed in r83815, r83816, r83817 |
|
|