GTTF: kill spawned test processes when the launcher is killed

base/test/test_launcher.cc

Index: base/test/test_launcher.cc
diff --git a/base/test/test_launcher.cc b/base/test/test_launcher.cc
index deabb9a4add1dc1b8ed692d08b5cb6466f4be403..6b90e6c0b6a81793dc08d01ac9bbc52fde5b4052 100644
--- a/base/test/test_launcher.cc
+++ b/base/test/test_launcher.cc
@@ -4,6 +4,10 @@
 
 #include "base/test/test_launcher.h"
 
+#if defined(OS_POSIX)
+#include <fcntl.h>
+#endif
+
 #include "base/at_exit.h"
 #include "base/bind.h"
 #include "base/command_line.h"
@@ -11,6 +15,7 @@
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/format_macros.h"
+#include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
@@ -44,6 +49,84 @@ const FilePath::CharType kDefaultOutputFile[] = FILE_PATH_LITERAL(
 
 namespace {
 
+// Set of live launch test processes with corresponding lock (it is allowed
+// for callers to launch processes on different threads).
+LazyInstance<std::set<ProcessHandle> > g_live_process_handles;

[sky, 2013/09/10 16:17:27]

These aren't globals. My take of the style guide is that these should not have a
g_ in front of them.

[Paweł Hajdan Jr., 2013/09/10 17:17:08]

I'm fine to make changes but would like to understand first. Is it because of
anonymous namespace or LazyInstance, or something else?

Using git grep I've found plenty of examples of using g_ with LazyInstance.

I don't think anonymous namespace would change things either - they're certainly
not local variables...

[sky, 2013/09/10 19:14:46]

My reasoning is that it's not a global. g_browser_process is a global.

[Paweł Hajdan Jr., 2013/09/10 19:56:45]

Current version of the code is consistent with usage in base. See git grep
LazyInstance base. g_ is used even though the variable is not exposed via
headers.

+LazyInstance<Lock> g_live_process_handles_lock;
+
+#if defined(OS_POSIX)
+// Self-pipe that makes it possible to do complex shutdown handling
+// outside of the signal handler.
+int g_shutdown_pipe[2] = { -1, -1 };
+
+void ShutdownPipeSignalHandler(int signal) {
+  HANDLE_EINTR(write(g_shutdown_pipe[1], "q", 1));
+}
+
+// I/O watcher for the reading end of the self-pipe above.
+// Terminates any launched child processes and exits the process.
+class SignalFDWatcher : public MessageLoopForIO::Watcher {
+ public:
+  SignalFDWatcher() {
+  }
+
+  virtual void OnFileCanReadWithoutBlocking(int fd) OVERRIDE {
+    fprintf(stdout, "\nCaught signal. Killing spawned test processes...\n");
+    fflush(stdout);
+
+    // Keep the lock until exiting the process to prevent further processes
+    // from being spawned.
+    AutoLock lock(g_live_process_handles_lock.Get());
+
+    fprintf(stdout,
+            "Sending SIGTERM to %" PRIuS " child processes... ",
+            g_live_process_handles.Get().size());
+    fflush(stdout);
+
+    for (std::set<ProcessHandle>::iterator i =
+             g_live_process_handles.Get().begin();
+         i != g_live_process_handles.Get().end();
+         ++i) {
+      kill((-1) * (*i), SIGTERM);  // Send the signal to entire process group.
+    }
+
+    fprintf(stdout,
+            "done.\nGiving processes a chance to terminate cleanly... ");
+    fflush(stdout);
+
+    PlatformThread::Sleep(TimeDelta::FromMilliseconds(500));
+
+    fprintf(stdout, "done.\n");
+    fflush(stdout);
+
+    fprintf(stdout,
+            "Sending SIGKILL to %" PRIuS " child processes... ",
+            g_live_process_handles.Get().size());
+    fflush(stdout);
+
+    for (std::set<ProcessHandle>::iterator i =
+             g_live_process_handles.Get().begin();
+         i != g_live_process_handles.Get().end();
+         ++i) {
+      kill((-1) * (*i), SIGKILL);  // Send the signal to entire process group.
+    }
+
+    fprintf(stdout, "done.\n");
+    fflush(stdout);
+
+    // The signal would normally kill the process, so exit now.
+    exit(1);
+  }
+
+  virtual void OnFileCanWriteWithoutBlocking(int fd) OVERRIDE {
+    NOTREACHED();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SignalFDWatcher);
+};
+#endif  // defined(OS_POSIX)
+
 // Parses the environment variable var as an Int32.  If it is unset, returns
 // default_val.  If it is set, unsets it then converts it to Int32 before
 // returning it.  If unsetting or converting to an Int32 fails, print an
@@ -541,8 +624,18 @@ int LaunchChildTestProcessWithOptions(const CommandLine& command_line,
 #endif
 
   base::ProcessHandle process_handle;
-  if (!base::LaunchProcess(command_line, options, &process_handle))
-    return -1;
+
+  {
+    // Note how we grab the lock before the process possibly gets created.
+    // This ensures that when the lock is held, ALL the processes are registered
+    // in the set.
+    AutoLock lock(g_live_process_handles_lock.Get());
+
+    if (!base::LaunchProcess(command_line, options, &process_handle))
+      return -1;
+
+    g_live_process_handles.Get().insert(process_handle);
+  }
 
   int exit_code = 0;
   if (!base::WaitForExitCodeWithTimeout(process_handle,
@@ -555,16 +648,25 @@ int LaunchChildTestProcessWithOptions(const CommandLine& command_line,
     base::KillProcess(process_handle, -1, true);
   }
 
+  {
+    // Note how we grab the log before issuing a possibly broad process kill.
+    // Other code parts that grab the log kill processes, so avoid trying
+    // to do that twice and trigger all kinds of log messages.
+    AutoLock lock(g_live_process_handles_lock.Get());
+
 #if defined(OS_POSIX)
-  if (exit_code != 0) {
-    // On POSIX, in case the test does not exit cleanly, either due to a crash
-    // or due to it timing out, we need to clean up any child processes that
-    // it might have created. On Windows, child processes are automatically
-    // cleaned up using JobObjects.
-    base::KillProcessGroup(process_handle);
-  }
+    if (exit_code != 0) {

[sky, 2013/09/10 16:17:27]

How come you have this inside the lock? Doesn't seem like it needs to be part of
the lock.

[Paweł Hajdan Jr., 2013/09/10 17:17:08]

This is tricky, and suggestions how to improve the comment above are welcome.

If this is not under lock, we'd possibly try to kill that process twice, and
base::KillProcessGroup spews ugly log messages in that case. Using the lock
avoids that.

Alternatively I think I could just use raw kill(...) here, but I still prefer
current solution.

[sky, 2013/09/10 19:14:46]

Under what circumstances might you try to kill the same process twice?

[Paweł Hajdan Jr., 2013/09/10 19:56:45]

Worker thread executing this code at the same time main thread executes
SignalFDWatcher::OnFileCanReadWithoutBlocking .

This has actually happened during my testing and prompted this change to the
code.

+      // On POSIX, in case the test does not exit cleanly, either due to a crash
+      // or due to it timing out, we need to clean up any child processes that
+      // it might have created. On Windows, child processes are automatically
+      // cleaned up using JobObjects.
+      base::KillProcessGroup(process_handle);
+    }
 #endif
 
+    g_live_process_handles.Get().erase(process_handle);
+  }
+
   base::CloseProcessHandle(process_handle);
 
   return exit_code;
@@ -586,6 +688,29 @@ int LaunchTests(TestLauncherDelegate* launcher_delegate,
 
   int exit_code = 0;
 
+#if defined(OS_POSIX)
+  CHECK_EQ(0, pipe(g_shutdown_pipe));
+
+  struct sigaction action;
+  memset(&action, 0, sizeof(action));
+  sigemptyset(&action.sa_mask);
+  action.sa_handler = &ShutdownPipeSignalHandler;
+
+  CHECK_EQ(0, sigaction(SIGINT, &action, NULL));
+  CHECK_EQ(0, sigaction(SIGQUIT, &action, NULL));
+  CHECK_EQ(0, sigaction(SIGTERM, &action, NULL));
+
+  MessageLoopForIO::FileDescriptorWatcher controller;
+  SignalFDWatcher watcher;
+
+  CHECK(MessageLoopForIO::current()->WatchFileDescriptor(
+            g_shutdown_pipe[0],
+            true,
+            MessageLoopForIO::WATCH_READ,
+            &controller,
+            &watcher));
+#endif  // defined(OS_POSIX)
+
   MessageLoop::current()->PostTask(
       FROM_HERE,
       Bind(&RunTestIteration,