Repository navigation
shutil.rmtree and os.listdir cannot recover on error conditions #52769
Description
| BPO | 8523 |
|---|---|
| Nosy | @tarekziade, @merwok, @bitdancer |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/tarekziade'
closed_at = None
created_at = <Date 2010-04-24.18:57:42.010>
labels = ['easy', 'type-feature', 'library']
title = 'shutil.rmtree and os.listdir cannot recover on error conditions'
updated_at = <Date 2013-04-07.19:11:40.267>
user = 'https://bugs.python.org/rubenlm'bugs.python.org fields:
activity = <Date 2013-04-07.19:11:40.267>
actor = 'r.david.murray'
assignee = 'tarek'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2010-04-24.18:57:42.010>
creator = 'rubenlm'
dependencies = []
files = ['29460']
hgrepos = []
issue_num = 8523
keywords = ['patch', 'easy']
message_count = 12.0
messages = ['104117', '104191', '104247', '104482', '104489', '104492', '104508', '104509', '104511', '104513', '184536', '186237']
nosy_count = 5.0
nosy_names = ['tarek', 'eric.araujo', 'r.david.murray', 'rubenlm', 'andrewsg']
pr_nums = []
priority = 'low'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue8523'
versions = ['Python 3.4']
Activity
rubenlm commentedon Apr 24, 2010
The code that lists directory contents in rmtree is:
If there is an error there is nothing the "onerror" function can do to fix the problem because the variable "names" will not be updated after the problem is solved in "onerror".
Two possible solutions:
1 - Call os.listdir() again after onerror()
2 - Allow onerror() to return a value and set "names" to that value.
bitdancer commentedon Apr 26, 2010
If solution 1 is acceptable in the general case, then I think a better fix would look like this:
That is, this is another case in which we can't continue even if onerror returns. However, onerror is free to correct the problem and then call rmtree. (The danger, of course, is infinite recursion, but I don't think it is our responsibility to protect the author of an onerror handler from that potential mistake.)
By analogy to the other place rmtree returns after an onerror call, the above fix does fix a real bug, regardless of the disposition of the feature request, since currently if onerror returns we get a name error.
rubenlm commentedon Apr 26, 2010
Your solution sounds fine to me.
Currently we don't get a NameError because "names" is set to [] before the "try". What happens is that the "for" is skipped and later rmdir fails with "directory not empty".
tarekziade commentedon Apr 29, 2010
The whole error handling in rmtree strikes me as something that cannot be used efficiently. (see also bpo-7969).
How can you decide in an isolated function, that can be called anywhere in the tree you want to remove, the proper thing to do ? You don't know the global status of what is going on.
I think rmtree() should drop these onerror calls and have two different behaviors:
1/ remove all it can in the tree, and return a list of files it couldn't remove, with the error for each file. The developer can then act upon.
2/ analyze the tree to see if the full removal can be done. If it's possible, it does it, if not, it doesn't do anything and return the problems.
For 2/ a possible way to do it could be to copy in a temporary place files that are being removed and copy them back in place in case a problem occurs. This can be long and space consuming though, for big files and big trees. I am not 100% sure 2/ is really useful though...
rubenlm commentedon Apr 29, 2010
Do you really need the global status? I wrote an onerror that seems to works fine after I modified rmtree with the "return" suggested by r.david.murray. It assumes that:
There are other reasons it can fail (attributes, acl, readonly filesystem, ...) but having access to the global status doesn't seem to be of much help anyway.
I don't like your fix 2/ because it can fail to copy files if you don't have read permissions for the file but have write permissions in the directory (so you can delete the file). Besides, the behaviour doesn't seem useful.
/1 seems ok to me but to make use of the global status it provides the user must write a somewhat complex recovery code.
All in all it seems the current behaviour of having an onerror function is more user friendly.
tarekziade commentedon Apr 29, 2010
The onerror() code you did is as complex as a global function working with a sequence returned by rmtree, since it is used *everywhere* in rmtree, and not only for os.listdir issues. IOW, if you didn't handle other possible failures than os.listdir errors, it will fail if rmtree calls it for other APIs like os.remove, etc..
If we state that onerror() is not used as a fallback in rmtree(), and do whatever it wants to do on its side, in an isolated manner, then I find it simpler that this function works will a list of paths rmtree() failed to removed, at the end of the rmtree() process.
I'd be curious to see your onerror() function btw: if it's here just to silent permission errors, 1/ would make it even simpler: don't deal with the error list returned by rmtree() that's all.
bitdancer commentedon Apr 29, 2010
Well, I don't think removing the current onerror support is a viable option for backward compatibility reasons, so we might as well fix it.
I agree that (2) sounds tricky to get reliably right, while I don't see how (1) is an improvement over onerror. It seems to me that the list is either really a tree, in which case the error handler has to reimplement most of rmtree's logic in order to do a recover, or it is a list of error filepath pairs, in which case the error handler would be in most cases simply be iterating over the list and branching based on the error flag...which is no different than what onerror allows, but onerror doesn't need the loop code. Actually, it's less capable than what onerror allows, since if onerror successfully removes a problem file, rmtree will remove the directory, whereas a (1) style handler will have to have its own error logic for dealing with the successive removals (and the (1) style list will have to be guaranteed to be sorted in the correct bottom up order).
I guess I don't see how knowing the global state (which in this case appears to mean the total list of files and directories where removal failed) is useful very often, if ever. It feels like it is a more complicated API that provides little benefit. Do you have some use cases in mind, Tarek?
On the other hand, it seems to me that a nice improvement to onerror would be for it to accept an object, and call an error-case-specific method for each different error case, perhaps even checking for method existence first and doing its normal error handling for that case if the method isn't defined.
rubenlm commentedon Apr 29, 2010
Here is my current error handler:
Looking back to this code there is an infinite recursion bug if os.chmod fails for some reason in the os.listdir condition. I don't see an easy way to solve this...
tarekziade commentedon Apr 29, 2010
The options could be deprecated since the new behavior would *return* errors.
What I have in mind is robustness and simplicity:
robustness because rmtree() will stop calling third party code that can possibly fail and blow the whole process, while working at removing the tree.
Simplicity because, if it fails at removing some files using the usual os.* APIs, it will just return these errors.
Having this two phases-process will ensure that rmtree() did all that was possible to remove files.
And as I said previously, I am curious to know what is going to be done in the onerror() function when something fails in rmtree(). I doubt any third-party code will do better.
This statement "I couldn't copy this file, try it yourself" seems doomed to complexity.
If the only use case for onerror() is to silent failures, returning these failures seem quite enough. Ala smtp when you get back a list of mails that couldn't be send: it doesn't ask you to send them by yourself, just informs you.
Now maybe we do miss some APIs to check for a file tree sanity, like:
tarekziade commentedon Apr 29, 2010
Looking at your example rubenlm, it appears like a case that is missing in rmtree().
You are trying to chmod your tree if a file in there cannot be removed because of the permissions. This sounds like something we need to add in rmtree() directly, for example under a "force_permissions" flag that would handle permission failures by trying to chmod.
I think rmtree() should not try to delegate the hard work to third party code, and should try to handle as much failures as possible,
and just return errors.
andrewsg commentedon Mar 18, 2013
Product of the #pycon 2013 sprint with r.david.murray's assistance. This implements the list of results as per tarek's suggested 1/ behavior in cases where ignore_errors=True. Parameters accepted are not changed; return value is changed from None to an empty list in case of no errors or onerror defined by the user, and to a list of tuples exactly like onerror arguments in the case of ignore_errors=True.
As the ignore_errors=True closure-based implementation was adopted from test_shutil.py code, test_shutil.py is changed in one place to take advantage of the new return value in order to add coverage of the new functionality.
bitdancer commentedon Apr 7, 2013
I think this patch looks good, but it needs a documentation update to go with it. Do you want to work on that, Andrew?
It also seems as though there's no bug that it is practical to fix here, so I'm changing this to a pure enhancement request. If anyone disagrees with that, please describe what you think the appropriate bug fix is, and we can break the enhancement request out into a separate issue.
terryjreedy commentedon Feb 18, 2025
@serhiy-storchaka Do you think that this issue should be closed after you closed the PR? Or should something else be tried?
serhiy-storchaka commentedon Sep 22, 2025
I closed the PR because the solution that it proposes (collecting and returning the list of errors if
ignore_errorsis true`) is already available by specifying the custom error handler. This would only make the code for the particular problem few lines shorter.But this is not the original issue. The original issue is that
shutil.rmtree()provides poor control on errors handling. You can ignore errors (equivalent toignore_errors=True), you can re-raise an exception (default behavior). You can log errors in addition to ignoring them, and you can ignore only particular errors -- this is almost all that you can do. You cannot fix an underlying issue (for example, remove the read-only bit or ask the user if it is okay to remove it) and then sayshutil.rmtree()to repeat again. You can callshutil.rmtree()recursively after an attempt to fix the error, this is how cleanup is implemented in TemporaryDirectory and CPython tests, but this way has limitation -- the limit of the recursion depth.This is a design issue, and not easy. It may be impossible to solve without changing the API, then we need to add new error handler which allows more control.
There are similar issues in
os.walk()and similar functions.