[MERGE] [BUG 363837] Catch _win32_delete_readonly failure to remove file or directory and try to recover

[Maritza Mendez]

On Mon, May 4, 2009 at 7:08 AM, Aaron Bentley <aaron at aaronbentley.com>wrote:

> Of these, only diff and merge use external programs, so I don't see how
> the rest can be subject to the same race condition.  Unless what you
> mean is that any arbitrary deletion on Windows can fail because some
> random program may be holding a file open.
>

Yes, that's exactly what I meant.  I don't think any of us would accept this
behavior on Unix, and support for Windows should be held to the same
standard.  For me this comes down to whether bzr is viable in combination
with common Windows environments.

I'm also conditioned to catch these things as soon as possible, before they
propagate.  This is a low-level problem which should be prevented whenever
possible.  In my line of work, low-level problems which are allowed to
propagate (and perhaps not caught by every caller) have been known to cost
lives, so I'm paranoid.  Of course we don't write production code in python,
either...  But that level of paranoia is not justified for bzr.  :)

> (A) still give several chances in _win323_delete_readonly because the
> best and most maintainable fixes are where the problem lies;

Not yet convinced.
>

I understand your opinion.  Again, I accept that my paranoia is more than
bzr needs.


>
> > (B) raise only generic exceptions in osutils
>
> Don't understand.  If you fail to handle the error, you should raise it.
>

This really only made sense if (A) made sense.  Since we are rejecting (A)
we also agree to reject (B).

> (C) defer exception handling to the callers (diff, merge, remote,
> /etc./) because those routines may need to do some cleanup and only
> those routines know the "context" of the failed rmtree.
>
> So I need someone to tell me how to formally withdraw/reject my request
> at bundlebuggy

Actually, only reviewers can do that.  I've just done it for you.
>

Thanks.

> and I need feedback on plan ABC above.
>
> Please catch specific exceptions, not using a bare except clause.  Many
> causes of deletion failure are unrecoverable (file does not exist, file
> is a directory, etc), so they should propagate immediately.


That's really my point also.  My objections to what I found in osutils are
(1) a bare exception comes back to the caller (notice the bare "raise" in
the released code) and (2) no attempt to do anything useful about it is
made.  I see why you don't like my first attempt at improving this, and I
think we can still do better.


> Because OSError and WinError use error codes, I typically write code like:
>
> try:
>    os.unlink('foo')
> except OSError, e:
>    if e.errno != errno.ENOENT:
>        raise


So I am prepared to create and submit a different patch when I get spare
time.  I still intend to add second-chance code with a small delay, but
these deltas will then be in the high-level callers (diff and merge).  This
will result in some duplication and maybe some small obfuscation.  It also
means that diff and merge will deal with a low-level problem which so far
has been seen only on Windows.  I suspect someone will have a strong opinion
on this.  Ultimately, all I need is *one* acceptable solution.  It does not
have to be my preferred solution.

Thank you, Alexander, for an informative and helpful discussion.

To recap, my original patch was based on the opinion that the best place to
mitigate a low-level win32 error is in the low-level win32 layer so that
high-level code becomes involved only if the low-level can't handle it.
Alexander makes a good counter-proposal, albeit spilling over into non-win32
code.

Does anyone else have opinions on this?

Gracias!
-M
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.ubuntu.com/archives/bazaar/attachments/20090504/4d7d37b4/attachment.htm