Simplify process exit-code handling on Posix, take 2.

runtime/bin/process_linux.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "platform/globals.h"
 #if defined(TARGET_OS_LINUX)
 
 #include "bin/process.h"
 
 #include <errno.h>  // NOLINT
 #include <fcntl.h>  // NOLINT
 #include <poll.h>  // NOLINT
-#include <signal.h>  // NOLINT
 #include <stdio.h>  // NOLINT
 #include <stdlib.h>  // NOLINT
 #include <string.h>  // NOLINT
 #include <sys/wait.h>  // NOLINT
 #include <unistd.h>  // NOLINT
 
 #include "bin/fdutils.h"
 #include "bin/log.h"
 #include "bin/thread.h"
 
@@ skipping 80 matching lines @@
   // Mutex protecting all accesses to the linked list of active
   // processes.
   static dart::Mutex* mutex_;
 };
 
 
 ProcessInfo* ProcessInfoList::active_processes_ = NULL;
 dart::Mutex* ProcessInfoList::mutex_ = new dart::Mutex();
 
 
-// The exit code handler sets up a separate thread which is signalled
-// on SIGCHLD. That separate thread can then get the exit code from
+// The exit code handler sets up a separate thread which waits for child
+// processes to die. That separate thread can then get the exit code from

[Søren Gjesse, 2013/09/11 08:23:48]

s/die/terminate

[Anders Johnsen, 2013/09/11 11:54:52]

