[Android] Delete temporaries in SetProtectedFileContents.

[Android] Delete temporaries in SetProtectedFileContents.

BUG=371054
NOTRY=true
R=yfriedman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269090

[jbudorick, 2014-05-07 22:05:13]

This could also save the value returned by command_runner, delete the temps, and
then return the value. However, since SetProtectedFileContents didn't return
anything prior to a week ago (https://codereview.chromium.org/251743003), and
nothing depends on its return value, having it not return at all seemed like the
better option.

[Yaron, 2014-05-07 22:09:43]

lgtm

Can we go back and delete the temp files on the affected bots?

[jbudorick, 2014-05-07 22:15:40]

On 2014/05/07 22:09:43, Yaron wrote:
> lgtm
> 
> Can we go back and delete the temp files on the affected bots?

Yes. Doing so may cause a transient break if you happen to delete the temp files
while the bot is executing SetProtectedFileContents, but hitting that seems
unlikely.

[jbudorick, 2014-05-07 22:15:45]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-05-07 22:19:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jbudorick@chromium.org/272713002/1

[craigdh, 2014-05-08 00:04:09]

https://codereview.chromium.org/272713002/diff/1/build/android/pylib/android_...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/272713002/diff/1/build/android/pylib/android_...
build/android/pylib/android_commands.py:1180: self.RunShellCommand('rm ' +
temp_file)
this should probably be done as a try/finally to ensure these always get called.

[jbudorick, 2014-05-08 00:13:06]

On 2014/05/08 00:04:09, craigdh wrote:
>
https://codereview.chromium.org/272713002/diff/1/build/android/pylib/android_...
> File build/android/pylib/android_commands.py (right):
> 
>
https://codereview.chromium.org/272713002/diff/1/build/android/pylib/android_...
> build/android/pylib/android_commands.py:1180: self.RunShellCommand('rm ' +
> temp_file)
> this should probably be done as a try/finally to ensure these always get
called.

I think the only times we'll get exceptions thrown here are when we're having
trouble interacting with the device anyway. In that case, we'd probably wind up
throwing another exception when we go to delete the temp file (and not deleting
the file from the device). I suppose we could eat any exceptions thrown while in
the finally block.

This may also be the case after the DeviceUtils refactor.

[chromium-reviews, 2014-05-08 00:25:53]

At the moment that's mostly true, but considering the function being called
is only known at runtime being robust and future proof is probably good. I
agree you should eat those finally exceptions (though they'll probably
expose the same root cause -- device unreachable).


On Wed, May 7, 2014 at 6:13 PM, <jbudorick@chromium.org> wrote:

> On 2014/05/08 00:04:09, craigdh wrote:
>
> https://codereview.chromium.org/272713002/diff/1/build/
> android/pylib/android_commands.py
>
>> File build/android/pylib/android_commands.py (right):
>>
>
>
> https://codereview.chromium.org/272713002/diff/1/build/
> android/pylib/android_commands.py#newcode1180
>
>> build/android/pylib/android_commands.py:1180: self.RunShellCommand('rm '
>> +
>> temp_file)
>> this should probably be done as a try/finally to ensure these always get
>>
> called.
>
> I think the only times we'll get exceptions thrown here are when we're
> having
> trouble interacting with the device anyway. In that case, we'd probably
> wind up
> throwing another exception when we go to delete the temp file (and not
> deleting
> the file from the device). I suppose we could eat any exceptions thrown
> while in
> the finally block.
>
> This may also be the case after the DeviceUtils refactor.
>
> https://codereview.chromium.org/272713002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jbudorick, 2014-05-08 16:47:11]

The CQ bit was unchecked by jbudorick@chromium.org

[jbudorick, 2014-05-08 16:49:48]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-05-08 16:55:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jbudorick@chromium.org/272713002/1

[Yaron, 2014-05-08 22:18:26]

Committed patchset #1 manually as r269090 (tree was closed).