mozfile.rmtree should handle windows directory in use better

RESOLVED WORKSFORME

Status

RESOLVED WORKSFORME
6 years ago
4 years ago

People

(Reporter: k0scist, Unassigned, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=python])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
From https://bugzilla.mozilla.org/show_bug.cgi?id=839786:

"""
Hmmm. It seems silly to trap a WindowsError this high up in the
stack. I almost feel this logic should live in
mozfile.mozfile.rmtree() and rmtree() should raise its own Exception
type detailing what failed.
"""

So precisely that. If a directory is in use on windows, this windows
error wil be thrown. mozfile.rmtree should catch it and reraise a more
helpful error, e.g. MozRmtreeError or what not.

AFAIK, this particular error ("directory in use") is a windows-only
error right now; OSX and linux will happily allow you to delete files
that are actively used.

Ideally, once completed this should be mirrored to m-c for e.g. mach use.
Whiteboard: [good first bug][mentor=jmaher][lang=python]
Some additional comments on this bug:
* mozbase (which includes mozfile) lives in mozilla-central/testing/mozbase/
* we will write a patch for mozilla-central- not github or anything like that
* a patch for this should include a test case to verify the functionality
Hey, i want to take this bug.
Should i raise an error or print something like 'Directory in use'. If error has to be raised what it should be.
(Assignee)

Updated

4 years ago
Mentor: jmaher
Whiteboard: [good first bug][mentor=jmaher][lang=python] → [good first bug][lang=python]
Luv-  Thanks for your interest in working on this project, I am excited to help you if needed and see this get resolved.

To answer your question, we should raise an exception.  Here is an example of an exception in mozfile:
http://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozfile/mozfile/mozfile.py#102

Keep in mind the one caveat here is that if this breaks thinks somehow which I am not aware of the wait the api is being used, we will either need to wait for that use of the api to be fixed, or return an error code.
Created attachment 8442143 [details] [diff] [review]
fix_test_841808.patch

edit the tests corresponding to the changes in this bug
In the first patch i have made the changes in the mozfile.remove function as i understood. 
In second patch the changes in the tests correspnding to the first patch are made.
I work on ubuntu and don't have windows so i read all the errno's and thought errno.ETXTBSY as the required one. Tell me if i did something silly and the changes to  that. Thanks
Comment on attachment 8442138 [details] [diff] [review]
fix_841808.patch

Henrik, I took a look at this patch and it is simple and looks to be correct.  You always have great insight into these modules, could you take a minute and look over this?
Attachment #8442138 - Flags: review?(hskupin)
Comment on attachment 8442143 [details] [diff] [review]
fix_test_841808.patch

thanks for modifying this test case!  Is there a chance we need to test for OSError as well?  I suspect not, but since you were just in the code what are your thoughts?
Assignee: nobody → luvagarwal1995su
Status: NEW → ASSIGNED
Comment on attachment 8442138 [details] [diff] [review]
fix_841808.patch

Review of attachment 8442138 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozbase/mozfile/mozfile/mozfile.py
@@ +164,5 @@
>                      raise
>  
> +                if e.errno == errno.ETXTBSY:
> +                    raise Exception("mozfile.remove: the directory '%s' is in use" %
> +                                    path)

So this bug is about directory handling on Windows, but the error type as selected here is for a busy text file. I don't see how this correlates.

Joel, do you know which specific situation will be fixed here? It's not really clear to me. IMO when a directory is still in use we should still re-try to delete it, and not directly throw an exception. Same as the lines above, means the expected error type should be added to the list.
Attachment #8442138 - Flags: review?(hskupin) → review-
Comment on attachment 8442143 [details] [diff] [review]
fix_test_841808.patch

Review of attachment 8442143 [details] [diff] [review]:
-----------------------------------------------------------------

Please try to combine those two patches into a single one. This is more helpful when it comes to testing and landing of the patch.

::: testing/mozbase/mozfile/tests/test_remove.py
@@ +69,5 @@
>          if mozinfo.isWin:
> +            # On the Windows family Exception should be raised. (We
> +            # modified WindowsError to a more comprehensible error
> +            # in the function mozfile.remove)
> +            self.assertRaises(Exception, mozfile.remove, self.tempdir)

WindowsError is subclassed from OSError, and that's why also contains an OSError code. See the code you were modifying in the other patch. So you should keep the reference to OSError here.
So this bug was filed a while back as a non critical path bug.  Essentially we are looking to make this friendlier if the directory cannot be removed.  I don't believe the directory itself can be busy, but a file inside of the directory is the case we care about.  This has cropped up many times (I have seen it in Talos often) when we are cleaning up from a browser run.  

Adding retry logic sounds like a good option, maybe we should do this after 3 retries?
The code which got changed here already is inside the retry loop. Something which would indeed be interesting is the output of such a Talos run. Means, that we can see which specific error is thrown here. Do you have such a reference? 

The description of the newly added exception handler is:

Macro: int ETXTBSY
    An attempt to execute a file that is currently open for writing, or write to a file that is currently being executed. (The name stands for "text file busy".) This is not an error in the GNU system; the text is copied as necessary.
