[MERGE] [BUG 363837] Catch _win32_delete_readonly failure to remove file or directory and try to recover
Maritza Mendez
martitzam at gmail.com
Sun May 10 20:48:19 BST 2009
Hi Alexander,
Thanks. You are right. All three exceptions are a WindowsError. The first
two are access denied (Windows system error 0x05) and the third one is the
sharing violation (Windows system error 0x20 = the 32 (decimal) reported
when bzr passes it through). Although I have not gone so far as to see what
process is holding the files -- and the directory -- but its pretty obvious
that it must be BeyondCompare. Here is the actual output I generated from
_win32_delete_readonly:
Path:
C:/docume~1/martitza/locals~1/temp/bzr-diff-yribs_\new\IssueTrackingNeeds.txt
Exception Type: <type 'exceptions.WindowsError'> ['__class__',
'__delattr__', '__dict__', '__doc__', '__format__', '__getattribute__',
'__getitem__', '__getslice__', '__hash__', '__init__', '__new__',
'__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__setstate__',
'__sizeof__', '__str__', '__subclasshook__', '__unicode__', 'args', 'errno',
'filename', 'message', 'strerror', 'winerror']
Exception Attributes:5
Path: C:/docume~1/martitza
/locals~1/temp/bzr-diff-yribs_\old\IssueTrackingNeeds.txt
Exception Type: <type 'exceptions.WindowsError'> ['__class__',
'__delattr__', '__dict__', '__doc__', '__format__', '__getattribute__',
'__getitem__', '__getslice__', '__hash__', '__init__', '__new__',
'__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__setstate__',
'__sizeof__', '__str__', '__subclasshook__', '__unicode__', 'args', 'errno',
'filename', 'message', ' strerror', 'winerror']
Exception Attributes:5
Path: C:/docume~1/martitza/locals~1/temp/bzr-diff-yribs_
Exception Type: <type 'exceptions.WindowsError'> ['__class__',
'__delattr__', '__dict__', '__doc__', '__format__', '__getattribute__',
'__getitem__', '__getslice__', '__hash__', '__init__', '__new__',
'__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__setstate__',
'__sizeof__', '__str__', '__subclasshook__', '__unicode__', 'args', 'errno',
'filename', 'message', ' strerror', 'winerror']
Exception Attributes:32
So the options to make _win32_delete_readonly more fine grained remain
either an ugly delay (like my first patch) which may break later or some
awkward code to try to figure out what process (BC3 in this case) is at
fault. Seems like a lot of work for the reward.
I think it's still best to just wrap the osutils.rmtree in diff.py (line
734) in a try/except:
def finish(self):
try:
osutils.rmtree(self._root)
except:
note("The temporary directory [%s] was not cleanly removed." %
self._root)
The result is output which looks like this:
The temporary directory [C:/docume~1/kferrio/locals~1/temp/bzr-diff-yribs_]
was not cleanly removed.
instead of this:
bzr: ERROR: [Error 32] The process cannot access the file because it is
being used by another process:
'C:/docume~1/martitza/locals~1/temp/bzr-diff-yribs_'
I would rather make this a debug-level error than an info-level error, so
that it does not show up on stderr by default. As I read trace.py it is not
obvious to me what is the preferred way to create debug-level logging. So
rather than violate that convention, I opted for note/info. I may look
through the sources for examples of text which get printed only to bzr.log
to figure out the debug level.
Thanks again. I will test a bit more before submitting a merge request.
-M
2009/5/7 Alexander Belchenko <bialix at ukr.net>
> Hi Maritza,
>
> Because I'm the one who was involved in writing _win32_delete_readonly,
> I'll try to provide some hints for you.
>
> def _win32_delete_readonly(function, path, excinfo):
> """Error handler for shutil.rmtree function [for win32]
> Helps to remove files and dirs marked as read-only.
> """
> exception = excinfo[1]
> if function in (os.remove, os.rmdir) \
> and isinstance(exception, OSError) \
> and exception.errno == errno.EACCES:
> make_writable(path)
> function(path)
> else:
> raise
>
> The problem in the code above is that it used too-broad testing for error,
> e.g.
>
> and exception.errno == errno.EACCES:
>
> errno.EACCES it's a POSIX-style error constant, IIUC it's about Access
> Denied.
>
> Fortunately it seems like actually we have there WindowsError exception,
> and therefore can get real Windows Error Code (as of GetLastError()
> returns).
>
> So, if you're adding following debugging prints to the function:
>
> def _win32_delete_readonly(function, path, excinfo):
> """Error handler for shutil.rmtree function [for win32]
> Helps to remove files and dirs marked as read-only.
> """
> exception = excinfo[1]
>
> + print type(exception), dir(exception)
> + print getattr(exception, 'winerror', None)
>
> if function in (os.remove, os.rmdir) \
> and isinstance(exception, OSError) \
> and exception.errno == errno.EACCES:
> make_writable(path)
> function(path)
> else:
> raise
>
>
> You will obtain additional information about your error with
> BeyondCompare3. Especially the second print should print to you actual
> Windows error code (
> http://msdn.microsoft.com/en-us/library/ms681381(VS.85).aspx<http://msdn.microsoft.com/en-us/library/ms681381%28VS.85%29.aspx>
> )
>
> Knowing actual error code there is possible to imprtove
> _win32_delete_readonly function to make more fine-grained analysis why
> deletion of file fails and provide appropriate solution for different cases:
> deleting read-only files, files currently in use etc.
>
> HTH
>
> Maritza Mendez пишет:
>
>>
>>
>> On Wed, May 6, 2009 at 2:01 AM, Alexander Belchenko <bialix at ukr.net<mailto:
>> bialix at ukr.net>> wrote:
>>
>> Maritza Mendez пишет:
>>
>>
>>
>> On Tue, May 5, 2009 at 8:54 AM, Aaron Bentley
>> <aaron at aaronbentley.com <mailto:aaron at aaronbentley.com>
>> <mailto:aaron at aaronbentley.com <mailto:aaron at aaronbentley.com>>>
>>
>> wrote:
>>
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Maritza Mendez wrote:
>> > The more I think about this, the more I think that a cure
>> may be
>> worse
>> > than the problem. So I am leaning toward...
>>
>> > bzr would catch the exception, print a polite message
>> > about the external misbehavior, and just keep going.
>>
>> I think that this would be fine, so long as it's an opt-in
>> behaviour.
>> If you do it as an onerror callback, note that problems may
>> cascade,
>> because failure to delete a file can cause failure to delete its
>> containing directory.
>>
>> Aaron
>>
>> I think it is "opt-in" enough to limit the response to catching
>> OSError and ignoring all other exceptions. What do you think?
>> At worst, bzr would continue to miss some things it misses now.
>>
>> I have to remind myself that this whole discussion comes from
>> the fact that _win32_delete_readonly does not robustly handle
>> failures in osutils.rmtree call to shutil.rmtree. Since that
>> seems to be the whole purpose of _win32_delete_readonly, I might
>> suggest that osutils.rmtree and _win32_delete_readonly do not
>> meet design requirements.
>>
>>
>> Which one design requirements?
>>
>> The _win32_delete_readonly function intended to help in the cases
>> where bzr need to remove file marked as read-only. Nothing more.
>> There is special test in test_osutils.py to ensure this behavior.
>>
>>
>> Point taken. I have not seen any design document for bzr, unless email
>> archives count. I'm just saying that the purpose of osutils.rmtree
>> _appears_ to be to deal with files which are (for whatever reason) readonly
>> on Windows and need to be rendered into a removable state. If that is the
>> (derived) requirement, then the current implemnatation is good, but not
>> foolproof, as we see.
>>
>> -M
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.ubuntu.com/archives/bazaar/attachments/20090510/680eba8f/attachment-0001.htm
More information about the bazaar
mailing list