Closed Bug 861984 Opened 11 years ago Closed 11 years ago

mozfile rmtree should retry on windows directory not empty

Categories

(Testing :: Mozbase, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: k0scist, Assigned: markh)

References

Details

Attachments

(1 file, 3 obsolete files)

see https://bugzilla.mozilla.org/show_bug.cgi?id=858537 for more context.

For windows, mozfile should simply have a retry for this specific
error during os.rmdir . jaws' attached logs show mozfile.rmtree
failing to remove a directory claiming it was not empty, but then the
directory being empty immediately afterward, leading me to believe
that there's some subtle race condition here. (w/ apologies to :ted)
Blocks: 858537
Could it be that some files were still in use on Windows under the folder which should be deleted? We encountered a similar situation last week when we tried to uninstall Firefox and rmtree failed. I have implemented a retry mechanism for it. See my patch on bug 857956.
This is slightly different than the patch referenced in comment 1, as it only targets Windows and only looks for a couple of specific errors known to be symptoms of this problem.

(I've actually had this patch in my tree for over a week, but it took until now for me to hit the problem and see the new message be printed.  It was actually printed 3 times, implying that either we should sleep a little longer, or should retry more times)
Attachment #826596 - Flags: feedback?(jhammel)
Comment on attachment 826596 [details] [diff] [review]
Retry os.rmdir and os.remove operations on Windows in certain error cases

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

Please make a patch against the github repository as per https://wiki.mozilla.org/Auto-tools/Projects/Mozbase#Working_on_Mozbase_and_Contributing_Patches

I'm also not sure if callWithWindowsRetry should be a top-level function, but its fine as-is for now.

::: testing/mozbase/mozfile/mozfile/mozfile.py
@@ +110,4 @@
>  
>      return top_level_files
>  
> +### utility for broken windows.

please add an additional blank line following the comment

@@ +113,5 @@
> +### utility for broken windows.
> +def callWithWindowsRetry(func, *args):
> +    # It's possible to see spurious errors on Windows due to various things
> +    # keeping a handle to the directory open (explorer, virus scanners, etc)
> +    # So we try a few times if it fails with a known error.

please convert this to a docstring

@@ +121,5 @@
> +            func(*args)
> +            break
> +        except Exception as e:
> +            retryCount += 1
> +            if retryCount == 5 or os.name != 'nt':

please parameterize this; make it a (named) argument

@@ +128,5 @@
> +            # Error 32 == "The process cannot access the file because it is being used by another process"
> +            if not isinstance(e, WindowsError) or e.winerror not in [32, 145]:
> +                raise
> +            print "Retrying file/directory removal as Windows thinks it is in use"
> +            time.sleep(0.5)

please parameterize this; make this a function argument
Attachment #826596 - Flags: feedback?(jhammel) → feedback-
Changes as requested.  I wasn't sure on the best way to handle "*args" and named params, so I replaced |*args| with |filename|, which also means the name of the file/directory can be printed.  The new params have defaults, so I didn't actually specify them at the call sites.
Attachment #826596 - Attachment is obsolete: true
Attachment #827696 - Flags: feedback?(jhammel)
Comment on attachment 827696 [details] [diff] [review]
0001-Bug-861984-mozfile-rmtree-should-retry-on-some-Windo.patch

jhammel's out for a bit. I'll take a look at this so you're not waiting too long for an answer. :)
Attachment #827696 - Flags: feedback?(jhammel) → feedback?(wlachance)
Comment on attachment 827696 [details] [diff] [review]
0001-Bug-861984-mozfile-rmtree-should-retry-on-some-Windo.patch

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

Looks good except for some minor nits. Could you address them and then upload a patch with the reviewer assigned to myself?

::: mozfile/mozfile/mozfile.py
@@ +111,5 @@
>      return top_level_files
>  
> +### utility for broken windows.
> +
> +def _callWithWindowsRetry(func, filename, numRetries=5, retryDelay=0.5):

The camel case variable names do not match the style in the rest of mozfile.

Could you rename this to _call_with_windows_retry(func, filename, num_retries, retry_delay) ?

@@ +122,5 @@
> +    while True:
> +        try:
> +            func(filename)
> +            break
> +        except Exception as e:

Could you not just catch WindowsError here? This would remove the need for conditional logic to re-raise exceptions.
Attachment #827696 - Flags: feedback?(wlachance) → feedback+
Attached patch Updated based on feedback (obsolete) — Splinter Review
Thanks for jumping in!

> Could you rename this to _call_with_windows_retry(func, filename, num_retries, retry_delay) ?

I renamed it, but I wasn't clear on whether you were happy with the arg defaults, so I kept them.  It now reads:

def _call_with_windows_retry(func, filename, numRetries=5, retryDelay=0.5):

> Could you not just catch WindowsError here? This would remove the need for conditional logic to re-raise exceptions.

I believe WindowsError would be a NameError on non-windows platforms.
Attachment #827696 - Attachment is obsolete: true
Attachment #828334 - Flags: review?(wlachance)
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Comment on attachment 828334 [details] [diff] [review]
Updated based on feedback

Almost there. Just a few small things I'd like changed.

(In reply to Mark Hammond [:markh] from comment #7)
> Created attachment 828334 [details] [diff] [review]
> Updated based on feedback
> 
> Thanks for jumping in!
> 
> > Could you rename this to _call_with_windows_retry(func, filename, num_retries, retry_delay) ?
> 
> I renamed it, but I wasn't clear on whether you were happy with the arg
> defaults, so I kept them.  It now reads:
> 
> def _call_with_windows_retry(func, filename, numRetries=5, retryDelay=0.5):

Could you rename numRetries->num_retries, retryDelay->retry_delay
 
> > Could you not just catch WindowsError here? This would remove the need for conditional logic to re-raise exceptions.
> 
> I believe WindowsError would be a NameError on non-windows platforms.

Ah yes, you are right. In that case though, I would prefer you put this on top of the file. 

try:
    WindowsError
except NameError:
    WindowsError = None

A bit of ugliness up front but it'll make what's going on in that function clearer.
Attachment #828334 - Flags: review?(wlachance) → review-
Attachment #828334 - Attachment is obsolete: true
Attachment #828926 - Flags: review?(wlachance)
Comment on attachment 828926 [details] [diff] [review]
With requested changes

Awesome, thank you!
Attachment #828926 - Flags: review?(wlachance) → review+
Pushed: https://github.com/mozilla/mozbase/commit/77fa756620f77a5dab1a5b07cb4cdd054695119e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: