[Python-checkins] gh-102828: add onexc arg to shutil.rmtree. Deprecate onerror. (#102829)
iritkatriel
webhook-mailer at python.org
Sun Mar 19 14:33:59 EDT 2023
https://github.com/python/cpython/commit/d51a6dc28e1b2cd0353a78bd13f46e288fa39aa6
commit: d51a6dc28e1b2cd0353a78bd13f46e288fa39aa6
branch: main
author: Irit Katriel <1055913+iritkatriel at users.noreply.github.com>
committer: iritkatriel <1055913+iritkatriel at users.noreply.github.com>
date: 2023-03-19T18:33:51Z
summary:
gh-102828: add onexc arg to shutil.rmtree. Deprecate onerror. (#102829)
files:
A Misc/NEWS.d/next/Library/2023-03-19-15-30-59.gh-issue-102828.NKClXg.rst
M Doc/library/shutil.rst
M Doc/whatsnew/3.12.rst
M Lib/shutil.py
M Lib/test/test_shutil.py
diff --git a/Doc/library/shutil.rst b/Doc/library/shutil.rst
index b33dbe21b1fa..acba66258fe8 100644
--- a/Doc/library/shutil.rst
+++ b/Doc/library/shutil.rst
@@ -292,15 +292,15 @@ Directory and files operations
.. versionadded:: 3.8
The *dirs_exist_ok* parameter.
-.. function:: rmtree(path, ignore_errors=False, onerror=None, *, dir_fd=None)
+.. function:: rmtree(path, ignore_errors=False, onerror=None, *, onexc=None, dir_fd=None)
.. index:: single: directory; deleting
Delete an entire directory tree; *path* must point to a directory (but not a
symbolic link to a directory). If *ignore_errors* is true, errors resulting
from failed removals will be ignored; if false or omitted, such errors are
- handled by calling a handler specified by *onerror* or, if that is omitted,
- they raise an exception.
+ handled by calling a handler specified by *onexc* or *onerror* or, if both
+ are omitted, exceptions are propagated to the caller.
This function can support :ref:`paths relative to directory descriptors
<dir_fd>`.
@@ -315,14 +315,17 @@ Directory and files operations
otherwise. Applications can use the :data:`rmtree.avoids_symlink_attacks`
function attribute to determine which case applies.
- If *onerror* is provided, it must be a callable that accepts three
- parameters: *function*, *path*, and *excinfo*.
+ If *onexc* is provided, it must be a callable that accepts three parameters:
+ *function*, *path*, and *excinfo*.
The first parameter, *function*, is the function which raised the exception;
it depends on the platform and implementation. The second parameter,
*path*, will be the path name passed to *function*. The third parameter,
- *excinfo*, will be the exception information returned by
- :func:`sys.exc_info`. Exceptions raised by *onerror* will not be caught.
+ *excinfo*, is the exception that was raised. Exceptions raised by *onexc*
+ will not be caught.
+
+ The deprecated *onerror* is similar to *onexc*, except that the third
+ parameter it receives is the tuple returned from :func:`sys.exc_info`.
.. audit-event:: shutil.rmtree path,dir_fd shutil.rmtree
@@ -337,6 +340,9 @@ Directory and files operations
.. versionchanged:: 3.11
The *dir_fd* parameter.
+ .. versionchanged:: 3.12
+ Added the *onexc* parameter, deprecated *onerror*.
+
.. attribute:: rmtree.avoids_symlink_attacks
Indicates whether the current platform and implementation provides a
@@ -509,7 +515,7 @@ rmtree example
~~~~~~~~~~~~~~
This example shows how to remove a directory tree on Windows where some
-of the files have their read-only bit set. It uses the onerror callback
+of the files have their read-only bit set. It uses the onexc callback
to clear the readonly bit and reattempt the remove. Any subsequent failure
will propagate. ::
@@ -521,7 +527,7 @@ will propagate. ::
os.chmod(path, stat.S_IWRITE)
func(path)
- shutil.rmtree(directory, onerror=remove_readonly)
+ shutil.rmtree(directory, onexc=remove_readonly)
.. _archiving-operations:
diff --git a/Doc/whatsnew/3.12.rst b/Doc/whatsnew/3.12.rst
index 32fec962560a..a36e68528c4f 100644
--- a/Doc/whatsnew/3.12.rst
+++ b/Doc/whatsnew/3.12.rst
@@ -337,6 +337,11 @@ shutil
of the process to *root_dir* to perform archiving.
(Contributed by Serhiy Storchaka in :gh:`74696`.)
+* :func:`shutil.rmtree` now accepts a new argument *onexc* which is an
+ error handler like *onerror* but which expects an exception instance
+ rather than a *(typ, val, tb)* triplet. *onerror* is deprecated.
+ (Contributed by Irit Katriel in :gh:`102828`.)
+
sqlite3
-------
@@ -498,6 +503,10 @@ Deprecated
fields are deprecated. Use :data:`sys.last_exc` instead.
(Contributed by Irit Katriel in :gh:`102778`.)
+* The *onerror* argument of :func:`shutil.rmtree` is deprecated. Use *onexc*
+ instead. (Contributed by Irit Katriel in :gh:`102828`.)
+
+
Pending Removal in Python 3.13
------------------------------
diff --git a/Lib/shutil.py b/Lib/shutil.py
index 867925aa10cc..89a7ac72d983 100644
--- a/Lib/shutil.py
+++ b/Lib/shutil.py
@@ -575,12 +575,12 @@ def _rmtree_islink(path):
return os.path.islink(path)
# version vulnerable to race conditions
-def _rmtree_unsafe(path, onerror):
+def _rmtree_unsafe(path, onexc):
try:
with os.scandir(path) as scandir_it:
entries = list(scandir_it)
- except OSError:
- onerror(os.scandir, path, sys.exc_info())
+ except OSError as err:
+ onexc(os.scandir, path, err)
entries = []
for entry in entries:
fullname = entry.path
@@ -596,28 +596,28 @@ def _rmtree_unsafe(path, onerror):
# a directory with a symlink after the call to
# os.scandir or entry.is_dir above.
raise OSError("Cannot call rmtree on a symbolic link")
- except OSError:
- onerror(os.path.islink, fullname, sys.exc_info())
+ except OSError as err:
+ onexc(os.path.islink, fullname, err)
continue
- _rmtree_unsafe(fullname, onerror)
+ _rmtree_unsafe(fullname, onexc)
else:
try:
os.unlink(fullname)
- except OSError:
- onerror(os.unlink, fullname, sys.exc_info())
+ except OSError as err:
+ onexc(os.unlink, fullname, err)
try:
os.rmdir(path)
- except OSError:
- onerror(os.rmdir, path, sys.exc_info())
+ except OSError as err:
+ onexc(os.rmdir, path, err)
# Version using fd-based APIs to protect against races
-def _rmtree_safe_fd(topfd, path, onerror):
+def _rmtree_safe_fd(topfd, path, onexc):
try:
with os.scandir(topfd) as scandir_it:
entries = list(scandir_it)
except OSError as err:
err.filename = path
- onerror(os.scandir, path, sys.exc_info())
+ onexc(os.scandir, path, err)
return
for entry in entries:
fullname = os.path.join(path, entry.name)
@@ -630,25 +630,25 @@ def _rmtree_safe_fd(topfd, path, onerror):
try:
orig_st = entry.stat(follow_symlinks=False)
is_dir = stat.S_ISDIR(orig_st.st_mode)
- except OSError:
- onerror(os.lstat, fullname, sys.exc_info())
+ except OSError as err:
+ onexc(os.lstat, fullname, err)
continue
if is_dir:
try:
dirfd = os.open(entry.name, os.O_RDONLY, dir_fd=topfd)
dirfd_closed = False
- except OSError:
- onerror(os.open, fullname, sys.exc_info())
+ except OSError as err:
+ onexc(os.open, fullname, err)
else:
try:
if os.path.samestat(orig_st, os.fstat(dirfd)):
- _rmtree_safe_fd(dirfd, fullname, onerror)
+ _rmtree_safe_fd(dirfd, fullname, onexc)
try:
os.close(dirfd)
dirfd_closed = True
os.rmdir(entry.name, dir_fd=topfd)
- except OSError:
- onerror(os.rmdir, fullname, sys.exc_info())
+ except OSError as err:
+ onexc(os.rmdir, fullname, err)
else:
try:
# This can only happen if someone replaces
@@ -656,23 +656,23 @@ def _rmtree_safe_fd(topfd, path, onerror):
# os.scandir or stat.S_ISDIR above.
raise OSError("Cannot call rmtree on a symbolic "
"link")
- except OSError:
- onerror(os.path.islink, fullname, sys.exc_info())
+ except OSError as err:
+ onexc(os.path.islink, fullname, err)
finally:
if not dirfd_closed:
os.close(dirfd)
else:
try:
os.unlink(entry.name, dir_fd=topfd)
- except OSError:
- onerror(os.unlink, fullname, sys.exc_info())
+ except OSError as err:
+ onexc(os.unlink, fullname, err)
_use_fd_functions = ({os.open, os.stat, os.unlink, os.rmdir} <=
os.supports_dir_fd and
os.scandir in os.supports_fd and
os.stat in os.supports_follow_symlinks)
-def rmtree(path, ignore_errors=False, onerror=None, *, dir_fd=None):
+def rmtree(path, ignore_errors=False, onerror=None, *, onexc=None, dir_fd=None):
"""Recursively delete a directory tree.
If dir_fd is not None, it should be a file descriptor open to a directory;
@@ -680,21 +680,39 @@ def rmtree(path, ignore_errors=False, onerror=None, *, dir_fd=None):
dir_fd may not be implemented on your platform.
If it is unavailable, using it will raise a NotImplementedError.
- If ignore_errors is set, errors are ignored; otherwise, if onerror
- is set, it is called to handle the error with arguments (func,
+ If ignore_errors is set, errors are ignored; otherwise, if onexc or
+ onerror is set, it is called to handle the error with arguments (func,
path, exc_info) where func is platform and implementation dependent;
path is the argument to that function that caused it to fail; and
- exc_info is a tuple returned by sys.exc_info(). If ignore_errors
- is false and onerror is None, an exception is raised.
+ the value of exc_info describes the exception. For onexc it is the
+ exception instance, and for onerror it is a tuple as returned by
+ sys.exc_info(). If ignore_errors is false and both onexc and
+ onerror are None, the exception is reraised.
+ onerror is deprecated and only remains for backwards compatibility.
+ If both onerror and onexc are set, onerror is ignored and onexc is used.
"""
sys.audit("shutil.rmtree", path, dir_fd)
if ignore_errors:
- def onerror(*args):
+ def onexc(*args):
pass
- elif onerror is None:
- def onerror(*args):
+ elif onerror is None and onexc is None:
+ def onexc(*args):
raise
+ elif onexc is None:
+ if onerror is None:
+ def onexc(*args):
+ raise
+ else:
+ # delegate to onerror
+ def onexc(*args):
+ func, path, exc = args
+ if exc is None:
+ exc_info = None, None, None
+ else:
+ exc_info = type(exc), exc, exc.__traceback__
+ return onerror(func, path, exc_info)
+
if _use_fd_functions:
# While the unsafe rmtree works fine on bytes, the fd based does not.
if isinstance(path, bytes):
@@ -703,30 +721,30 @@ def onerror(*args):
# lstat()/open()/fstat() trick.
try:
orig_st = os.lstat(path, dir_fd=dir_fd)
- except Exception:
- onerror(os.lstat, path, sys.exc_info())
+ except Exception as err:
+ onexc(os.lstat, path, err)
return
try:
fd = os.open(path, os.O_RDONLY, dir_fd=dir_fd)
fd_closed = False
- except Exception:
- onerror(os.open, path, sys.exc_info())
+ except Exception as err:
+ onexc(os.open, path, err)
return
try:
if os.path.samestat(orig_st, os.fstat(fd)):
- _rmtree_safe_fd(fd, path, onerror)
+ _rmtree_safe_fd(fd, path, onexc)
try:
os.close(fd)
fd_closed = True
os.rmdir(path, dir_fd=dir_fd)
- except OSError:
- onerror(os.rmdir, path, sys.exc_info())
+ except OSError as err:
+ onexc(os.rmdir, path, err)
else:
try:
# symlinks to directories are forbidden, see bug #1669
raise OSError("Cannot call rmtree on a symbolic link")
- except OSError:
- onerror(os.path.islink, path, sys.exc_info())
+ except OSError as err:
+ onexc(os.path.islink, path, err)
finally:
if not fd_closed:
os.close(fd)
@@ -737,11 +755,11 @@ def onerror(*args):
if _rmtree_islink(path):
# symlinks to directories are forbidden, see bug #1669
raise OSError("Cannot call rmtree on a symbolic link")
- except OSError:
- onerror(os.path.islink, path, sys.exc_info())
- # can't continue even if onerror hook returns
+ except OSError as err:
+ onexc(os.path.islink, path, err)
+ # can't continue even if onexc hook returns
return
- return _rmtree_unsafe(path, onerror)
+ return _rmtree_unsafe(path, onexc)
# Allow introspection of whether or not the hardening against symlink
# attacks is supported on the current platform
diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py
index 8fe62216ecdc..fee3e7f76563 100644
--- a/Lib/test/test_shutil.py
+++ b/Lib/test/test_shutil.py
@@ -195,7 +195,7 @@ def test_rmtree_works_on_bytes(self):
shutil.rmtree(victim)
@os_helper.skip_unless_symlink
- def test_rmtree_fails_on_symlink(self):
+ def test_rmtree_fails_on_symlink_onerror(self):
tmp = self.mkdtemp()
dir_ = os.path.join(tmp, 'dir')
os.mkdir(dir_)
@@ -213,6 +213,25 @@ def onerror(*args):
self.assertEqual(errors[0][1], link)
self.assertIsInstance(errors[0][2][1], OSError)
+ @os_helper.skip_unless_symlink
+ def test_rmtree_fails_on_symlink_onexc(self):
+ tmp = self.mkdtemp()
+ dir_ = os.path.join(tmp, 'dir')
+ os.mkdir(dir_)
+ link = os.path.join(tmp, 'link')
+ os.symlink(dir_, link)
+ self.assertRaises(OSError, shutil.rmtree, link)
+ self.assertTrue(os.path.exists(dir_))
+ self.assertTrue(os.path.lexists(link))
+ errors = []
+ def onexc(*args):
+ errors.append(args)
+ shutil.rmtree(link, onexc=onexc)
+ self.assertEqual(len(errors), 1)
+ self.assertIs(errors[0][0], os.path.islink)
+ self.assertEqual(errors[0][1], link)
+ self.assertIsInstance(errors[0][2], OSError)
+
@os_helper.skip_unless_symlink
def test_rmtree_works_on_symlinks(self):
tmp = self.mkdtemp()
@@ -236,7 +255,7 @@ def test_rmtree_works_on_symlinks(self):
self.assertTrue(os.path.exists(file1))
@unittest.skipUnless(_winapi, 'only relevant on Windows')
- def test_rmtree_fails_on_junctions(self):
+ def test_rmtree_fails_on_junctions_onerror(self):
tmp = self.mkdtemp()
dir_ = os.path.join(tmp, 'dir')
os.mkdir(dir_)
@@ -255,6 +274,26 @@ def onerror(*args):
self.assertEqual(errors[0][1], link)
self.assertIsInstance(errors[0][2][1], OSError)
+ @unittest.skipUnless(_winapi, 'only relevant on Windows')
+ def test_rmtree_fails_on_junctions_onexc(self):
+ tmp = self.mkdtemp()
+ dir_ = os.path.join(tmp, 'dir')
+ os.mkdir(dir_)
+ link = os.path.join(tmp, 'link')
+ _winapi.CreateJunction(dir_, link)
+ self.addCleanup(os_helper.unlink, link)
+ self.assertRaises(OSError, shutil.rmtree, link)
+ self.assertTrue(os.path.exists(dir_))
+ self.assertTrue(os.path.lexists(link))
+ errors = []
+ def onexc(*args):
+ errors.append(args)
+ shutil.rmtree(link, onexc=onexc)
+ self.assertEqual(len(errors), 1)
+ self.assertIs(errors[0][0], os.path.islink)
+ self.assertEqual(errors[0][1], link)
+ self.assertIsInstance(errors[0][2], OSError)
+
@unittest.skipUnless(_winapi, 'only relevant on Windows')
def test_rmtree_works_on_junctions(self):
tmp = self.mkdtemp()
@@ -277,7 +316,7 @@ def test_rmtree_works_on_junctions(self):
self.assertTrue(os.path.exists(dir3))
self.assertTrue(os.path.exists(file1))
- def test_rmtree_errors(self):
+ def test_rmtree_errors_onerror(self):
# filename is guaranteed not to exist
filename = tempfile.mktemp(dir=self.mkdtemp())
self.assertRaises(FileNotFoundError, shutil.rmtree, filename)
@@ -309,6 +348,37 @@ def onerror(*args):
self.assertIsInstance(errors[1][2][1], NotADirectoryError)
self.assertEqual(errors[1][2][1].filename, filename)
+ def test_rmtree_errors_onexc(self):
+ # filename is guaranteed not to exist
+ filename = tempfile.mktemp(dir=self.mkdtemp())
+ self.assertRaises(FileNotFoundError, shutil.rmtree, filename)
+ # test that ignore_errors option is honored
+ shutil.rmtree(filename, ignore_errors=True)
+
+ # existing file
+ tmpdir = self.mkdtemp()
+ write_file((tmpdir, "tstfile"), "")
+ filename = os.path.join(tmpdir, "tstfile")
+ with self.assertRaises(NotADirectoryError) as cm:
+ shutil.rmtree(filename)
+ self.assertEqual(cm.exception.filename, filename)
+ self.assertTrue(os.path.exists(filename))
+ # test that ignore_errors option is honored
+ shutil.rmtree(filename, ignore_errors=True)
+ self.assertTrue(os.path.exists(filename))
+ errors = []
+ def onexc(*args):
+ errors.append(args)
+ shutil.rmtree(filename, onexc=onexc)
+ self.assertEqual(len(errors), 2)
+ self.assertIs(errors[0][0], os.scandir)
+ self.assertEqual(errors[0][1], filename)
+ self.assertIsInstance(errors[0][2], NotADirectoryError)
+ self.assertEqual(errors[0][2].filename, filename)
+ self.assertIs(errors[1][0], os.rmdir)
+ self.assertEqual(errors[1][1], filename)
+ self.assertIsInstance(errors[1][2], NotADirectoryError)
+ self.assertEqual(errors[1][2].filename, filename)
@unittest.skipIf(sys.platform[:6] == 'cygwin',
"This test can't be run on Cygwin (issue #1071513).")
@@ -368,6 +438,100 @@ def check_args_to_onerror(self, func, arg, exc):
self.assertTrue(issubclass(exc[0], OSError))
self.errorState = 3
+ @unittest.skipIf(sys.platform[:6] == 'cygwin',
+ "This test can't be run on Cygwin (issue #1071513).")
+ @os_helper.skip_if_dac_override
+ @os_helper.skip_unless_working_chmod
+ def test_on_exc(self):
+ self.errorState = 0
+ os.mkdir(TESTFN)
+ self.addCleanup(shutil.rmtree, TESTFN)
+
+ self.child_file_path = os.path.join(TESTFN, 'a')
+ self.child_dir_path = os.path.join(TESTFN, 'b')
+ os_helper.create_empty_file(self.child_file_path)
+ os.mkdir(self.child_dir_path)
+ old_dir_mode = os.stat(TESTFN).st_mode
+ old_child_file_mode = os.stat(self.child_file_path).st_mode
+ old_child_dir_mode = os.stat(self.child_dir_path).st_mode
+ # Make unwritable.
+ new_mode = stat.S_IREAD|stat.S_IEXEC
+ os.chmod(self.child_file_path, new_mode)
+ os.chmod(self.child_dir_path, new_mode)
+ os.chmod(TESTFN, new_mode)
+
+ self.addCleanup(os.chmod, TESTFN, old_dir_mode)
+ self.addCleanup(os.chmod, self.child_file_path, old_child_file_mode)
+ self.addCleanup(os.chmod, self.child_dir_path, old_child_dir_mode)
+
+ shutil.rmtree(TESTFN, onexc=self.check_args_to_onexc)
+ # Test whether onexc has actually been called.
+ self.assertEqual(self.errorState, 3,
+ "Expected call to onexc function did not happen.")
+
+ def check_args_to_onexc(self, func, arg, exc):
+ # test_rmtree_errors deliberately runs rmtree
+ # on a directory that is chmod 500, which will fail.
+ # This function is run when shutil.rmtree fails.
+ # 99.9% of the time it initially fails to remove
+ # a file in the directory, so the first time through
+ # func is os.remove.
+ # However, some Linux machines running ZFS on
+ # FUSE experienced a failure earlier in the process
+ # at os.listdir. The first failure may legally
+ # be either.
+ if self.errorState < 2:
+ if func is os.unlink:
+ self.assertEqual(arg, self.child_file_path)
+ elif func is os.rmdir:
+ self.assertEqual(arg, self.child_dir_path)
+ else:
+ self.assertIs(func, os.listdir)
+ self.assertIn(arg, [TESTFN, self.child_dir_path])
+ self.assertTrue(isinstance(exc, OSError))
+ self.errorState += 1
+ else:
+ self.assertEqual(func, os.rmdir)
+ self.assertEqual(arg, TESTFN)
+ self.assertTrue(isinstance(exc, OSError))
+ self.errorState = 3
+
+ def test_both_onerror_and_onexc(self):
+ onerror_called = False
+ onexc_called = False
+
+ def onerror(*args):
+ nonlocal onerror_called
+ onerror_called = True
+
+ def onexc(*args):
+ nonlocal onexc_called
+ onexc_called = True
+
+ os.mkdir(TESTFN)
+ self.addCleanup(shutil.rmtree, TESTFN)
+
+ self.child_file_path = os.path.join(TESTFN, 'a')
+ self.child_dir_path = os.path.join(TESTFN, 'b')
+ os_helper.create_empty_file(self.child_file_path)
+ os.mkdir(self.child_dir_path)
+ old_dir_mode = os.stat(TESTFN).st_mode
+ old_child_file_mode = os.stat(self.child_file_path).st_mode
+ old_child_dir_mode = os.stat(self.child_dir_path).st_mode
+ # Make unwritable.
+ new_mode = stat.S_IREAD|stat.S_IEXEC
+ os.chmod(self.child_file_path, new_mode)
+ os.chmod(self.child_dir_path, new_mode)
+ os.chmod(TESTFN, new_mode)
+
+ self.addCleanup(os.chmod, TESTFN, old_dir_mode)
+ self.addCleanup(os.chmod, self.child_file_path, old_child_file_mode)
+ self.addCleanup(os.chmod, self.child_dir_path, old_child_dir_mode)
+
+ shutil.rmtree(TESTFN, onerror=onerror, onexc=onexc)
+ self.assertTrue(onexc_called)
+ self.assertFalse(onerror_called)
+
def test_rmtree_does_not_choke_on_failing_lstat(self):
try:
orig_lstat = os.lstat
diff --git a/Misc/NEWS.d/next/Library/2023-03-19-15-30-59.gh-issue-102828.NKClXg.rst b/Misc/NEWS.d/next/Library/2023-03-19-15-30-59.gh-issue-102828.NKClXg.rst
new file mode 100644
index 000000000000..be9b2bab24a3
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2023-03-19-15-30-59.gh-issue-102828.NKClXg.rst
@@ -0,0 +1,3 @@
+Add the ``onexc`` arg to :func:`shutil.rmtree`, which is like ``onerror``
+but expects an exception instance rather than an exc_info tuple. Deprecate
+``onerror``.
More information about the Python-checkins
mailing list