Repository navigation
Callable supplied to shutil.rmtree(onexc=...) should accept a single argument #103218
Description
Activity
The
functionargument enables the error handler to resolve the error and possibly retry the operation, or a related operation, in order to allowrmtree()to continue.The implementation already bends the truth, for example supplying
os.path.islinkwhen a junction check fails.To be strictly correct, we could eliminate the
_rmtree_islink()call in the top level ofshutil.rmtree():try: if os.path.islink(path): # symlinks to directories are forbidden, see bug #1669 raise OSError("Cannot call rmtree on a symbolic link") except OSError as err: onexc(os.path.islink, path, err) # can't continue even if onexc hook returns return try: if os.path.isjunction(path): raise OSError("Cannot call rmtree on a junction") except OSError as err: onexc(os.path.isjunction, path, err) return
In either case, a good way to resolve the error is to delete the symlink or junction reparse point. The upside of calling the error handler with
os.path.islinkfor a junction is that older code might handle the error appropriately viaos.unlink(). OTOH, older code would probably just log an error due toos.path.isjunction, and give up.The second argument,
path, is already be available viaexc.filename.Note that the existing
OSErrorcall in the above code doesn't include the filename. That would have to be fixed. Also, when_use_fd_functionsis true, there are a couple of cases that handle a genericException, which could beTypeErrororOverflowError(e.g.dir_fdis too big).The
functionargument enables the error handler to resolve the error and possibly retry the operation, or a related operation, in order to allowrmtree()to continue.When is the exception (and file path) not enough to determine that?
tempfileusesrmtree()and supplies its own error handler that resets permissions, but it doesn't look atfunction.In either case, a good way to resolve the error is to delete the symlink or junction reparse point. The upside of calling the error handler with
os.path.islinkfor a junction is that older code might handle the error appropriately viaos.unlink(). OTOH, older code would probably just log an error due toos.path.isjunction, and give up.That kind of illustrates why I dislike this: it makes the internal implementation part of the function's public interface. We have an opportunity to remove this feature while we're adjusting the error handling in
rmtree(), and personally I don't see a good reason to keep it.When is the exception (and file path) not enough to determine that?
tempfileusesrmtree()and supplies its own error handler that resets permissions, but it doesn't look atfunction.Granted, but I wouldn't hold up
tempfileas a shining example. On Windows, its error handler can blow up with a recursion error (see #79325) due to a sharing violation, or any other permission error on Windows that can't be fixed within the limits of the standard library, since the standard library has no support at all for managing file permissions on Windows. I posted a minor modification that avoids the recursion error, but Serhiy never followed up. I don't recall why I addedresetperms_funcs. I don't think it's necessary, or even for the better.Possibly the strongest argument in favor of doing away with the
functionparameter is that what the path refers to could change before the error handler executes. Or a file could be added to a directory that was empty, wherein a failedrmdir()now has to be handled viarmtree().3.12 beta 1 is out, so there's no longer an opportunity to revise the argument. Closing this ticket!
The
onexcargument was added two weeks ago to replaceonerror: d51a6dcThough it improves the third argument (replacing a 3-tuple with the exception object), I think we should also consider two further improvements while we're in the business of replacing
onerror, as the opportunity may not come again for a long time:function, provides an unreasonably deep insight into the internals ofrmtree(). The implementation already bends the truth, for example supplyingos.path.islinkwhen a junction check fails. Error reporting can already be achieved by printing or re-raising the exception. It's not used in the standard library (e.g. bytempfile). What legitimate use cases are there? I propose we remove it!path, is already be available viaexc.filename. This is noted inos.walk()'s documentation of itsonerrorargument. Also propose we remove it.If we make
onexcaccept a single argument, it would align with theonerrorargument toos.walk()andos.fwalk(), and provide more relevant (/less redundant) information.