[Python-checkins] cpython (merge 3.3 -> default): Issue #19077: tempfile.TemporaryDirectory cleanup no longer fails when

Mon Jan 27 10:33:43 CET 2014

http://hg.python.org/cpython/rev/d792eb3afa58
changeset: 88772:d792eb3afa58
parent: 88770:0eec47f9f8a1
parent: 88771:50aa9e3ab9a4
user: Serhiy Storchaka <storchaka at gmail.com>
date: Mon Jan 27 11:21:54 2014 +0200
summary:
 Issue #19077: tempfile.TemporaryDirectory cleanup no longer fails when
called during shutdown. Emitting resource warning in __del__ no longer fails.
Original patch by Antoine Pitrou.
files:
 Lib/tempfile.py | 90 ++++++++------------------
 Lib/test/test_tempfile.py | 86 +++++++++++--------------
 Misc/NEWS | 4 +
 3 files changed, 72 insertions(+), 108 deletions(-)

diff --git a/Lib/tempfile.py b/Lib/tempfile.py
--- a/Lib/tempfile.py
+++ b/Lib/tempfile.py
@@ -29,11 +29,12 @@
 
 import functools as _functools
 import warnings as _warnings
-import sys as _sys
 import io as _io
 import os as _os
+import shutil as _shutil
 import errno as _errno
 from random import Random as _Random
+import weakref as _weakref
 
 try:
 import _thread
@@ -335,10 +336,12 @@
 underlying file object, without adding a __del__ method to the
 temporary file."""
 
+ file = None # Set here since __del__ checks it
+ close_called = False
+
 def __init__(self, file, name, delete=True):
 self.file = file
 self.name = name
- self.close_called = False
 self.delete = delete
 
 # NT provides delete-on-close as a primitive, so we don't need
@@ -350,14 +353,13 @@
 # that this must be referenced as self.unlink, because the
 # name TemporaryFileWrapper may also get None'd out before
 # __del__ is called.
- unlink = _os.unlink
 
- def close(self):
- if not self.close_called:
+ def close(self, unlink=_os.unlink):
+ if not self.close_called and self.file is not None:
 self.close_called = True
 self.file.close()
 if self.delete:
- self.unlink(self.name)
+ unlink(self.name)
 
 # Need to ensure the file is deleted on __del__
 def __del__(self):
@@ -657,10 +659,23 @@
 in it are removed.
 """
 
+ # Handle mkdtemp raising an exception
+ name = None
+ _finalizer = None
+ _closed = False
+
 def __init__(self, suffix="", prefix=template, dir=None):
- self._closed = False
- self.name = None # Handle mkdtemp raising an exception
 self.name = mkdtemp(suffix, prefix, dir)
+ self._finalizer = _weakref.finalize(
+ self, self._cleanup, self.name,
+ warn_message="Implicitly cleaning up {!r}".format(self))
+
+ @classmethod
+ def _cleanup(cls, name, warn_message=None):
+ _shutil.rmtree(name)
+ if warn_message is not None:
+ _warnings.warn(warn_message, ResourceWarning)
+
 
 def __repr__(self):
 return "<{} {!r}>".format(self.__class__.__name__, self.name)
@@ -668,60 +683,13 @@
 def __enter__(self):
 return self.name
 
- def cleanup(self, _warn=False):
- if self.name and not self._closed:
- try:
- self._rmtree(self.name)
- except (TypeError, AttributeError) as ex:
- # Issue #10188: Emit a warning on stderr
- # if the directory could not be cleaned
- # up due to missing globals
- if "None" not in str(ex):
- raise
- print("ERROR: {!r} while cleaning up {!r}".format(ex, self,),
- file=_sys.stderr)
- return
- self._closed = True
- if _warn:
- self._warn("Implicitly cleaning up {!r}".format(self),
- ResourceWarning)
-
 def __exit__(self, exc, value, tb):
 self.cleanup()
 
- def __del__(self):
- # Issue a ResourceWarning if implicit cleanup needed
- self.cleanup(_warn=True)
+ def cleanup(self):
+ if self._finalizer is not None:
+ self._finalizer.detach()
+ if self.name is not None and not self._closed:
+ _shutil.rmtree(self.name)
+ self._closed = True
 
- # XXX (ncoghlan): The following code attempts to make
- # this class tolerant of the module nulling out process
- # that happens during CPython interpreter shutdown
- # Alas, it doesn't actually manage it. See issue #10188
- _listdir = staticmethod(_os.listdir)
- _path_join = staticmethod(_os.path.join)
- _isdir = staticmethod(_os.path.isdir)
- _islink = staticmethod(_os.path.islink)
- _remove = staticmethod(_os.remove)
- _rmdir = staticmethod(_os.rmdir)
- _warn = _warnings.warn
-
- def _rmtree(self, path):
- # Essentially a stripped down version of shutil.rmtree. We can't
- # use globals because they may be None'ed out at shutdown.
- for name in self._listdir(path):
- fullname = self._path_join(path, name)
- try:
- isdir = self._isdir(fullname) and not self._islink(fullname)
- except OSError:
- isdir = False
- if isdir:
- self._rmtree(fullname)
- else:
- try:
- self._remove(fullname)
- except OSError:
- pass
- try:
- self._rmdir(path)
- except OSError:
- pass
diff --git a/Lib/test/test_tempfile.py b/Lib/test/test_tempfile.py
--- a/Lib/test/test_tempfile.py
+++ b/Lib/test/test_tempfile.py
@@ -11,7 +11,7 @@
 import weakref
 
 import unittest
