Cleanup orphaned testserver processes on posix.

Cleanup orphaned testserver processes on posix.

If a chrome test that uses a testserver were to crash, the testserver process ends up being orphaned, potentially affecting subsequent tests. This patch causes child processes of a test case to be killed along with the test case if it were to be abruptly terminated.


BUG=55808
TEST=sync_integration_tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=60192

[Raghu Simha, 2010-09-17 05:20:20]

Sending this out for preliminary review. I can think of a couple of ways of
accomplishing what we're trying to do. This patch implements option 1. Let me
know if you think there's a better way of implementing this change.

1) base::KillProcess() keeps track of any child processes that
base::LaunchApp()/LaunchAppAndGetTask() might have kicked off, and cleans them
up using kill with a pgid instead of a pid.
Pros: Child processes are not left lying around until the next test case runs.
Cons: This change might affect other classes that rely on base:LaunchApp().

2) TestServer::LaunchPython() checks for older versions of itself before
launching a new test server and cleans them up using base::KillProcesses() with
the appropriate filter.
Pros: This change is restricted to code that uses TestServer::LaunchPython().
Cons: Orphaned versions of test servers will stick around until a later test
case run before they are cleaned up.

[Paweł Hajdan Jr., 2010-09-17 15:51:39]

+agl

I'm somewhat worried about making this behavior automatic and implicit.

[agl, 2010-09-17 15:58:41]

NACK.

We don't want to change the default behaviour of these functions. Least surprise
and all that.

In general, it's not a bad idea, but it needs to be explicit. KillProcessGroup,
some way to request a new group for child etc. (Oh, and setpgid(0, 0) is the
typical incantation for that).

Of course, it's all just a bodge for not coding carefully. But I know that we
have a zoo of different code in play when testing and it's not really reasonable
to expect it all to work just right for us.

[Raghu Simha, 2010-09-21 02:06:45]

Thanks for the review. I've reimplemented this fix as you suggested, with the
default behavior remaining unchanged.

PS: You're right -- the chrome tests use a process tree with multiple levels,
where, if a parent process (test case) creates a child process (test server) and
goes on to die, it's left to the grand-parent process (test runner) to clean up
the child. (In windows, we use JobObjects, and the child is automatically killed
when the parent dies.)

[Paweł Hajdan Jr., 2010-09-21 08:55:48]

You should also modify the test server launcher in net/test/test_server.

http://codereview.chromium.org/3423012/diff/11005/14001
File base/process_util.h (right):

http://codereview.chromium.org/3423012/diff/11005/14001#newcode232
base/process_util.h:232: bool LaunchApp(const std::vector<std::string>& argv,
nit: Why not LaunchAppInSameProcessGroup and get rid of the bool param? More
greppable, and more obvious when reading code (instead of a mysterious
true/false parameter which in this case is very important).

http://codereview.chromium.org/3423012/diff/11005/14001#newcode301
base/process_util.h:301: // true if this is successful, false otherwise.
nit: Just "Returns true on success.".

http://codereview.chromium.org/3423012/diff/11005/14002
File base/process_util_posix.cc (right):

http://codereview.chromium.org/3423012/diff/11005/14002#newcode198
base/process_util_posix.cc:198: // Attempts to kill the process group identified
by |process_group_id|. Returns
nit: This comment is the same as in the header. Please remove.

http://codereview.chromium.org/3423012/diff/11005/14002#newcode203
base/process_util_posix.cc:203: DPLOG(ERROR) << "Unable to terminate process
group " << process_group_id;
We should log the error in Release mode too.

http://codereview.chromium.org/3423012/diff/11005/14002#newcode457
base/process_util_posix.cc:457: if (pgid < 0) {
nit: No need for {}

In fact, you don't need the temporary pgid variable either.

Just if (setpgid(...) < 0) return false.

[Raghu Simha, 2010-09-21 18:39:44]

On 2010/09/21 08:55:48, Paweł Hajdan Jr. wrote:
> You should also modify the test server launcher in net/test/test_server.

I have intentionally left out net/test/test_server. Basically, the test runner
(grand parent) cleans up the python test server process (child) if the test case
(parent) dies. We do this by starting the parent in a new process group and
having the child inherit the pgid. Then, the grand parent can clean up the child
since it knows the parent's pid (and therefore, the child's pgid). Making the
child belong to a new pgid will defeat the purpose, since the grand parent has
no idea what the child's pid/pgid will be.

[Raghu Simha, 2010-09-21 18:40:11]

http://codereview.chromium.org/3423012/diff/11005/14001
File base/process_util.h (right):

http://codereview.chromium.org/3423012/diff/11005/14001#newcode232
base/process_util.h:232: bool LaunchApp(const std::vector<std::string>& argv,
Good idea. This should be called LaunchAppInNewProcessGroup though, not
LaunchAppInSameProcessGroup. 

Done.

http://codereview.chromium.org/3423012/diff/11005/14001#newcode301
base/process_util.h:301: // true if this is successful, false otherwise.
On 2010/09/21 08:55:49, Paweł Hajdan Jr. wrote:
> nit: Just "Returns true on success.".

Done.

http://codereview.chromium.org/3423012/diff/11005/14002
File base/process_util_posix.cc (right):

http://codereview.chromium.org/3423012/diff/11005/14002#newcode198
base/process_util_posix.cc:198: // Attempts to kill the process group identified
by |process_group_id|. Returns
On 2010/09/21 08:55:49, Paweł Hajdan Jr. wrote:
> nit: This comment is the same as in the header. Please remove.

Removed.

http://codereview.chromium.org/3423012/diff/11005/14002#newcode203
base/process_util_posix.cc:203: DPLOG(ERROR) << "Unable to terminate process
group " << process_group_id;
On 2010/09/21 08:55:49, Paweł Hajdan Jr. wrote:
> We should log the error in Release mode too.

Done.

http://codereview.chromium.org/3423012/diff/11005/14002#newcode457
base/process_util_posix.cc:457: if (pgid < 0) {
On 2010/09/21 08:55:49, Paweł Hajdan Jr. wrote:
> nit: No need for {}
> 
> In fact, you don't need the temporary pgid variable either.
> 
> Just if (setpgid(...) < 0) return false.

Done.

[Raghu Simha, 2010-09-22 01:36:38]

I ran this patch on the trybots with an additional test (not in this patch)
called "Die", that simply crashes by doing a DCHECK(false). The orphaned test
server process was successfully killed, and the subsequent test ran cleanly.

See the logs at
http://build.chromium.org/buildbot/try-server/builders/linux_sync/builds/101

[Paweł Hajdan Jr., 2010-09-22 09:24:54]

Yay LGTM! Thanks so much for working on this.