Henrick, to take care of the retry logic which you pointed above (error should not raise until retry_count < 3) the if condition can be anded with retry_count > 2. But i am confused what error type should be considered. And also in comment 10 you said to refer OSError in the test but i am modifying the error to a new one so how can i still refer to OSError (modifying the error is the purpose).
(In reply to Luv Agarwal(:lagarwal) from comment #13)
> Henrick, to take care of the retry logic which you pointed above (error
> should not raise until retry_count < 3) the if condition can be anded with
> retry_count > 2. But i am confused what error type should be considered.

Correct, I'm also not sure about the correct error type here. That's why I was asking Joel if he still has a reference to a broken Talos run, which suffered from this problem.

> And also in comment 10 you said to refer OSError in the test but i am modifying
> the error to a new one so how can i still refer to OSError (modifying the
> error is the purpose).

I wouldn't modify the Exception type here, but simply re-raise it. Something we currently do if we reached the maximum number of retries (see http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozfile/mozfile/mozfile.py#166). Reason is that this will hide all the details where the exception was originally raised, and what the real issue was. Having an OSError with an errno, is way more helpful. Also we currently print strerror to the console right now, in case the deletion failed. See some lines below.
Mentor: hskupin

Comment 15

4 years ago
Created attachment 8487204 [details] [diff] [review]
merge_fix_test_841808.patch
Attachment #8487204 - Flags: review?(jmaher)
Comment on attachment 8487204 [details] [diff] [review]
merge_fix_test_841808.patch

Review of attachment 8487204 [details] [diff] [review]:
-----------------------------------------------------------------

if we raise the original exception we would need to modify the testcase.

Great job on your first attempt at this bug. I suspect we will get this resolved without too many further iterations!

::: testing/mozbase/mozfile/mozfile/mozfile.py
@@ +164,5 @@
>                      raise
>  
> +                if e.errno == errno.ETXTBSY:
> +                    raise Exception("mozfile.remove: the directory '%s' is in use" % path)
> +                    

can we raise the original exception instead of a new one.  While the new message is nice, could we just do what we do on line 164.

also on line 168 here, we have a blank line with spaces, lets make it a blank line

::: testing/mozbase/mozfile/tests/test_remove.py
@@ +75,2 @@
>              self.assertTrue(os.path.exists(self.tempdir))
> +            

nit: you have a blank line with whitespace here, it should be a real blank line
Attachment #8487204 - Flags: review?(jmaher) → review-

Comment 17

4 years ago
(In reply to Joel Maher (:jmaher) from comment #16)

If the original exception is raised, then the 'self.assertRaises(Exception, mozfile.remove, self.tempdir)' line becomes unnecessary, right? As far as I can see, removal of that line is the only modification needed for the test file. Other than the whitespaces, of course, which I'll fix.
would it be possible to assert this is an oserror or windowserror?  maybe we should stick a comment in there with a note indicating that we need to test this exception condition, but it is hard to test.

Comment 19

4 years ago
Created attachment 8487655 [details] [diff] [review]
merge_fix_test_841808_updated.patch
Attachment #8487204 - Attachment is obsolete: true
Attachment #8487655 - Flags: review?(jmaher)
Comment on attachment 8487655 [details] [diff] [review]
merge_fix_test_841808_updated.patch

Review of attachment 8487655 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozbase/mozfile/mozfile/mozfile.py
@@ +163,5 @@
>                  if e.errno not in [errno.EACCES, errno.ENOTEMPTY]:
>                      raise
>  
>                  if e.errno == errno.ETXTBSY:
> +                    raise 

nit, extra ' ' after the raise is not needed.

::: testing/mozbase/mozfile/tests/test_remove.py
@@ +75,1 @@
>              self.assertTrue(os.path.exists(self.tempdir))

honestly I wouldn't remove the assertRaises here because we are testing that keeping the file open and removing the directory throws an exception.  Maybe we should just leave this test alone.
Attachment #8487655 - Flags: review?(jmaher) → review?(hskupin)
Comment on attachment 8487655 [details] [diff] [review]
merge_fix_test_841808_updated.patch

Review of attachment 8487655 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozbase/mozfile/mozfile/mozfile.py
@@ -163,5 @@
>                  if e.errno not in [errno.EACCES, errno.ENOTEMPTY]:
>                      raise
>  
>                  if e.errno == errno.ETXTBSY:
> -                    raise Exception("mozfile.remove: the directory '%s' is in use" % path)

I'm not sure what those changes are. When I look at the latest mozbase code on mxr, this line does not exist:

http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozfile/mozfile/mozfile.py

So where does it come from? Is that an inter diff of one of your former patches?

Also please check the if condition before with the list of two exceptions. The one you have here is not part of the list, so we already raise above, and this code would never be reached.

::: testing/mozbase/mozfile/tests/test_remove.py
@@ +66,5 @@
>  
>          # keep file open and then try removing the dir-tree
>          if mozinfo.isWin:
> +            # On the Windows family WindowsError (subclassed from WindowsError) 
> +            # should be raised.

This comment doesn't really apply to the assertRaises() call, where we are checking for OSError. I would say just remove it.

@@ +75,1 @@
>              self.assertTrue(os.path.exists(self.tempdir))

That's what I also think. We definitely should throw an exception in such a case. Maybe we should replace the bare Exception with an OSError one.
Attachment #8487655 - Flags: review?(hskupin) → review-
Joel, I wonder if we need a patch on this bug at all. I think the changes I did couple of months ago, should perfectly cover this. That might also be why the test we were referring here, already tests what it should test.
Flags: needinfo?(jmaher)
whimboo, that might be the right solution.  it is so easy to fix bugs on accident or forget that they are not applicable.

Please close if that is the problem.  Kishor, this happens sometimes, don't feel bad.  The good news is you have setup a developement environment and have added your first patch.
Flags: needinfo?(jmaher)
Agreed, and also sorry from my side. I should have already seen this with the first review, but badly missed that. 

Kishor, we have a couple of other good opportunities to work on for automation related bugs. If you want an easy bug to fix please check the automation area of bugsahoi: http://www.joshmatthews.net/bugsahoy/?automation=1&unowned=1&simple=1. We are happy to help you whatever you choose.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WORKSFORME
Assignee: luvagarwal1995su → nobody
You need to log in before you can comment on or make changes to this bug.