-from test import support
+from test import support, script_helper
 
 
 if hasattr(os, 'stat'):
@@ -1088,7 +1088,8 @@
 self.nameCheck(tmp.name, dir, pre, suf)
 # Create a subdirectory and some files
 if recurse:
- self.do_create(tmp.name, pre, suf, recurse-1)
+ d1 = self.do_create(tmp.name, pre, suf, recurse-1)
+ d1.name = None
 with open(os.path.join(tmp.name, "test.txt"), "wb") as f:
 f.write(b"Hello world!")
 return tmp
@@ -1120,7 +1121,7 @@
 def test_cleanup_with_symlink_to_a_directory(self):
 # cleanup() should not follow symlinks to directories (issue #12464)
 d1 = self.do_create()
- d2 = self.do_create()
+ d2 = self.do_create(recurse=0)
 
 # Symlink d1/foo -> d2
 os.symlink(d2.name, os.path.join(d1.name, "foo"))
@@ -1150,60 +1151,51 @@
 finally:
 os.rmdir(dir)
 
- @unittest.expectedFailure # See issue #10188
 def test_del_on_shutdown(self):
 # A TemporaryDirectory may be cleaned up during shutdown
- # Make sure it works with the relevant modules nulled out
 with self.do_create() as dir:
- d = self.do_create(dir=dir)
- # Mimic the nulling out of modules that
- # occurs during system shutdown
- modules = [os, os.path]
- if has_stat:
- modules.append(stat)
- # Currently broken, so suppress the warning
- # that is otherwise emitted on stdout
- with support.captured_stderr() as err:
- with NulledModules(*modules):
- d.cleanup()
- # Currently broken, so stop spurious exception by
- # indicating the object has already been closed
- d._closed = True
- # And this assert will fail, as expected by the
- # unittest decorator...
- self.assertFalse(os.path.exists(d.name),
- "TemporaryDirectory %s exists after cleanup" % d.name)
+ for mod in ('builtins', 'os', 'shutil', 'sys', 'tempfile', 'warnings'):
+ code = """if True:
+ import builtins
+ import os
+ import shutil
+ import sys
+ import tempfile
+ import warnings
+
+ tmp = tempfile.TemporaryDirectory(dir={dir!r})
+ sys.stdout.buffer.write(tmp.name.encode())
+
+ tmp2 = os.path.join(tmp.name, 'test_dir')
+ os.mkdir(tmp2)
+ with open(os.path.join(tmp2, "test.txt"), "w") as f:
+ f.write("Hello world!")
+
+ {mod}.tmp = tmp
+
+ warnings.filterwarnings("always", category=ResourceWarning)
+ """.format(dir=dir, mod=mod)
+ rc, out, err = script_helper.assert_python_ok("-c", code)
+ tmp_name = out.decode().strip()
+ self.assertFalse(os.path.exists(tmp_name),
+ "TemporaryDirectory %s exists after cleanup" % tmp_name)
+ err = err.decode('utf-8', 'backslashreplace')
+ self.assertNotIn("Exception ", err)
+ self.assertIn("ResourceWarning: Implicitly cleaning up", err)
 
 def test_warnings_on_cleanup(self):
- # Two kinds of warning on shutdown
- # Issue 10888: may write to stderr if modules are nulled out
- # ResourceWarning will be triggered by __del__
+ # ResourceWarning will be triggered by __del__
 with self.do_create() as dir:
- if os.sep != '\\':
- # Embed a backslash in order to make sure string escaping
- # in the displayed error message is dealt with correctly
- suffix = '\\check_backslash_handling'
- else:
- suffix = ''
- d = self.do_create(dir=dir, suf=suffix)
-
- #Check for the Issue 10888 message
- modules = [os, os.path]
- if has_stat:
- modules.append(stat)
- with support.captured_stderr() as err:
- with NulledModules(*modules):
- d.cleanup()
- message = err.getvalue().replace('\\\\', '\\')
- self.assertIn("while cleaning up", message)
- self.assertIn(d.name, message)
+ d = self.do_create(dir=dir, recurse=3)
+ name = d.name
 
 # Check for the resource warning
 with support.check_warnings(('Implicitly', ResourceWarning), quiet=False):
 warnings.filterwarnings("always", category=ResourceWarning)
- d.__del__()
- self.assertFalse(os.path.exists(d.name),
- "TemporaryDirectory %s exists after __del__" % d.name)
+ del d
+ support.gc_collect()
+ self.assertFalse(os.path.exists(name),
+ "TemporaryDirectory %s exists after __del__" % name)
 
 def test_multiple_close(self):
 # Can be cleaned-up many times without error
diff --git a/Misc/NEWS b/Misc/NEWS
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -48,6 +48,10 @@
 Library
 -------
 
+- Issue #19077: tempfile.TemporaryDirectory cleanup no longer fails when
+ called during shutdown. Emitting resource warning in __del__ no longer fails.
+ Original patch by Antoine Pitrou.
+
 - Issue #20394: Silence Coverity warning in audioop module.
 
 - Issue #20367: Fix behavior of concurrent.futures.as_completed() for
-- 
Repository URL: http://hg.python.org/cpython
