[Telemetry] Refactor common Android cleanup functions.

build/android/pylib/android_commands.py

Index: build/android/pylib/android_commands.py
diff --git a/build/android/pylib/android_commands.py b/build/android/pylib/android_commands.py
index 6f9b99cc2405f9eab6760fe5d71d4fa5c5a9196c..75940800d49ddcefa042a4fbd05243ae504c5e75 100644
--- a/build/android/pylib/android_commands.py
+++ b/build/android/pylib/android_commands.py
@@ -465,6 +465,7 @@ class AndroidCommands(object):
 
   def RestartAdbdOnDevice(self):
     logging.info('Killing adbd on the device...')
+    self._adb.SendCommand('wait-for-device')

[tonyg, 2013/11/25 15:46:33]

While I love the fix, it seems misplaced to me.

This method doesn't do anything special before this that would require
wait-for-device. So that tells me that the wait-for-device belongs elsewhere
(e.g. at the end of RestartAdbServer). Otherwise, you could foresee a
wait-for-device call at the beginning of every method in this file.

Also, does this fix http://crbug.com/322828 ?

[Primiano Tucci (use gerrit), 2013/11/25 17:26:51]

Right, I should have added more context, my bad.
The call stack involved here is.
android_commands.py::RunShellCommand
 -> adb_interface.py::SendShellCommand
   -> adb_interface.py::SendCommand
     -> run_command.py::RunCommand

Now, run_command.py::RunCommand is not robust enough to tolerate adb server
restarts. If you look to its code it has a retry_count. However the retry logic
kicks in only in case of a command timeout, and not in the case in which the
device temporary disappear (e.g., while adb server is respawning).

I think (but I might be wrong, I still miss some adb details) that the main
problem is that at the time RestartAdbdOnDevice is called, there is no guarantee
that adbd is already running, thus KillAll fails. I think that in my specific
case, this happens because of the call to RestartAdbServer() which precedes this
call in CleanupPendingProcesses.

Looking better at the code, it looks like waiting for the adb (host) daemon in
StartAdbServer with psgrep is not enough and does not guarantee that the device
will be visible in the next call to adb whatever.

Ideally we'd move wait-for-device in StartAdbServer.
However, the adb server (host side) is only one, while the devices can be many,
and there is no knowledge about them in StartAdbServer. That's why I placed the
wait-for-device here (does it make sense till this point?).
The only other place in which it could make sense IMHO is in
CleanupPendingProcesses() itself.
I suspect something similar is happening. But I miss a mental picture of the
current adb stop-start flow in the clank bots. Is it possible that, in that bug,
the adb server was restarted just before the failure point?

[bulach, 2013/11/27 12:10:53]

I agree this is misplaced and StartAdbServer's pgrep condition is insufficient
:)
I think it should be enough do "adb devices" in line 506 before declaring it has
succeeded?
at that point, pgrep would have said there's an adb running, so running any
command that would involve talking to the adb daemon in the host should be
enough? wdyt?

[Primiano Tucci (use gerrit), 2013/11/27 15:14:41]

Oh right, I got lost in the many layers. makes perfect sense.
Very funny that now I can't re-reproduce the issue locally anymore (but I'm sure
I've seen it failing yesterday exactly in that point).
Maybe let's add a note to the crbug.com/322828 and see if the flakiness goes
away back after this cl.

     adb_pids = self.KillAll('adbd', signal=signal.SIGTERM, with_su=True)
     assert adb_pids, 'Unable to obtain adbd pid'
     logging.info('Waiting for device to settle...')