Android: makes "RestartAdbdOnDevice" more robust.

Android: makes "RestartAdbdOnDevice" more robust.

Some bots fail on the command that kills adbd on the device,
even though the device should be online and available.

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

[bulach, 2013-11-27 14:59:52]

ptal

[tonyg, 2013-11-27 15:25:54]

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

https://codereview.chromium.org/91563002/diff/1/build/android/pylib/android_c...
build/android/pylib/android_commands.py:470: assert adb_pids, 'Unable to obtain
adbd pid'
heh heh, catching your own assertion is kind of funny.

How about:

logging.info('Killing adbd on the device...')
try:
  if self.KillAll('adbd', signal=signal.SIGTERM, with_su=True):
    logging.info('Waiting for device to settle...')
    self._adb.SendCommand('wait-for-device')
  else:
    logging.error('Failed to kill adbd on the device')
except:
  logging.error('Exception when trying to kill adbd on the device [%s]', e)

[bulach, 2013-11-27 16:17:59]

hehe, thanks tony! another look please?

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

https://codereview.chromium.org/91563002/diff/1/build/android/pylib/android_c...
build/android/pylib/android_commands.py:470: assert adb_pids, 'Unable to obtain
adbd pid'
On 2013/11/27 15:25:55, tonyg wrote:
> heh heh, catching your own assertion is kind of funny.
> 
> How about:
> 
> logging.info('Killing adbd on the device...')
> try:
>   if self.KillAll('adbd', signal=signal.SIGTERM, with_su=True):
>     logging.info('Waiting for device to settle...')
>     self._adb.SendCommand('wait-for-device')
>   else:
>     logging.error('Failed to kill adbd on the device')
> except:
>   logging.error('Exception when trying to kill adbd on the device [%s]', e)

ha! :) good point.. made it a bit clear now.

[tonyg, 2013-11-27 16:24:15]

https://codereview.chromium.org/91563002/diff/30001/build/android/pylib/andro...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/91563002/diff/30001/build/android/pylib/andro...
build/android/pylib/android_commands.py:472:
self._adb.SendCommand('wait-for-device')
The idea of this wait-for-device is that it must be done after killing adbd.
However, if it needs to be done for some other reason, then callers are relying
on an unintended side effect of this method.

I think we should only do this if we actually kill adbd. That will flush out any
callers or other places that need their own wait-for-device for other reasons.

https://codereview.chromium.org/91563002/diff/30001/build/android/pylib/andro...
build/android/pylib/android_commands.py:473: except Except as e:
s/Except/Exception/ -- no?

[bulach, 2013-11-27 18:04:27]

thanks tony! another look?

https://codereview.chromium.org/91563002/diff/30001/build/android/pylib/andro...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/91563002/diff/30001/build/android/pylib/andro...
build/android/pylib/android_commands.py:472:
self._adb.SendCommand('wait-for-device')
On 2013/11/27 16:24:15, tonyg wrote:
> The idea of this wait-for-device is that it must be done after killing adbd.
> However, if it needs to be done for some other reason, then callers are
relying
> on an unintended side effect of this method.
> 
> I think we should only do this if we actually kill adbd. That will flush out
any
> callers or other places that need their own wait-for-device for other reasons.

you're right. I was mistaken, KillAll just returns the len of pids it sent the
signal to, no guarantees if the procs were actually restarted...
I made it slightly more robust by checking that the pid has indeed changed, and
printing an error if they haven't. wdyt?

https://codereview.chromium.org/91563002/diff/30001/build/android/pylib/andro...
build/android/pylib/android_commands.py:473: except Except as e:
On 2013/11/27 16:24:15, tonyg wrote:
> s/Except/Exception/ -- no?

facepalm :(

[tonyg, 2013-11-27 18:12:26]

lgtm

[commit-bot: I haz the power, 2013-11-27 19:07:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/91563002/70001

[commit-bot: I haz the power, 2013-11-28 02:14:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/91563002/70001

[commit-bot: I haz the power, 2013-11-28 02:49:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/91563002/70001

[commit-bot: I haz the power, 2013-11-28 03:16:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/91563002/70001

[commit-bot: I haz the power, 2013-11-28 03:40:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/91563002/70001

[commit-bot: I haz the power, 2013-11-28 06:35:34]

Change committed as 237708