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

[SourceForge.net]

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-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