Ensure host_forwarder is killed when setting up/tearing down sharding.

Ensure host_forwarder is killed when setting up sharding.

This fixes the following issue:
- TestSharder.SetupSharding() in run_java_tests.py wasn't calling
  the super class' SetupSharding() method which kills host_forwarder. The call
  to Forwarder.KillHost() was moved to RunShardedTest() which isn't supposed to
  be overridden. This should be more robust.

Additionally in case '$ host_forwarder kill-server' failed for any reason,
Forwarder.KillHost() now falls back to pkill.

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

[felipeg, 2012-11-29 14:56:15]

lgtm

[Philippe, 2012-11-29 15:00:27]

On 2012/11/29 14:56:15, felipeg1 wrote:
> lgtm

I'm waiting for you Marcus :)

[bulach, 2012-11-29 15:50:30]

lgtm, but a suggestion below... if possible, I'd prefer avoiding introducing
"atexit" in "library" components if possible..

https://chromiumcodereview.appspot.com/11421149/diff/1/build/android/pylib/ba...
File build/android/pylib/base_test_sharder.py (right):

https://chromiumcodereview.appspot.com/11421149/diff/1/build/android/pylib/ba...
build/android/pylib/base_test_sharder.py:72: """Notifies that we completed the
tests."""
add a "pass" here..

https://chromiumcodereview.appspot.com/11421149/diff/1/build/android/pylib/ba...
build/android/pylib/base_test_sharder.py:90:
atexit.register(self._KillHostForwarder)
does this work? self._KillHostForwarder is a method, which requires "self" as a
first param... I've never used atexit before, but maybe this needs to pass
"self" as param..

registering atexits though, is pretty hairy... I'd much rather ensure that the
entry-point cleans up everything that may have been left behind, than trying to
cleanup via atexit... wdyt?

[Philippe, 2012-11-29 17:53:31]

Please take another look Marcus.

https://chromiumcodereview.appspot.com/11421149/diff/1/build/android/pylib/ba...
File build/android/pylib/base_test_sharder.py (right):

https://chromiumcodereview.appspot.com/11421149/diff/1/build/android/pylib/ba...
build/android/pylib/base_test_sharder.py:72: """Notifies that we completed the
tests."""
On 2012/11/29 15:50:30, bulach wrote:
> add a "pass" here..

Oops :)

https://chromiumcodereview.appspot.com/11421149/diff/1/build/android/pylib/ba...
build/android/pylib/base_test_sharder.py:90:
atexit.register(self._KillHostForwarder)
On 2012/11/29 15:50:30, bulach wrote:
> does this work? self._KillHostForwarder is a method, which requires "self" as
a
> first param... I've never used atexit before, but maybe this needs to pass
> "self" as param..
> 
> registering atexits though, is pretty hairy... I'd much rather ensure that the
> entry-point cleans up everything that may have been left behind, than trying
to
> cleanup via atexit... wdyt?

This seems to work, it is one of the ways to pass a method and the instance it
is called on in Python (which I discovered recently to be honest).

Yeah I totally forgot that pylib was a library used outside the regular testing
infrastructure (e.g. telemetry).
We do kill the host_forwarder during setup so I guess I could just get rid of
the tear down step assuming that all the clients using the forwarder do kill
host_forwarder during setup as well (I will double check that).
But I would still be a bit more confident if the tear down happens. I added a
try/finally that should cover us for most of the cases (the others being when
python quits for a reason other than an exception).

[bulach, 2012-11-29 18:36:24]

lgtm, but one suggestion below..

just to clarify: since this script can be called (and interrupted :) both
interactively and via the buildbots, I think it's extremely hard to try to
maintain a known "post-condition"... it's probably better to just ensure that
the "pre-conditions" are known and put into that state... 

trying to ensure post-conditions makes the code harder to read, and it can't
never be fully guaranteed to work..

wdyt?

https://chromiumcodereview.appspot.com/11421149/diff/2006/build/android/pylib...
File build/android/pylib/base_test_sharder.py (right):

https://chromiumcodereview.appspot.com/11421149/diff/2006/build/android/pylib...
build/android/pylib/base_test_sharder.py:92: try:
hmm, this try..finally block is really long and makes it harder to read with the
other ones inside... since the only exception that can be thrown is on 148, at
which point all hope is lost, I'd suggest keeping as it was, and then just call
self._KillHostForwarder() at the end....

[Philippe, 2012-11-30 09:01:54]

Thanks Marcus!

https://chromiumcodereview.appspot.com/11421149/diff/2006/build/android/pylib...
File build/android/pylib/base_test_sharder.py (right):

https://chromiumcodereview.appspot.com/11421149/diff/2006/build/android/pylib...
build/android/pylib/base_test_sharder.py:92: try:
On 2012/11/29 18:36:24, bulach wrote:
> hmm, this try..finally block is really long and makes it harder to read with
the
> other ones inside... since the only exception that can be thrown is on 148, at
> which point all hope is lost, I'd suggest keeping as it was, and then just
call
> self._KillHostForwarder() at the end....

Done. Yes I agree. It's not worth making the code more complicated given that
this wouldn't cover 100% of the cases.

[commit-bot: I haz the power, 2012-11-30 09:03:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/11421149/8001

[commit-bot: I haz the power, 2012-11-30 10:56:34]

Change committed as 170437