GTTF: kill spawned test processes when the launcher is killed

base/test/test_launcher.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 "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"
 #include "base/environment.h"
 #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"
 #include "base/process/kill.h"
 #include "base/process/launch.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/test_timeouts.h"
 #include "base/threading/thread_checker.h"
@@ skipping 13 matching lines @@
 const char kTestTotalShards[] = "GTEST_TOTAL_SHARDS";
 // The environment variable name for the test shard index.
 const char kTestShardIndex[] = "GTEST_SHARD_INDEX";
 
 // The default output file for XML output.
 const FilePath::CharType kDefaultOutputFile[] = FILE_PATH_LITERAL(
     "test_detail.xml");
 
 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
 // error and exit with failure.
 int32 Int32FromEnvOrDie(const char* const var, int32 default_val) {
   scoped_ptr<Environment> env(Environment::Create());
   std::string str_val;
   int32 result;
   if (!env->GetVar(var, &str_val))
     return default_val;
@@ skipping 477 matching lines @@
 int LaunchChildTestProcessWithOptions(const CommandLine& command_line,
                                       const LaunchOptions& options,
                                       base::TimeDelta timeout,
                                       bool* was_timeout) {
 #if defined(OS_POSIX)
   // Make sure an option we rely on is present - see LaunchChildGTestProcess.
   DCHECK(options.new_process_group);
 #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,
                                         &exit_code,
                                         timeout)) {
     *was_timeout = true;
     exit_code = -1;  // Set a non-zero exit code to signal a failure.
 
     // Ensure that the process terminates.
     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);
   }
-#endif
 
   base::CloseProcessHandle(process_handle);
 
   return exit_code;
 }
 
 int LaunchTests(TestLauncherDelegate* launcher_delegate,
                 int argc,
                 char** argv) {
   const CommandLine* command_line = CommandLine::ForCurrentProcess();
 
 
   int32 total_shards;
   int32 shard_index;
   InitSharding(&total_shards, &shard_index);
 
   int cycles = 1;
   if (command_line->HasSwitch(kGTestRepeatFlag))
     StringToInt(command_line->GetSwitchValueASCII(kGTestRepeatFlag), &cycles);
 
   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,
            launcher_delegate,
            total_shards,
            shard_index,
            cycles,
            &exit_code,
            true));
 
   MessageLoop::current()->Run();
 
   return exit_code;
 }
 
 }  // namespace base