| msg75530 - (view) |
Author: Ammon Riley (ammon_riley) |
Date: 2008-11-05 20:24 |
| If the disk fills up during the copy operation, shutil.copyfile() leaks file descriptors. The problem is the order of the close() statements in the finally block. In the event the copy operation runs out of disk space, the fdst.close() call triggers an IOError, which prevents the fsrc.close() call from being called. Swapping the two calls, so that fsrc is closed first prevents this issue, though it doesn't solve the underlying problem that IOErrors raised during the close() operation will prevent the second close() from being called. A probably better solution: def copyfile(src, dst): """Copy data from src to dst""" if _samefile(src, dst): raise Error, "`%s` and `%s` are the same file" % (src, dst) fsrc = None fdst = None try: fsrc = open(src, 'rb') fdst = open(dst, 'wb') copyfileobj(fsrc, fdst) finally: try: if fsrc: fsrc.close() finally: if fdst: fdst.close() |
|
|
| msg104798 - (view) |
Author: Tres Seaver (tseaver) * |
Date: 2010-05-02 19:42 |
| This bug exists on Python 2.6, too. It seems to me that the right solution here is to use both opened files as context managers. See attached patch (made against the release26-maint branch). The patch also cleans up the old-style exception signature in the precondition check. |
|
|
| msg104799 - (view) |
Author: Tres Seaver (tseaver) * |
Date: 2010-05-02 19:48 |
| The patch doesn't apply cleanly to trunk. Attached is one which does. |
|
|
| msg104990 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2010-05-05 00:44 |
| Could you write a test? Use a fake file objects that raise (or not) an IOError on close(), and then check that close() was closed on both files. There are 4 cases: input.close() raises or not an exception, output.close() raises or not an exception. |
|
|
| msg105053 - (view) |
Author: Tres Seaver (tseaver) * |
Date: 2010-05-05 15:51 |
| I would be glad to write those tests, if you could explain the strategy you have in mind more fully: since 'shutil.copyfile' performs the 'open' itself, I couldn't figure out how to stub in such a mocked up file. Does 'open' provide a hook for testing purposes that I don't know about? |
|
|
| msg105079 - (view) |
Author: Ammon Riley (ammon_riley) |
Date: 2010-05-05 19:34 |
| You can replace the built-in open(), with one of your own devising: >>> import shutil >>> def open(*a, **k): ... raise IOError("faked error.") ... >>> __builtins__.open = open >>> shutil.copyfile("snake", "egg") Traceback (most recent call last): File "", line 1, in File "/usr/lib/python2.6/shutil.py", line 52 in copyfile fsrc = open(src, 'rb') File "", line 2, in open IOError: faked error. Note that your open() replacement will need a bit of smarts, since it needs to succeed for some open() calls, and fail for others, so you'll want to stash the original __builtins__.open() for future use. |
|
|
| msg105090 - (view) |
Author: Tres Seaver (tseaver) * |
Date: 2010-05-05 21:59 |
| This patch adds tests for the four edge cases (opening source fails, opening dest fails, closing dest fails, closing source fails). |
|
|
| msg105105 - (view) |
Author: Tarek Ziadé (tarek) * |
Date: 2010-05-05 22:43 |
| committed in r80830, r80831, r80833 and r80835 Thanks all ! |
|
|
| msg105113 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2010-05-06 00:31 |
| Wow, nice trick (shutil.open = func) :-) |
|
|