Closed
Bug 861984
Opened 11 years ago
Closed 11 years ago
mozfile rmtree should retry on windows directory not empty
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: k0scist, Assigned: markh)
References
Details
Attachments
(1 file, 3 obsolete files)
2.34 KB,
patch
|
wlach
:
review+
|
Details | Diff | Splinter Review |
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)
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
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)
Reporter | ||
Comment 3•11 years ago
|
||
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-
Assignee | ||
Comment 4•11 years ago
|
||
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 5•11 years ago
|
||
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 6•11 years ago
|
||
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+
Assignee | ||
Comment 7•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Comment 8•11 years ago
|
||
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-
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #828334 -
Attachment is obsolete: true
Attachment #828926 -
Flags: review?(wlachance)
Comment 10•11 years ago
|
||
Comment on attachment 828926 [details] [diff] [review] With requested changes Awesome, thank you!
Attachment #828926 -
Flags: review?(wlachance) → review+
Comment 11•11 years ago
|
||
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.
Description
•