Done.

 // processes that have exited and communicate it to Dart through the
 // event loop.
 class ExitCodeHandler {
  public:
   // Ensure that the ExitCodeHandler has been initialized.
   static bool EnsureInitialized() {

[Søren Gjesse, 2013/09/11 08:23:48]

No need for a return value as it always returns true.

[Anders Johnsen, 2013/09/11 11:54:52]

Done.

     // Multiple isolates could be starting processes at the same
-    // time. Make sure that only one of them initializes the
-    // ExitCodeHandler.
+    // time. Make sure that only one ExitCodeHandler thread exists.
     MutexLocker locker(mutex_);
-    if (initialized_) {
+    if (running_) {
       return true;
     }
 
-    // Allocate a pipe that the signal handler can write a byte to and
-    // that the exit handler thread can poll.
-    int result = TEMP_FAILURE_RETRY(pipe(sig_chld_fds_));
-    if (result < 0) {
-      return false;
-    }
-    FDUtils::SetCloseOnExec(sig_chld_fds_[0]);
-    FDUtils::SetCloseOnExec(sig_chld_fds_[1]);
-
-    // Start thread that polls the pipe and handles process exits when
-    // data is received on the pipe.
-    result = dart::Thread::Start(ExitCodeHandlerEntry, sig_chld_fds_[0]);
+    // Start thread that handles process exits when waitpid returns.

[Søren Gjesse, 2013/09/11 08:23:48]

s/waitpid/wait

[Anders Johnsen, 2013/09/11 11:54:52]

Done.

+    int result = dart::Thread::Start(ExitCodeHandlerEntry, 0);
     if (result != 0) {
       FATAL1("Failed to start exit code handler worker thread %d", result);
     }
 
-    // Mark write end non-blocking.
-    FDUtils::SetNonBlocking(sig_chld_fds_[1]);
-
-    // Thread started and the ExitCodeHandler is initialized.
-    initialized_ = true;
+    // Thread started and the ExitCodeHandler is running.
+    running_ = true;
     return true;
   }
 
-  // Get the write end of the pipe.
-  static int WakeUpFd() {
-    return sig_chld_fds_[1];
-  }
-
   static void TerminateExitCodeThread() {
     MutexLocker locker(mutex_);
-    if (!initialized_) {
+    if (!running_) {
       return;
     }
 
-    uint8_t data = kThreadTerminateByte;
-    ssize_t result =
-        TEMP_FAILURE_RETRY(write(ExitCodeHandler::WakeUpFd(), &data, 1));
-    if (result < 1) {
-      perror("Failed to write to wake-up fd to terminate exit code thread");
+    thread_terminate_monitor_->Enter();
+
+    // Set terminate_done_ to false, so we can use it as a guard for our
+    // monitor (note that this is done before setting running_ to false).
+    terminate_done_ = false;
+    running_ = false;
+
+    // Fork to wake up waitpid.
+    if (TEMP_FAILURE_RETRY(fork()) == 0) {
+      exit(0);
     }
 
-    {
-      MonitorLocker terminate_locker(thread_terminate_monitor_);
-      while (!thread_terminated_) {
-        terminate_locker.Wait();
-      }
+    while (!terminate_done_) {
+      thread_terminate_monitor_->Wait(dart::Monitor::kNoTimeout);
     }
-  }
-
-  static void ExitCodeThreadTerminated() {
-    MonitorLocker locker(thread_terminate_monitor_);
-    thread_terminated_ = true;
-    locker.Notify();
+    thread_terminate_monitor_->Exit();
   }
 
  private:
-  static const uint8_t kThreadTerminateByte = 1;
-
-  // GetProcessExitCodes is called on a separate thread when a SIGCHLD
-  // signal is received to retrieve the exit codes and post them to
-  // dart.
-  static void GetProcessExitCodes() {
-    pid_t pid = 0;
-    int status = 0;
-    while ((pid = TEMP_FAILURE_RETRY(waitpid(-1, &status, WNOHANG))) > 0) {
-      int exit_code = 0;
-      int negative = 0;
-      if (WIFEXITED(status)) {
-        exit_code = WEXITSTATUS(status);
-      }
-      if (WIFSIGNALED(status)) {
-        exit_code = WTERMSIG(status);
-        negative = 1;
-      }
-      intptr_t exit_code_fd = ProcessInfoList::LookupProcessExitFd(pid);
-      if (exit_code_fd != 0) {
-        int message[2] = { exit_code, negative };
-        ssize_t result =
-            FDUtils::WriteToBlocking(exit_code_fd, &message, sizeof(message));
-        // If the process has been closed, the read end of the exit
-        // pipe has been closed. It is therefore not a problem that
-        // write fails with a broken pipe error. Other errors should
-        // not happen.
-        if (result != -1 && result != sizeof(message)) {
-          FATAL("Failed to write entire process exit message");
-        } else if (result == -1 && errno != EPIPE) {
-          FATAL1("Failed to write exit code: %d", errno);
-        }
-        ProcessInfoList::RemoveProcess(pid);
-      }
-    }
-  }
-
-
   // Entry point for the separate exit code handler thread started by
   // the ExitCodeHandler.
   static void ExitCodeHandlerEntry(uword param) {
-    struct pollfd pollfds;
-    pollfds.fd = param;
-    pollfds.events = POLLIN;
-    while (true) {
-      int result = TEMP_FAILURE_RETRY(poll(&pollfds, 1, -1));
-      if (result == -1) {
-        ASSERT(EAGAIN == EWOULDBLOCK);
-        if (errno != EWOULDBLOCK) {
-          perror("ExitCodeHandler poll failed");
+    pid_t pid = 0;
+    int status = 0;
+    while (running_) {

[Søren Gjesse, 2013/09/11 08:23:48]

We should not check running_ without holding the lock protecting it.

[Anders Johnsen, 2013/09/11 11:54:52]

Done.

+      if ((pid = TEMP_FAILURE_RETRY(wait(&status))) > 0) {
+        int exit_code = 0;
+        int negative = 0;
+        if (WIFEXITED(status)) {
+          exit_code = WEXITSTATUS(status);
         }
-      } else {
-        // Read the byte from the wake-up fd.
-        ASSERT(result = 1);
-        intptr_t data = 0;
-        ssize_t read_bytes = FDUtils::ReadFromBlocking(pollfds.fd, &data, 1);
-        if (read_bytes < 1) {
-          perror("Failed to read from wake-up fd in exit-code handler");
+        if (WIFSIGNALED(status)) {
+          exit_code = WTERMSIG(status);
+          negative = 1;
         }
-        if (data == ExitCodeHandler::kThreadTerminateByte) {
-          ExitCodeThreadTerminated();
+        intptr_t exit_code_fd = ProcessInfoList::LookupProcessExitFd(pid);
+        if (exit_code_fd != 0) {
+          int message[2] = { exit_code, negative };
+          ssize_t result =
+              FDUtils::WriteToBlocking(exit_code_fd, &message, sizeof(message));
+          // If the process has been closed, the read end of the exit
+          // pipe has been closed. It is therefore not a problem that
+          // write fails with a broken pipe error. Other errors should
+          // not happen.
+          if (result != -1 && result != sizeof(message)) {
+            FATAL("Failed to write entire process exit message");
+          } else if (result == -1 && errno != EPIPE) {
+            FATAL1("Failed to write exit code: %d", errno);
+          }
+          ProcessInfoList::RemoveProcess(pid);
+        }
+      } else if (errno == ECHILD) {
+        if (mutex_->TryLock()) {
+          // We got the lock. This means that we are not in the process of
+          // shutting down (and not will since we have the lock). We can thus do
+          // a quick exit by setting running_ to false, unlock and return (exit
+          // the thread).
+          running_ = false;
+          mutex_->Unlock();
           return;
         }
-        // Get the exit code from all processes that have died.
-        GetProcessExitCodes();
+        // If we didn't get the lock, we are trying to terminate. Continue here
+        // as that will ensure we reach our monitor notify.
       }
     }
+    thread_terminate_monitor_->Enter();
+    terminate_done_ = true;
+    thread_terminate_monitor_->Notify();
+    thread_terminate_monitor_->Exit();
   }
 
   static dart::Mutex* mutex_;

[Søren Gjesse, 2013/09/11 08:23:48]

Can't we just get away with one monitor protecting both running_ and terminate_?

  while (true) {
    pid = TEMP_FAILURE_RETRY(wait(&status))
    monitor_->Enter();
    if (terminate_) {
      running_ = false;
      monitor->Notify();
      monitor_->Exit();
      return;
    }

    if (pid == -1) {
      running_ = false;
      monitor_->Exit();
      return;
    }

    // actual work.
    monitor_->Exit();
  }

[Anders Johnsen, 2013/09/11 11:54:52]

Rewrote to only have one monitor in total.

-  static bool initialized_;
-  static int sig_chld_fds_[2];
-  static bool thread_terminated_;
+  static bool running_;
+  static bool terminate_done_;
   static dart::Monitor* thread_terminate_monitor_;
 };
 
 
 dart::Mutex* ExitCodeHandler::mutex_ = new dart::Mutex();
-bool ExitCodeHandler::initialized_ = false;
-int ExitCodeHandler::sig_chld_fds_[2] = { 0, 0 };
-bool ExitCodeHandler::thread_terminated_ = false;
+bool ExitCodeHandler::running_ = false;
+bool ExitCodeHandler::terminate_done_ = false;
 dart::Monitor* ExitCodeHandler::thread_terminate_monitor_ = new dart::Monitor();
 
 
 static void SetChildOsErrorMessage(char** os_error_message) {
   const int kBufferSize = 1024;
   char error_buf[kBufferSize];
   *os_error_message = strdup(strerror_r(errno, error_buf, kBufferSize));
 }
 
 
-static void SigChldHandler(int process_signal, siginfo_t* siginfo, void* tmp) {
-  // Save errno so it can be restored at the end.
-  int entry_errno = errno;
-  // Signal the exit code handler where the actual processing takes
-  // place.
-  ssize_t result =
-      TEMP_FAILURE_RETRY(write(ExitCodeHandler::WakeUpFd(), "", 1));
-  if (result < 1) {
-    perror("Failed to write to wake-up fd in SIGCHLD handler");
-  }
-  // Restore errno.
-  errno = entry_errno;
-}
-
-
 static void ReportChildError(int exec_control_fd) {
   // In the case of failure in the child process write the errno and
   // the OS error message to the exec control pipe and exit.
   int child_errno = errno;
   const int kBufferSize = 1024;
   char error_buf[kBufferSize];
   char* os_error_message = strerror_r(errno, error_buf, kBufferSize);
   ASSERT(sizeof(child_errno) == sizeof(errno));
   int bytes_written =
       FDUtils::WriteToBlocking(
@@ skipping 19 matching lines @@
                    intptr_t* id,
                    intptr_t* exit_event,
                    char** os_error_message) {
   pid_t pid;
   int read_in[2];  // Pipe for stdout to child process.
   int read_err[2];  // Pipe for stderr to child process.
   int write_out[2];  // Pipe for stdin to child process.
   int exec_control[2];  // Pipe to get the result from exec.
   int result;
 
-  bool initialized = ExitCodeHandler::EnsureInitialized();
-  if (!initialized) {
-    SetChildOsErrorMessage(os_error_message);
-    Log::PrintErr("Error initializing exit code handler: %s\n",
-                  *os_error_message);
-    return errno;
-  }
-
   result = TEMP_FAILURE_RETRY(pipe(read_in));
   if (result < 0) {
     SetChildOsErrorMessage(os_error_message);
     Log::PrintErr("Error pipe creation failed: %s\n", *os_error_message);
     return errno;
   }
   FDUtils::SetCloseOnExec(read_in[0]);
 
   result = TEMP_FAILURE_RETRY(pipe(read_err));
   if (result < 0) {
@@ skipping 55 matching lines @@
 
   char** program_environment = NULL;
   if (environment != NULL) {
     program_environment = new char*[environment_length + 1];
     for (int i = 0; i < environment_length; i++) {
       program_environment[i] = environment[i];
     }
     program_environment[environment_length] = NULL;
   }
 
-  struct sigaction act;
-  bzero(&act, sizeof(act));
-  act.sa_sigaction = SigChldHandler;
-  act.sa_flags = SA_NOCLDSTOP | SA_SIGINFO;
-  if (sigaction(SIGCHLD, &act, 0) != 0) {
-    perror("Process start: setting signal handler failed");
-  }
   pid = TEMP_FAILURE_RETRY(fork());
   if (pid < 0) {
     SetChildOsErrorMessage(os_error_message);
     delete[] program_arguments;
     TEMP_FAILURE_RETRY(close(read_in[0]));
     TEMP_FAILURE_RETRY(close(read_in[1]));
     TEMP_FAILURE_RETRY(close(read_err[0]));
     TEMP_FAILURE_RETRY(close(read_err[1]));
     TEMP_FAILURE_RETRY(close(write_out[0]));
     TEMP_FAILURE_RETRY(close(write_out[1]));
@@ skipping 123 matching lines @@
   FDUtils::SetNonBlocking(read_in[0]);
   *in = read_in[0];
   TEMP_FAILURE_RETRY(close(read_in[1]));
   FDUtils::SetNonBlocking(write_out[1]);
   *out = write_out[1];
   TEMP_FAILURE_RETRY(close(write_out[0]));
   FDUtils::SetNonBlocking(read_err[0]);
   *err = read_err[0];
   TEMP_FAILURE_RETRY(close(read_err[1]));
 
+  // Be sure to listen for exit-codes, now we have a child-process.
+  bool initialized = ExitCodeHandler::EnsureInitialized();
+  if (!initialized) {
+    SetChildOsErrorMessage(os_error_message);
+    Log::PrintErr("Error initializing exit code handler: %s\n",
+                  *os_error_message);
+    return errno;
+  }
+
   *id = pid;
   return 0;
 }
 
 
 class BufferList: public BufferListBase {
  public:
   bool Read(int fd, intptr_t available) {
     // Read all available bytes.
     while (available > 0) {
@@ skipping 120 matching lines @@
 
 
 intptr_t Process::CurrentProcessId() {
   return static_cast<intptr_t>(getpid());
 }
 
 }  // namespace bin
 }  // namespace dart
 
 #endif  // defined(TARGET_OS_LINUX)