Give localserver time to shut down

net/test/spawned_test_server/local_test_server.cc

Index: net/test/spawned_test_server/local_test_server.cc
diff --git a/net/test/spawned_test_server/local_test_server.cc b/net/test/spawned_test_server/local_test_server.cc
index 7ee12b99f69fc5032ea477c5ca8a1915547e835f..599559709381fc732733fe18f9b48fa6f077536d 100644
--- a/net/test/spawned_test_server/local_test_server.cc
+++ b/net/test/spawned_test_server/local_test_server.cc
@@ -126,14 +126,17 @@ bool LocalTestServer::Stop() {
 
 #if defined(OS_WIN)
   // This kills all the processes in the job object.
-  job_handle_.Close();
+  bool ret = TerminateJobObject(job_handle_, 0);

[Paweł Hajdan Jr., 2014/02/18 19:29:54]

Can this actually fail? If so, when and why?

How is that different from just closing the job handle with
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE?

[M-A Ruel, 2014/02/18 19:39:33]

Sigbjorn found out it is actually killing contained processes asynchronously.

[sigbjorn, 2014/02/19 09:29:13]

Our tests would have random failures due to this function returning False. We
only saw the failures on our virtual test slaves with virtual disks (thus very
slow I/O). Increasing the timeout in WaitForSingleProcess from 0 to 10 seconds
eliminated the problem.

That was using job_handle_.Close() though, I have no idea if TerminateJobObject
behaves the same way.

[Paweł Hajdan Jr., 2014/02/19 19:20:49]

Could you try this instead?

1. Close the job handle (like it was before).
2. Give the process more time to shut down (all platforms, and use the timeouts
from base/test/test_timeouts).
3. Always kill the process if wait fails.

[sigbjorn, 2014/02/20 09:25:56]

You mean like patch set 1 did? Except change the hardcoded 10 to a proper
timeout?

I have no strong opinion on how this should be implemented, and will follow
recommendations, but right now we seem to be going in circles. If you and maruel
could agree on a preferred method first, then I can upload a patch which does
that.

[Paweł Hajdan Jr., 2014/02/20 19:06:20]

Sounds reasonable.
I think my comments are pretty consistent: mostly objecting to lack of
KillProcess.

[sigbjorn, 2014/02/21 13:02:10]

Now in patch set 5

+  // If necessary, wait for the processes to terminate.
+  if (!ret)
+    ret = base::WaitForSingleProcess(process_handle_,
+                                     base::TimeDelta::FromSeconds(60));
+#else
+  bool ret = base::WaitForSingleProcess(process_handle_, base::TimeDelta());
 #endif
 
-  // First check if the process has already terminated.
-  bool ret = base::WaitForSingleProcess(process_handle_, base::TimeDelta());
-  if (!ret) {
+  if (!ret)
     ret = base::KillProcess(process_handle_, 1, true);
-  }
 
   if (ret) {
     base::CloseProcessHandle(process_handle_);