Closed Bug 960496 Opened 8 years ago Closed 8 years ago

[mozfile] mozfile.remove() doesn't take OSError into account for retrying the removal

Categories

(Testing :: Mozbase, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(1 file, 1 obsolete file)

As seen today during a Mozmill run with restart tests, mozfile.remove() fails on OS X when Mozmill is finally shutting down the application. At this time the profile is still in use and cannot be removed. Retrying fixes the problem for me.
Attached patch Patch v1 (obsolete) — Splinter Review
It's interesting where we could fail with file operations! I have seen so much troubles today on OS X! Looks like that when we call shutils.rmtree() or change the permissions, the application ran can still remove files from the folder, and we would fail. To be on the safe side we should really call exists() before each and every file operation!

This patch has been heavily tested on OS X, and I will continue on Linux and Windows. If you want I can also submit a try server run.
Attachment #8361255 - Flags: review?(ahalberstadt)
Comment on attachment 8361255 [details] [diff] [review]
Patch v1

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

::: mozfile/mozfile/mozfile.py
@@ +152,5 @@
>          """
>          retry_count = 0
>          while True:
> +            failure = False
> +            errno = None

Isn't that line overriding already imported module?

@@ +169,2 @@
>                  # Error 145 == The directory is not empty
> +                if e.winerror not in [5, 32, 145]:

Not sure if errno has an windows alternative, but if it does it would be good to apply.

@@ +171,5 @@
> +                    raise
> +                failure = True
> +                errno = e.winerror
> +
> +            except OSError, e:

Totally matter of taste, but i would create shared exception block
for WindowsError and OSError as the result of handling these exception is the same.
I would create only map in which platform is the keys, and errnos are values and errno would be smartly getattr'ed from object (trying winerror/errno respectfully).

@@ +175,5 @@
> +            except OSError, e:
> +                # Error   2 == No such file or directory (for entries in subfolders)
> +                # Error  39 == The directory is not empty
> +                # Error  66 == The directory is not empty
> +                if e.errno not in [2, 39, 66]:

Hardcoded integes, i think it would be good to use some already defined constants (like a already mentioned "errno" module).

@@ +181,5 @@
> +                failure = True
> +                errno = e.errno
> +
> +            finally:
> +                if failure:

Again, maybe we should avoid unecessary nesting?
Also it's a little strange, shouldn't we create section:
except: failure = True 
To catch the rest of exceptions? It would be easier for next person to see failures in their code.

@@ +189,5 @@
> +                    retry_count += 1
> +
> +                    try:
> +                        message = os.strerror(errno)
> +                    except ValueError:

If you pass None, then strerror raises TypeError Exception.

@@ +190,5 @@
> +
> +                    try:
> +                        message = os.strerror(errno)
> +                    except ValueError:
> +                        message = "Unknown failure"

I think it would be better to try extract message from last available exception than hide this.
It will be easier to debug what happened.

@@ +206,4 @@
>  
>      # Sets specified pemissions depending on filetype.
>      def update_permissions(path):
> +        if os.path.exists(path):

it's a matter of a personal taste, but i would made:
if not os.path.exists(path): return

To avoid unecessary nesting of next code-parts.
Attachment #8361255 - Flags: review-
Comment on attachment 8361255 [details] [diff] [review]
Patch v1

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

::: mozfile/mozfile/mozfile.py
@@ +152,5 @@
>          """
>          retry_count = 0
>          while True:
> +            failure = False
> +            errno = None

Isn't that line overriding already imported module?

@@ +169,2 @@
>                  # Error 145 == The directory is not empty
> +                if e.winerror not in [5, 32, 145]:

Not sure if errno has an windows alternative, but if it does it would be good to apply.

@@ +171,5 @@
> +                    raise
> +                failure = True
> +                errno = e.winerror
> +
> +            except OSError, e:

Totally matter of taste, but i would create shared exception block
for WindowsError and OSError as the result of handling these exception is the same.
I would create only map in which platform is the keys, and errnos are values and errno would be smartly getattr'ed from object (trying winerror/errno respectfully).

@@ +175,5 @@
> +            except OSError, e:
> +                # Error   2 == No such file or directory (for entries in subfolders)
> +                # Error  39 == The directory is not empty
> +                # Error  66 == The directory is not empty
> +                if e.errno not in [2, 39, 66]:

Hardcoded integes, i think it would be good to use some already defined constants (like a already mentioned "errno" module).

@@ +181,5 @@
> +                failure = True
> +                errno = e.errno
> +
> +            finally:
> +                if failure:

Again, maybe we should avoid unecessary nesting?
Also it's a little strange, shouldn't we create section:
except: failure = True 
To catch the rest of exceptions? It would be easier for next person to see failures in their code.

@@ +189,5 @@
> +                    retry_count += 1
> +
> +                    try:
> +                        message = os.strerror(errno)
> +                    except ValueError:

If you pass None, then strerror raises TypeError Exception.

@@ +190,5 @@
> +
> +                    try:
> +                        message = os.strerror(errno)
> +                    except ValueError:
> +                        message = "Unknown failure"

I think it would be better to try extract message from last available exception than hide this.
It will be easier to debug what happened.

@@ +206,4 @@
>  
>      # Sets specified pemissions depending on filetype.
>      def update_permissions(path):
> +        if os.path.exists(path):

it's a matter of a personal taste, but i would made:
if not os.path.exists(path): return

To avoid unecessary nesting of next code-parts.
Attachment #8361255 - Flags: review-
:whimboo
I've applied patch at my version of mozbase at windows 8.1 and general mozbase tests are passing without fails.

I hope that i've helped.
Comment on attachment 8361255 [details] [diff] [review]
Patch v1

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

I agree with most of the review comments from :jot, so please address those and then r? me again.
Attachment #8361255 - Flags: review?(ahalberstadt)
(In reply to Jarek Śmiejczak from comment #3)
> > +            errno = None
> 
> Isn't that line overriding already imported module?

Oh, we didn't need the errno module in my last patch. So that was not a problem. But when we are going to use constants compared to numbers for the ids, the module is necessary and I have to change that here. That's correct.

> >                  # Error 145 == The directory is not empty
> > +                if e.winerror not in [5, 32, 145]:
> 
> Not sure if errno has an windows alternative, but if it does it would be
> good to apply.

As I have seen now, WindowsError is a subclass of OSError and also has the errno attribute, which is mapped to the errno.h values:
http://docs.python.org/2/library/exceptions.html#exceptions.WindowsError

> > +            except OSError, e:
> 
> Totally matter of taste, but i would create shared exception block
> for WindowsError and OSError as the result of handling these exception is
> the same.
> I would create only map in which platform is the keys, and errnos are values
> and errno would be smartly getattr'ed from object (trying winerror/errno
> respectfully).

I found an even better solution. There is no need to catch WindowsError. It's totally enough to catch OSError given that it is the parent class. It makes the code way easier.

> > +            except OSError, e:
> > +                # Error   2 == No such file or directory (for entries in subfolders)
> > +                # Error  39 == The directory is not empty
> > +                # Error  66 == The directory is not empty
> > +                if e.errno not in [2, 39, 66]:
> 
> Hardcoded integes, i think it would be good to use some already defined
> constants (like a already mentioned "errno" module).

I changed that with references.

> Again, maybe we should avoid unecessary nesting?
> Also it's a little strange, shouldn't we create section:
> except: failure = True 
> To catch the rest of exceptions? It would be easier for next person to see
> failures in their code.

We only want to catch OSError exception here, on which we will retry if the error code matches. Anything else should abort the method, and gets propagated accordingly.

> > +                    try:
> > +                        message = os.strerror(errno)
> > +                    except ValueError:
> 
> If you pass None, then strerror raises TypeError Exception.

I removed that call because e.strerror already provides us with the message.

> >      # Sets specified pemissions depending on filetype.
> >      def update_permissions(path):
> > +        if os.path.exists(path):
> 
> it's a matter of a personal taste, but i would made:
> if not os.path.exists(path): return
> 
> To avoid unecessary nesting of next code-parts.

Makes sense. Changed that.

The new patch will be up in a bit.
Attached patch patch v2Splinter Review
Updated patch. I will give it a deep testing on the platforms while it is under review.
Attachment #8361255 - Attachment is obsolete: true
Attachment #8361538 - Flags: review?(ahalberstadt)
Comment on attachment 8361538 [details] [diff] [review]
patch v2

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

Lgtm!

::: mozfile/mozfile/mozfile.py
@@ +148,5 @@
>          retry_count = 0
>          while True:
>              try:
> +                # We have to check for existence of the path because another
> +                # process could have already been removed it

nit: remove 'been'

@@ +178,4 @@
>  
>      # Sets specified pemissions depending on filetype.
>      def update_permissions(path):
> +        if not os.path.exists(path):

Are you sure we shouldn't just fail in this case?
Attachment #8361538 - Flags: review?(ahalberstadt) → review+
(In reply to Andrew Halberstadt [:ahal] from comment #8)
> >      # Sets specified pemissions depending on filetype.
> >      def update_permissions(path):
> > +        if not os.path.exists(path):
> 
> Are you sure we shouldn't just fail in this case?

Then we would have to update the caller, which is while traversing all the sub folders and files. Between
the call to os.walk() and updating the last file/folder other applications can remove, or remove the files/folders. What would you like more?
Flags: needinfo?(ahalberstadt)
If this is easier, then I'm fine just leaving it.
Flags: needinfo?(ahalberstadt)
Landed on master with the nit included:
https://github.com/mozilla/mozbase/commit/a7272b2a25afde18ec10c8e098c53db935499139
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.