From 04ddf007deda46f177a3b821a4467aa5fcb9cc7c Mon Sep 17 00:00:00 2001 From: "C.A.M. Gerlach" Date: Sun, 7 Mar 2021 21:53:53 -0600 Subject: [PATCH 1/6] bpo-29982: Add ignore_cleanup_errors param to TemporaryDirectory --- Lib/tempfile.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/Lib/tempfile.py b/Lib/tempfile.py index dc088d9d7e4484..a837f2bfe7dd5f 100644 --- a/Lib/tempfile.py +++ b/Lib/tempfile.py @@ -768,7 +768,7 @@ def writelines(self, iterable): return rv -class TemporaryDirectory(object): +class TemporaryDirectory: """Create and return a temporary directory. This has the same behavior as mkdtemp but can be used as a context manager. For example: @@ -780,14 +780,17 @@ class TemporaryDirectory(object): in it are removed. """ - def __init__(self, suffix=None, prefix=None, dir=None): + def __init__(self, suffix=None, prefix=None, dir=None, + ignore_cleanup_errors=False): self.name = mkdtemp(suffix, prefix, dir) + self._ignore_cleanup_errors = ignore_cleanup_errors self._finalizer = _weakref.finalize( self, self._cleanup, self.name, - warn_message="Implicitly cleaning up {!r}".format(self)) + warn_message="Implicitly cleaning up {!r}".format(self), + ignore_errors=self._ignore_cleanup_errors) @classmethod - def _rmtree(cls, name): + def _rmtree(cls, name, ignore_errors=False): def onerror(func, path, exc_info): if issubclass(exc_info[0], PermissionError): def resetperms(path): @@ -806,19 +809,20 @@ def resetperms(path): _os.unlink(path) # PermissionError is raised on FreeBSD for directories except (IsADirectoryError, PermissionError): - cls._rmtree(path) + cls._rmtree(path, ignore_errors=ignore_errors) except FileNotFoundError: pass elif issubclass(exc_info[0], FileNotFoundError): pass else: - raise + if not ignore_errors: + raise _shutil.rmtree(name, onerror=onerror) @classmethod - def _cleanup(cls, name, warn_message): - cls._rmtree(name) + def _cleanup(cls, name, warn_message, ignore_errors=False): + cls._rmtree(name, ignore_errors=ignore_errors) _warnings.warn(warn_message, ResourceWarning) def __repr__(self): @@ -832,6 +836,6 @@ def __exit__(self, exc, value, tb): def cleanup(self): if self._finalizer.detach(): - self._rmtree(self.name) + self._rmtree(self.name, ignore_errors=self._ignore_cleanup_errors) __class_getitem__ = classmethod(_types.GenericAlias) From 8e17d5130329a665fe39ba3e906c03655252216b Mon Sep 17 00:00:00 2001 From: "C.A.M. Gerlach" Date: Sun, 7 Mar 2021 22:00:41 -0600 Subject: [PATCH 2/6] Allow TemporaryDirectory cleanup to run again if dir still exists --- Lib/tempfile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/tempfile.py b/Lib/tempfile.py index a837f2bfe7dd5f..4b2547c98f1c71 100644 --- a/Lib/tempfile.py +++ b/Lib/tempfile.py @@ -835,7 +835,7 @@ def __exit__(self, exc, value, tb): self.cleanup() def cleanup(self): - if self._finalizer.detach(): + if self._finalizer.detach() or _os.path.exists(self.name): self._rmtree(self.name, ignore_errors=self._ignore_cleanup_errors) __class_getitem__ = classmethod(_types.GenericAlias) From 571e918a5a01d66c0c853ad727842a4573b2f0fe Mon Sep 17 00:00:00 2001 From: "C.A.M. Gerlach" Date: Sun, 7 Mar 2021 22:01:30 -0600 Subject: [PATCH 3/6] Add new tests for TemporaryDirectory cleanup when ignoring errors --- Lib/test/test_tempfile.py | 75 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_tempfile.py b/Lib/test/test_tempfile.py index 5822c7535e3784..fabb51aade97a5 100644 --- a/Lib/test/test_tempfile.py +++ b/Lib/test/test_tempfile.py @@ -1365,13 +1365,17 @@ def __exit__(self, *exc_info): d.clear() d.update(c) + class TestTemporaryDirectory(BaseTestCase): """Test TemporaryDirectory().""" - def do_create(self, dir=None, pre="", suf="", recurse=1, dirs=1, files=1): + def do_create(self, dir=None, pre="", suf="", recurse=1, dirs=1, files=1, + ignore_cleanup_errors=False): if dir is None: dir = tempfile.gettempdir() - tmp = tempfile.TemporaryDirectory(dir=dir, prefix=pre, suffix=suf) + tmp = tempfile.TemporaryDirectory( + dir=dir, prefix=pre, suffix=suf, + ignore_cleanup_errors=ignore_cleanup_errors) self.nameCheck(tmp.name, dir, pre, suf) self.do_create2(tmp.name, recurse, dirs, files) return tmp @@ -1410,6 +1414,25 @@ def test_explicit_cleanup(self): finally: os.rmdir(dir) + def test_explict_cleanup_ignore_errors(self): + """Test that cleanup doesn't return an error when ignoring them.""" + with tempfile.TemporaryDirectory() as working_dir: + temp_dir = self.do_create( + dir=working_dir, ignore_cleanup_errors=True) + temp_path = pathlib.Path(temp_dir.name) + self.assertTrue(temp_path.exists(), + f"TemporaryDirectory {temp_path!s} does not exist") + with open(temp_path / "a_file.txt", "w+t") as open_file: + open_file.write("Hello world!\n") + temp_dir.cleanup() + self.assertIn(len(list(temp_path.glob("*"))), {0, 1}, + "Not all closed files cleaned up in " + f"TemporaryDirectory {temp_path!s}") + temp_dir.cleanup() + self.assertFalse( + temp_path.exists(), + f"TemporaryDirectory {temp_path!s} exists after cleanup") + @os_helper.skip_unless_symlink def test_cleanup_with_symlink_to_a_directory(self): # cleanup() should not follow symlinks to directories (issue #12464) @@ -1444,6 +1467,22 @@ def test_del_on_collection(self): finally: os.rmdir(dir) + @support.cpython_only + def test_del_on_collection_ignore_errors(self): + """Test that ignoring errors works when TemporaryDirectory is gced.""" + with tempfile.TemporaryDirectory() as working_dir: + temp_dir = self.do_create( + dir=working_dir, ignore_cleanup_errors=True) + temp_path = pathlib.Path(temp_dir.name) + self.assertTrue(temp_path.exists(), + f"TemporaryDirectory {temp_path!s} does not exist") + with open(temp_path / "a_file.txt", "w+t") as open_file: + open_file.write("Hello world!\n") + del temp_dir + self.assertIn(len(list(temp_path.glob("*"))), {0, 1}, + "Not all closed files cleaned up in " + f"TemporaryDirectory {temp_path!s}") + def test_del_on_shutdown(self): # A TemporaryDirectory may be cleaned up during shutdown with self.do_create() as dir: @@ -1476,6 +1515,38 @@ def test_del_on_shutdown(self): self.assertNotIn("Exception ", err) self.assertIn("ResourceWarning: Implicitly cleaning up", err) + def test_del_on_shutdown_ignore_errors(self): + """Test ignoring errors works when a tempdir is gc'ed on shutdown.""" + with tempfile.TemporaryDirectory() as working_dir: + code = """if True: + import pathlib + import sys + import tempfile + import warnings + + temp_dir = tempfile.TemporaryDirectory( + dir={working_dir!r}, ignore_cleanup_errors=True) + sys.stdout.buffer.write(temp_dir.name.encode()) + + temp_dir_2 = pathlib.Path(temp_dir.name) / "test_dir" + temp_dir_2.mkdir() + with open(temp_dir_2 / "test0.txt", "w") as test_file: + test_file.write("Hello world!") + open_file = open(temp_dir_2 / "open_file.txt", "w") + open_file.write("Hello world!") + + warnings.filterwarnings("always", category=ResourceWarning) + """.format(working_dir=working_dir) + __, out, err = script_helper.assert_python_ok("-c", code) + temp_path = pathlib.Path(out.decode().strip()) + self.assertIn(len(list(temp_path.glob("*"))), {0, 1}, + "Not all closed files cleaned up in " + f"TemporaryDirectory {temp_path!s}") + err = err.decode('utf-8', 'backslashreplace') + self.assertNotIn("Exception", err) + self.assertNotIn("Error", err) + self.assertIn("ResourceWarning: Implicitly cleaning up", err) + def test_exit_on_shutdown(self): # Issue #22427 with self.do_create() as dir: From 5e9e3e2b86a98981d86a27100c1a0edf20fa2cf2 Mon Sep 17 00:00:00 2001 From: "C.A.M. Gerlach" Date: Sun, 7 Mar 2021 22:50:34 -0600 Subject: [PATCH 4/6] Update TemporaryDirectory doc for added ignore_cleanup_errors param --- Doc/library/tempfile.rst | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/Doc/library/tempfile.rst b/Doc/library/tempfile.rst index 2b8a35e2651e73..f84319180830e1 100644 --- a/Doc/library/tempfile.rst +++ b/Doc/library/tempfile.rst @@ -118,12 +118,12 @@ The module defines the following user-callable items: Added *errors* parameter. -.. function:: TemporaryDirectory(suffix=None, prefix=None, dir=None) +.. function:: TemporaryDirectory(suffix=None, prefix=None, dir=None, ignore_cleanup_errors=False) This function securely creates a temporary directory using the same rules as :func:`mkdtemp`. The resulting object can be used as a context manager (see :ref:`tempfile-examples`). On completion of the context or destruction - of the temporary directory object the newly created temporary directory + of the temporary directory object, the newly created temporary directory and all its contents are removed from the filesystem. The directory name can be retrieved from the :attr:`name` attribute of the @@ -132,12 +132,21 @@ The module defines the following user-callable items: the :keyword:`with` statement, if there is one. The directory can be explicitly cleaned up by calling the - :func:`cleanup` method. + :func:`cleanup` method. If *ignore_cleanup_errors* is true, any unhandled + exceptions during explicit or implicit cleanup (such as a + :exc:`PermissionError` removing open files on Windows) will be ignored, + and the remaining removable items deleted on a "best-effort" basis. + Otherwise, errors will be raised in whatever context cleanup occurs + (the :func:`cleanup` call, exiting the context manager, when the object + is garbage-collected or during interpreter shutdown). .. audit-event:: tempfile.mkdtemp fullpath tempfile.TemporaryDirectory .. versionadded:: 3.2 + .. versionchanged:: 3.10 + Added *ignore_cleanup_errors* parameter. + .. function:: mkstemp(suffix=None, prefix=None, dir=None, text=False) From 3c2ad3c1678442ca0be03fbbd2f990103cf9307c Mon Sep 17 00:00:00 2001 From: "C.A.M. Gerlach" Date: Sun, 7 Mar 2021 23:25:27 -0600 Subject: [PATCH 5/6] Add NEWS entry for ignore_cleanup_errors param to TemporaryDirectory --- .../next/Library/2021-03-07-23-23-03.bpo-29982.Q9iszT.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2021-03-07-23-23-03.bpo-29982.Q9iszT.rst diff --git a/Misc/NEWS.d/next/Library/2021-03-07-23-23-03.bpo-29982.Q9iszT.rst b/Misc/NEWS.d/next/Library/2021-03-07-23-23-03.bpo-29982.Q9iszT.rst new file mode 100644 index 00000000000000..fd71bc6e4e0df7 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2021-03-07-23-23-03.bpo-29982.Q9iszT.rst @@ -0,0 +1,3 @@ +Add optional parameter *ignore_cleanup_errors* to +:func:`tempfile.TemporaryDirectory` and allow multiple :func:`cleanup` attempts. +Contributed by C.A.M. Gerlach. From 447ca8802e7fe2206c02266ec3389c25853d4253 Mon Sep 17 00:00:00 2001 From: "C.A.M. Gerlach" Date: Tue, 9 Mar 2021 00:30:27 -0600 Subject: [PATCH 6/6] Check for TemporaryDirectory successful cleanup in tests based on OS --- Lib/test/test_tempfile.py | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/Lib/test/test_tempfile.py b/Lib/test/test_tempfile.py index fabb51aade97a5..3a3f6a999ce0af 100644 --- a/Lib/test/test_tempfile.py +++ b/Lib/test/test_tempfile.py @@ -1425,9 +1425,14 @@ def test_explict_cleanup_ignore_errors(self): with open(temp_path / "a_file.txt", "w+t") as open_file: open_file.write("Hello world!\n") temp_dir.cleanup() - self.assertIn(len(list(temp_path.glob("*"))), {0, 1}, - "Not all closed files cleaned up in " - f"TemporaryDirectory {temp_path!s}") + self.assertEqual(len(list(temp_path.glob("*"))), + int(sys.platform.startswith("win")), + "Unexpected number of files in " + f"TemporaryDirectory {temp_path!s}") + self.assertEqual( + temp_path.exists(), + sys.platform.startswith("win"), + f"TemporaryDirectory {temp_path!s} existance state unexpected") temp_dir.cleanup() self.assertFalse( temp_path.exists(), @@ -1479,9 +1484,14 @@ def test_del_on_collection_ignore_errors(self): with open(temp_path / "a_file.txt", "w+t") as open_file: open_file.write("Hello world!\n") del temp_dir - self.assertIn(len(list(temp_path.glob("*"))), {0, 1}, - "Not all closed files cleaned up in " - f"TemporaryDirectory {temp_path!s}") + self.assertEqual(len(list(temp_path.glob("*"))), + int(sys.platform.startswith("win")), + "Unexpected number of files in " + f"TemporaryDirectory {temp_path!s}") + self.assertEqual( + temp_path.exists(), + sys.platform.startswith("win"), + f"TemporaryDirectory {temp_path!s} existance state unexpected") def test_del_on_shutdown(self): # A TemporaryDirectory may be cleaned up during shutdown @@ -1539,9 +1549,14 @@ def test_del_on_shutdown_ignore_errors(self): """.format(working_dir=working_dir) __, out, err = script_helper.assert_python_ok("-c", code) temp_path = pathlib.Path(out.decode().strip()) - self.assertIn(len(list(temp_path.glob("*"))), {0, 1}, - "Not all closed files cleaned up in " - f"TemporaryDirectory {temp_path!s}") + self.assertEqual(len(list(temp_path.glob("*"))), + int(sys.platform.startswith("win")), + "Unexpected number of files in " + f"TemporaryDirectory {temp_path!s}") + self.assertEqual( + temp_path.exists(), + sys.platform.startswith("win"), + f"TemporaryDirectory {temp_path!s} existance state unexpected") err = err.decode('utf-8', 'backslashreplace') self.assertNotIn("Exception", err) self.assertNotIn("Error", err)