[ python-Bugs-1048941 ] os.rmtree error handling was always broken

Wed Oct 27 11:57:54 CEST 2004

Bugs item #1048941, was opened at 2004-10-18 03:24
Message generated for change (Comment added) made by jlgijsbers
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=1048941&group_id=5470

Category: None
Group: Python 2.3
Status: Open
Resolution: None
Priority: 8
Submitted By: Guido van Rossum (gvanrossum)
Assigned to: Johannes Gijsbers (jlgijsbers)
Summary: os.rmtree error handling was always broken

Initial Comment:
I just realized that the error handling in
shutil.rmtree has been broken ever since Python 2.3,
and the new implementation in 2.4 inherits this bug.

The problem is that the try/except is outside the for
loop, so that when an error is found, if either
ignore_errors or onerror is set, the desired action is
taken *once* but then the function returns rather than
trying to remove more files (ignoring or reporting
subsequent errors) as it should.

The sad thing is that the original code by David Ascher
(shutil.py rev 1.8) was correct! The bug was introduced
in rev 1.27, which claimed to make the implementation
agree with the docs. :-(

I'm giving this a high priority, hoping that it will be
fixed in 2.4b2 (and hopefully backported to 2.3).

----------------------------------------------------------------------

>Comment By: Johannes Gijsbers (jlgijsbers)
Date: 2004-10-27 11:57

Message:
Logged In: YES 
user_id=469548

Yes, I think this is the right way to go about it. I'd
personally make a couple of changes, but those are up to
your discretion:

* Factor out the try: except:'s to a nested function.
* Use sys.exc_info() instead of the exception for backwards
compatibility.
* Use 'os.path.isdir(fullname) and not
os.path.islink(fullname)' expression instead of the lstat
call. I think it more clearly communicates what we want,
even though it's more stat calls.

Latest version attached (rmtree-refactored.py).

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2004-10-26 18:08

Message:
Logged In: YES 
user_id=6380

How about this no-nonsense version? (rmtree.txt upload) The
only thing you could quibble about is what to do when
os.lstat fails. I chose backward compatibility.

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2004-10-23 20:25

Message:
Logged In: YES 
user_id=6380

BTW, if you want the correct path in the onerror calls for
os.listdir, your best bet is  not to use os.walk but to
write something similar that suits our purpose. After you
remove the features we don't use, it's really only a few
lines of code.

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2004-10-23 19:56

Message:
Logged In: YES 
user_id=6380

I disagree. The docs are wrong because a failing listdir
wasn't anticipated at first when this function was written.
A failing listdir is a common failure case; I don't see how
the os.path calls in os.walk could ever fail (since they all
catch os.error).

----------------------------------------------------------------------

Comment By: Johannes Gijsbers (jlgijsbers)
Date: 2004-10-23 05:01

Message:
Logged In: YES 
user_id=469548

You know what: I think we should remove the os.listdir error
handling:

* The docs only specify "errors resulting from *failed
removals*" as being handled. If they didn't we should worry
about OSErrors from the os.path.* calls in os.walk() too.
* The os.listdir error handling hasn't been in a final
release, so no backwards compatibility problems yet.
* We can't get the path right for os.listdir.
* The problem also existed before the rewrite using
os.walk(), so reverting to that is not a solution.

Patch (the last?) is attached.

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2004-10-21 06:42

Message:
Logged In: YES 
user_id=6380

Yes.

----------------------------------------------------------------------

Comment By: Johannes Gijsbers (jlgijsbers)
Date: 2004-10-20 22:30

Message:
Logged In: YES 
user_id=469548

With 'be careful, it may not exist.', do you mean rmtree
can't rely on exc.filename being set?

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2004-10-19 17:18

Message:
Logged In: YES 
user_id=6380

This looks fine, and can be checked in.

There's one minor remaining issue, but I don't see how to
fix it without changing the signature for the onerror
function that os.walk() takes, and that's a no-no: when the
problem is a directory permission preventing os.listdir() to
work, the path passed to the rmtree caller's onerror
function is always the toplevel path, rather than the
directory that couldn't be listed. The real path is included
in the error message. (Hmm, perhaps you could get it out of
there? I think it's an attribute of the exception object;
but be careful, it may not exist.)

It's unfortunate that the error argument to the onerror
function is a sys.exc_info() triple rather than an exception
object (calling sys.exc_info() is expensive), but that's an
API choice from the past that we can't change until Python 3.0.

----------------------------------------------------------------------

Comment By: Johannes Gijsbers (jlgijsbers)
Date: 2004-10-19 10:57

Message:
Logged In: YES 
user_id=469548

A new patch is attached. I factored out the
handle_call_error method so exception handling is more
local. Also, errors in os.walk were already being handled
using the _raise_err/except OSError combination,  but we now
do it using the onerror argument to os.walk.

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2004-10-19 01:42

Message:
Logged In: YES 
user_id=6380

(Actually, os.walk has an onerror argument, it just uses a
different convention and default, so we could thread this
through easily.)

----------------------------------------------------------------------

Comment By: Guido van Rossum (gvanrossum)
Date: 2004-10-19 01:32

Message:
Logged In: YES 
user_id=6380

(I'm sure Barry didn't mind, I just assigned it to him
because rev. 1.27 had his name on it. :-)

I saw the patch, don't have time to apply it, note that it
has three(!) nested try-except blocks; instead, it should
really have some smaller ones around just exactly the call
that you expect to raise an exception.  Exceptions elsewhere
should *not* be caught.

Also, the raise statement in handle_error() should just be
"raise" without parameters, which re-raises the original
exception. The exception already contains the filename, usually.

Finally: os.walk() seems to suppress errors in os.listdir().
Technically, errors in os.listdir() should be treated the
same as errors in os.remove() / os.rmdir(), but that means
we can't use os.walk() -- or we'd have to add error handling
to it recursively.

----------------------------------------------------------------------

Comment By: Johannes Gijsbers (jlgijsbers)
Date: 2004-10-19 00:18

Message:
Logged In: YES 
user_id=469548

Patch attached. A review would be appreciated, especially as
to whether there's a nicer way to handle the problem, but
I'll check it in before 2.4b2 anyway.

----------------------------------------------------------------------

Comment By: Johannes Gijsbers (jlgijsbers)
Date: 2004-10-18 08:06

Message:
Logged In: YES 
user_id=469548

Barry, hope you don't mind I'll take this one. I can have a
fix tonight or tomorrow.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=105470&aid=1048941&group_id=5470