[issue19077] More robust TemporaryDirectory cleanup
Serhiy Storchaka
report at bugs.python.org
Fri Jan 24 18:58:10 CET 2014
Serhiy Storchaka added the comment:
Here are updated patches for 3.3 and 3.4. Changed tests for 3.4, a patch for
3.3 is changed more. Unfortunately in 3.3 exceptions still can be raised when
try to emit a warning during shutdown. Is there any way to determine the
shutdown mode?
----------
Added file: http://bugs.python.org/file33689/tempdir_cleanup-3.3.patch
Added file: http://bugs.python.org/file33690/tempdir_finalize-3.4.patch
_______________________________________
Python tracker <report at bugs.python.org>
<http://bugs.python.org/issue19077>
_______________________________________
-------------- next part --------------
diff -r fc62fcd8e990 Lib/tempfile.py
--- a/Lib/tempfile.py Fri Jan 24 11:44:16 2014 -0500
+++ b/Lib/tempfile.py Fri Jan 24 19:52:38 2014 +0200
@@ -29,9 +29,9 @@
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
@@ -355,10 +355,13 @@
underlying file object, without adding a __del__ method to the
temporary file."""
+ # Set here since __del__ checks it
+ file = None
+ 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
@@ -370,14 +373,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):
@@ -677,9 +679,11 @@
in it are removed.
"""
+ # Handle mkdtemp raising an exception
+ name = 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)
def __repr__(self):
@@ -688,23 +692,18 @@
def __enter__(self):
return self.name
- def cleanup(self, _warn=False):
+ def cleanup(self, _warn=False, _warnings=_warnings):
if self.name and not self._closed:
try:
+ _shutil.rmtree(self.name)
+ except (TypeError, AttributeError) as ex:
+ if "None" not in '%s' % (ex,):
+ raise
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)
+ if _warn and _warnings.warn:
+ _warnings.warn("Implicitly cleaning up {!r}".format(self),
+ ResourceWarning)
def __exit__(self, exc, value, tb):
self.cleanup()
@@ -713,36 +712,19 @@
# Issue a ResourceWarning if implicit cleanup needed
self.cleanup(_warn=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)
- _os_error = OSError
- _warn = _warnings.warn
-
- def _rmtree(self, path):
+ def _rmtree(self, path, _OSError=OSError, _sep=_os.path.sep,
+ _listdir=_os.listdir, _remove=_os.remove, _rmdir=_os.rmdir):
# 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 self._os_error:
- isdir = False
- if isdir:
- self._rmtree(fullname)
- else:
+ if not isinstance(path, str):
+ _sep = _sep.encode()
+ try:
+ for name in _listdir(path):
+ fullname = path + _sep + name
try:
- self._remove(fullname)
- except self._os_error:
- pass
- try:
- self._rmdir(path)
- except self._os_error:
+ _remove(fullname)
+ except _OSError:
+ self._rmtree(fullname)
+ _rmdir(path)
+ except _OSError:
pass
diff -r fc62fcd8e990 Lib/test/test_tempfile.py
--- a/Lib/test/test_tempfile.py Fri Jan 24 11:44:16 2014 -0500
+++ b/Lib/test/test_tempfile.py Fri Jan 24 19:52:38 2014 +0200
@@ -11,7 +11,7 @@
import weakref
import unittest
-from test import support
+from test import support, script_helper
if hasattr(os, 'stat'):
@@ -1073,7 +1073,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
@@ -1105,7 +1106,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"))
@@ -1135,60 +1136,50 @@
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 ('os', 'shutil', 'sys', 'tempfile', 'warnings'):
+ code = """if True:
+ 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)
+ # Two kinds of warning on shutdown
+ # Issue 10188: may write to stderr if modules are nulled out
+ # ResourceWarning will be triggered by __del__
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 -r fc62fcd8e990 Misc/NEWS
--- a/Misc/NEWS Fri Jan 24 11:44:16 2014 -0500
+++ b/Misc/NEWS Fri Jan 24 19:52:38 2014 +0200
@@ -50,6 +50,11 @@
Library
-------
+- Issue #19077: tempfile.TemporaryDirectory cleanup is now most likely
+ successful when called during nulling out of modules during shutdown.
+ Howere it still can throw a misleading exception if emitting a warning
+ fails.
+
- Issue #20317: ExitStack.__exit__ could create a self-referential loop if an
exception raised by a cleanup operation already had its context set
correctly (for example, by the @contextmanager decorator). The infinite
-------------- next part --------------
diff -r 3c3624fec6c8 Lib/tempfile.py
--- a/Lib/tempfile.py Fri Jan 24 11:44:40 2014 -0500
+++ b/Lib/tempfile.py Fri Jan 24 19:52:30 2014 +0200
@@ -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 -r 3c3624fec6c8 Lib/test/test_tempfile.py
--- a/Lib/test/test_tempfile.py Fri Jan 24 11:44:40 2014 -0500
+++ b/Lib/test/test_tempfile.py Fri Jan 24 19:52:30 2014 +0200
@@ -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 -r 3c3624fec6c8 Misc/NEWS
--- a/Misc/NEWS Fri Jan 24 11:44:40 2014 -0500
+++ b/Misc/NEWS Fri Jan 24 19:52:30 2014 +0200
@@ -36,6 +36,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 #20189: unittest.mock now no longer assumes that any object for
which it could get an inspect.Signature is a callable written in Python.
Fix courtesy of Michael Foord.
More information about the Python-bugs-list
mailing list