devicemanagerADB removeDir may return before directory is deleted

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gbrown, Assigned: gbrown)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Consider code such as:

        if self.device.dirExists(self.remoteTmpDir):
            self.device.removeDir(self.remoteTmpDir)
        self.device.mkDir(self.remoteTmpDir)

On some devices, when remoteTmpDir initially exists, the end result of executing this is that remoteTmpDir does not exist, at least intermittently. Debugging shows that removeDir was called and issued rm -r, but the mkdir failed because the directory existed; immediately afterwards, the directory does not exist.

It appears that rm -r is still executing when mkdir is called.

(Regression from bug 807841 that introduced the "rm -r" directory removal.)
(Assignee)

Updated

6 years ago
Blocks: 811411
(In reply to Geoff Brown [:gbrown] from comment #0)
> Consider code such as:
> 
>         if self.device.dirExists(self.remoteTmpDir):
>             self.device.removeDir(self.remoteTmpDir)
>         self.device.mkDir(self.remoteTmpDir)
> 
> On some devices, when remoteTmpDir initially exists, the end result of
> executing this is that remoteTmpDir does not exist, at least intermittently.
> Debugging shows that removeDir was called and issued rm -r, but the mkdir
> failed because the directory existed; immediately afterwards, the directory
> does not exist.
> 
> It appears that rm -r is still executing when mkdir is called.
> 
> (Regression from bug 807841 that introduced the "rm -r" directory removal.)

This problem is because _runCmd returns a Popen object, which you're supposed to wait on. We don't always do that, hence we have silly race conditions that can result in things happening in the wrong order. I remember a b2g-related bug was filed about a similar issue a while back, not sure what happened with it (probably nothing).
(Assignee)

Comment 2

6 years ago
Created attachment 691506 [details] [diff] [review]
read output from rm -r command

Simply reading the command output seems to effectively ensure that the directory is deleted before returning from removeDir.

(I also deleted unreferenced function _removeSingleDir() -- just to clean up.)
Attachment #691506 - Flags: review?(wlachance)
Comment on attachment 691506 [details] [diff] [review]
read output from rm -r command

Ok, so I'm going to give this an r+, but only on the condition that you take my advice: :)

I really don't think reading the output (into an unused variable at that) is the right approach here. I remember that was one of the things that was causing problems in b2g (the read would finish before the process was finished). 

You can easily just wait for the process to finish with Popen's wait() function:

self._runCmd(["shell", "rm", "-r", remoteDir]).wait()
Attachment #691506 - Flags: review?(wlachance) → review+
(Assignee)

Comment 4

6 years ago
Created attachment 691545 [details] [diff] [review]
updated for review comments

:wlach - thanks! That is tidier and just as effective. Here is the updated patch. Would you mind checking it in for me?
Attachment #691506 - Attachment is obsolete: true
Attachment #691545 - Flags: review+
Pushed, thanks!

https://github.com/mozilla/mozbase/commit/66e6c444a5c43a6f28754dc2160f6335610c994a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.