Fix silent failures in test runner that lead to long timeouts

Fix silent failures in test runner that lead to long timeouts

Previously the test runner was using pexpect with a timeout to run the
test script. The problem with this is that it can cause the test runner
to hang if the child test script exits while still having live child
processes of its own. The exact sequence of operations leading to the
failure is:

1. TestRunner calls pexpect.run('test_script', timeout, ...).
2. The test script spawns at least one sub process (e.g., the host
   forwarder), which is attached to the same stdout as the test script
   itself.
3. Pexpect calls select on the test script's stdout file descriptor to
   receive output from the script. This is a blocking call.
4. The test script dies for some reason (e.g., disconnected device),
   failing to clean up all the sub processes it created in step 2.
5. Because the sub processes still have a reference to the
   (now-dead) test script's stdout, the select call in step #3 is not
   unblocked, so it hangs until the timeout fires (1.5h by default).

The root cause of the problem is that the test script's sub processes
outlive their parent (which is standard Unix behavior). In order to fix
this we would have to change all the known daemons spawned by every test
script to be proper daemons that release the controlling terminal and
make the test script kill all of its subprocesses when exiting
(i.e., prctl(PR_SET_PDEATHSIG, TERM)).

Because the above fix involves a large number of changes and is a little
fragile in the sense that we may miss updating some scripts by mistake,
this patch takes a defensive approach. Namely, we fix the problem by
replacing the use of pexpect with a custom loop which combines the call
to select() as well as polling for the exit status of the test script.
The call to select uses a smaller timeout (1s), so we can detect a dying
test script almost immediately.

This patch also simplifies some existing code in cmd_helper.

BUG=355952
TEST=src/build/android/test_runner.py perf -v --release --single-step \
     --src/tools/perf/run_benchmark -v --output-format=buildbot \
     --report-root-metrics --browser=android-chromium-testshell \
     rasterize_and_record_micro.key_mobile_sites

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

[Andrew Hayden (chromium.org), 2014-05-28 21:43:37]

LGTM, and kudos for tracking this down and fixing it.

https://codereview.chromium.org/300063017/diff/1/build/android/pylib/perf/tes...
File build/android/pylib/perf/test_runner.py (right):

https://codereview.chromium.org/300063017/diff/1/build/android/pylib/perf/tes...
build/android/pylib/perf/test_runner.py:204: exit_code, output =
cmd_helper.GetCmdStatusAndOutputWithTimeout(
Minor thing but for the sake of conformity maybe we should reverse the order of
the returned tuple to match the existing code?

[Sami, 2014-05-28 22:33:32]

On 2014/05/28 21:43:37, Andrew Hayden wrote:
> Minor thing but for the sake of conformity maybe we should reverse the order
of
> the returned tuple to match the existing code?

cmd_utils uses (code, output) so it looks like pexpect was backwards here.

[Primiano Tucci (use gerrit), 2014-05-28 22:44:09]

LGTM (it has ben a loooooong night) and thanks for cleaning up the crazy temp
file thing in GetCmdStatusAndOutput.

[Sami, 2014-05-28 22:44:28]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-05-28 22:45:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/300063017/20001

[commit-bot: I haz the power, 2014-05-29 02:58:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-05-29 03:01:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-29 03:01:28]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Andrew Hayden (chromium.org), 2014-05-29 07:58:18]

Windoze:

Traceback (most recent call last):
  <module> at tools\telemetry\run_tests:9
    from telemetry.unittest import gtest_testrunner
  <module> at tools\telemetry\telemetry\__init__.py:21
    from telemetry.core.browser_options import BrowserFinderOptions
  <module> at tools\telemetry\telemetry\core\browser_options.py:12
    from telemetry.core import browser_finder
  <module> at tools\telemetry\telemetry\core\browser_finder.py:11
    from telemetry.core.backends.chrome import android_browser_finder
  <module> at
tools\telemetry\telemetry\core\backends\chrome\android_browser_finder.py:18
    from telemetry.core.backends import adb_commands
  <module> at tools\telemetry\telemetry\core\backends\adb_commands.py:22
    from pylib import android_commands  # pylint: disable=F0401
  <module> at build\android\pylib\android_commands.py:25
    import cmd_helper
  <module> at build\android\pylib\cmd_helper.py:7
    import fcntl
ImportError: No module named fcntl


http://stackoverflow.com/questions/1422368/fcntl-substitute-on-windows

:(

[Andrew Hayden (chromium.org), 2014-05-29 08:14:15]

FWIW I have spent the past 20 minutes (Internet time: eternity) looking through
stackoverflow and python forums, and the (sadly) clear consensus is that for
nonblocking reads in Windows you will need to spawn a new thread that does the
reads and either sticks the data in a Queue object (which has nonblocking read
methods) or something similar. There doesn't appear to be any equivalent to
setting non-blocking in msvcrt (the "equivalent" of fcntl for win32, but not
really) and even if you could it looks like it's only supported there on
sockets, not file descriptors.

Sadly I think we'll have to add another thread to do this in a cross-platform
manner.

[Sami, 2014-05-29 14:54:44]

On 2014/05/29 08:14:15, Andrew Hayden wrote:
> FWIW I have spent the past 20 minutes (Internet time: eternity) looking
through
> stackoverflow and python forums, and the (sadly) clear consensus is that for
> nonblocking reads in Windows you will need to spawn a new thread that does the
> reads and either sticks the data in a Queue object (which has nonblocking read
> methods) or something similar. There doesn't appear to be any equivalent to
> setting non-blocking in msvcrt (the "equivalent" of fcntl for win32, but not
> really) and even if you could it looks like it's only supported there on
> sockets, not file descriptors.
> 
> Sadly I think we'll have to add another thread to do this in a cross-platform
> manner.

The previous module used here -- pexpect -- isn't supported under Windows either
so we're not losing anything here. Other bits of pylib are also Unix-specific,
which is a little unfortunate.

[Sami, 2014-05-29 14:54:49]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-05-29 14:57:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/300063017/40001

[commit-bot: I haz the power, 2014-05-29 19:02:25]

Change committed as 273530