Give localserver time to shut down

net/test/spawned_test_server/local_test_server.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/test/spawned_test_server/local_test_server.h"
 
 #include "base/command_line.h"
 #include "base/json/json_reader.h"
 #include "base/logging.h"
 #include "base/path_service.h"
@@ skipping 108 matching lines @@
 }
 
 bool LocalTestServer::Stop() {
   CleanUpWhenStoppingServer();
 
   if (!process_handle_)
     return true;
 
 #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_);
     process_handle_ = base::kNullProcessHandle;
   } else {
     VLOG(1) << "Kill failed?";
   }
 
   return ret;
 }
@@ skipping 102 matching lines @@
       break;
     default:
       NOTREACHED();
       return false;
   }
 
   return true;
 }
 
 }  // namespace net