Rework Windows process handling.

runtime/bin/process_win.cc

Index: runtime/bin/process_win.cc
diff --git a/runtime/bin/process_win.cc b/runtime/bin/process_win.cc
index 773c17f8f437a6f6a1a4473e8088fe7f301cb2bb..de66bd9546d5c2135917032fe477dfd696663704 100644
--- a/runtime/bin/process_win.cc
+++ b/runtime/bin/process_win.cc
@@ -7,11 +7,17 @@
 #include "bin/builtin.h"
 #include "bin/process.h"
 #include "bin/eventhandler.h"
+#include "bin/thread.h"
 #include "platform/globals.h"
 
 static const int kReadHandle = 0;
 static const int kWriteHandle = 1;
 
+
+// ProcessInfo is used to map a process id to the process handle and
+// the pipe used to communicate the exit code of the process to Dart.
+// ProcessInfo objects are kept in the static singly-linked
+// ProcessInfoList.
 class ProcessInfo {
  public:
   ProcessInfo(DWORD process_id, HANDLE process_handle, HANDLE exit_pipe)
@@ -19,7 +25,18 @@ class ProcessInfo {
         process_handle_(process_handle),
         exit_pipe_(exit_pipe) { }
 
-  intptr_t pid() { return process_id_; }
+  ~ProcessInfo() {
+    BOOL success = CloseHandle(process_handle_);
+    if (!success) {
+      FATAL("Failed to close process handle");
+    }
+    success = CloseHandle(exit_pipe_);
+    if (!success) {
+      FATAL("Failed to close process exit code pipe");
+    }
+  }
+
+  DWORD pid() { return process_id_; }
   HANDLE process_handle() { return process_handle_; }
   HANDLE exit_pipe() { return exit_pipe_; }
   ProcessInfo* next() { return next_; }
@@ -33,44 +50,290 @@ class ProcessInfo {
 };
 
 
-ProcessInfo* active_processes = NULL;
+// Singly-linked list of ProcessInfo objects for all active processes
+// started from Dart.
+class ProcessInfoList {
+ public:
+  static void AddProcess(DWORD pid, HANDLE handle, HANDLE pipe) {
+    MutexLocker locker(&mutex_);
+    ProcessInfo* info = new ProcessInfo(pid, handle, pipe);
+    info->set_next(active_processes_);
+    active_processes_ = info;
+    ++number_of_processes_;
+    BOOL success = SetEvent(GetProcessAddedEvent());
+    if (!success) {
+      FATAL("Failed to set process added event");
+    }
+  }
 
+  static bool LookupProcess(DWORD pid, HANDLE* handle, HANDLE* pipe) {
+    MutexLocker locker(&mutex_);
+    ProcessInfo* current = active_processes_;
+    while (current != NULL) {
+      if (current->pid() == pid) {
+        *handle = current->process_handle();
+        *pipe = current->exit_pipe();
+        return true;
+      }
+      current = current->next();
+    }
+    return false;
+  }
 
-static void AddProcess(ProcessInfo* process) {
-  process->set_next(active_processes);
-  active_processes = process;
-}
+  static DWORD LookupProcessByHandle(HANDLE handle, DWORD* pid, HANDLE* pipe) {

[Søren Gjesse, 2012/02/02 12:26:07]

bool return type.

[Mads Ager (google), 2012/02/02 13:47:45]

Good catch. Thanks.

+    MutexLocker locker(&mutex_);
+    ProcessInfo* current = active_processes_;
+    while (current != NULL) {
+      if (current->process_handle() == handle) {
+        *pid = current->pid();
+        *pipe = current->exit_pipe();
+        return true;
+      }
+      current = current->next();
+    }
+    return false;
+  }
+
+  static void RemoveProcess(DWORD pid) {
+    MutexLocker locker(&mutex_);
+    ProcessInfo* prev = NULL;
+    ProcessInfo* current = active_processes_;
+    while (current != NULL) {
+      if (current->pid() == pid) {
+        if (prev == NULL) {
+          active_processes_ = current->next();
+        } else {
+          prev->set_next(current->next());
+        }
+        delete current;
+        --number_of_processes_;
+        return;
+      }
+      prev = current;
+      current = current->next();
+    }
+  }
 
+  static void GetHandleArray(HANDLE** handles,
+                             DWORD* number_of_handles,
+                             intptr_t prefix_size) {
+    MutexLocker locker(&mutex_);
+    ASSERT(prefix_size >= 0);
+    *number_of_handles = prefix_size + number_of_processes_;
+    *handles = new HANDLE[*number_of_handles];
+    intptr_t i = prefix_size;
+    ProcessInfo* current = active_processes_;
+    while (current != NULL) {
+      (*handles)[i++] = current->process_handle();
+      current = current->next();
+    }
+    ASSERT(i == *number_of_handles);
+    // We have taken a new snapshot of the handles in the list. Reset
+    // the process_added_event so we will get signaled if more
+    // processes are added.
+    BOOL success = ResetEvent(GetProcessAddedEvent());
+    if (!success) {
+      FATAL("Failed to reset process added event");
+    }
+  }
 
-static ProcessInfo* LookupProcess(intptr_t pid) {
-  ProcessInfo* current = active_processes;
-  while (current != NULL) {
-    if (current->pid() == pid) {
-      return current;
+ private:
+  friend class ExitCodeHandler;
+  static HANDLE GetProcessAddedEvent() {
+    MutexLocker locker(&process_added_event_mutex_);
+    if (process_added_event_ == 0) {

[Søren Gjesse, 2012/02/02 12:26:07]

INVALID_HANDLE_VALUE instead of 0.

[Mads Ager (google), 2012/02/02 13:47:45]

Done.

+      process_added_event_ = CreateEvent(NULL, TRUE, FALSE, NULL);
+      if (process_added_event_ == NULL) {

[Søren Gjesse, 2012/02/02 12:26:07]

INVALID_HANDLE_VALUE instead of NULL.

[Mads Ager (google), 2012/02/02 13:47:45]

Not on this one. Strange, I agree, but that's what the documentation says: 

http://msdn.microsoft.com/en-us/library/windows/desktop/ms682396(v=vs.85).aspx

"If the function fails, the return value is NULL"

+        FATAL("Failed to allocate event for signaling addition of processes");
+      }
     }
-    current = current->next();
+    return process_added_event_;
   }
-  return NULL;
-}
+  // Number of processes currently in the list.
+  static intptr_t number_of_processes_;
+  // Linked list of ProcessInfo objects for all active processes
+  // started from Dart code.
+  static ProcessInfo* active_processes_;
+  // Mutex protecting all accesses to the linked list of active
+  // processes.
+  static dart::Mutex mutex_;
+  // Event used to signal that more processes have been added to the
+  // list.
+  static HANDLE process_added_event_;
+  static dart::Mutex process_added_event_mutex_;
+};
+
 
+intptr_t ProcessInfoList::number_of_processes_ = 0;
+ProcessInfo* ProcessInfoList::active_processes_ = NULL;
+dart::Mutex ProcessInfoList::mutex_;
+HANDLE ProcessInfoList::process_added_event_ = 0;

[Søren Gjesse, 2012/02/02 12:26:07]

INVALID_HANDLE_VALUE instead of 0.

[Mads Ager (google), 2012/02/02 13:47:45]

Done.

+dart::Mutex ProcessInfoList::process_added_event_mutex_;
 
-static void RemoveProcess(intptr_t pid) {
-  ProcessInfo* prev = NULL;
-  ProcessInfo* current = active_processes;
-  while (current != NULL) {
-    if (current->pid() == pid) {
-      if (prev == NULL) {
-        active_processes = current->next();
+
+// The exit code handler sets up a separate thread which is waiting
+// for Dart process termination and process start. When a process
+// terminates the exit code is extracted and communicated to Dart
+// through the event loop.
+class ExitCodeHandler {
+ public:
+  // Ensure that the ExitCodeHandler has been initialized.
+  static bool EnsureInitialized() {
+    // Multiple isolates could be starting processes at the same
+    // time. Make sure that only one of them initializes the
+    // ExitCodeHandler.
+    MutexLocker locker(&mutex_);
+    if (initialized_) {
+      return true;
+    }
+
+    // Allocate an event object to be signaled when the exit code
+    // thread should terminate.
+    terminate_event_ = CreateEvent(NULL, TRUE, FALSE, NULL);
+    if (terminate_event_ == NULL) {

[Søren Gjesse, 2012/02/02 12:26:07]

INVALID_HANDLE_VALUE instead of NULL.

[Mads Ager (google), 2012/02/02 13:47:45]

Not for this one according to CreateEvent documentation.

+      return false;
+    }
+
+    // Start thread that waits for the process-addition and
+    // thread-termination events as well as all process handles for
+    // all active processes.
+    HANDLE* events = new HANDLE[2];
+    events[0] = ProcessInfoList::GetProcessAddedEvent();
+    events[1] = terminate_event_;
+    new dart::Thread(ExitCodeHandlerEntry, reinterpret_cast<uword>(events));

[Søren Gjesse, 2012/02/02 12:26:07]

int result = dart::Thread::Start(ExitCodeHandlerEntry,
reinterpret_cast<uword>(events));
if (result != 0) {
  FATAL1("Failed to start exit code handler thread %d", result);
}

 - you'll see when rebasing.

[Mads Ager (google), 2012/02/02 13:47:45]

Done.

+
+    // Thread started and the ExitCodeHandler is initialized.
+    initialized_ = true;
+    return true;
+  }
+
+  static void TerminateExitCodeThread() {

[Søren Gjesse, 2012/02/02 12:26:07]

Maybe add a comment for this function that it actually waits for the thread to
terminate.

[Mads Ager (google), 2012/02/02 13:47:45]

Yes, that comment is in the process.h file. :)

+    MutexLocker locker(&mutex_);
+    if (!initialized_) {
+      return;
+    }
+
+    BOOL success = SetEvent(terminate_event_);
+    if (!success) {
+      FATAL("Failed to set terminate event for exit code handler shutdown");
+    }
+
+    {
+      MonitorLocker terminate_locker(&thread_terminate_monitor_);
+      while (!thread_terminated_) {
+        terminate_locker.Wait();
+      }
+    }
+  }
+
+  static void ExitCodeThreadTerminated() {
+    MonitorLocker locker(&thread_terminate_monitor_);
+    thread_terminated_ = true;
+    locker.Notify();
+  }
+
+ private:
+  // Entry point for the exit code handler thread started by the
+  // ExitCodeHandler.
+  static void ExitCodeHandlerEntry(uword param) {
+    HANDLE* events = reinterpret_cast<HANDLE*>(param);
+    HANDLE wake_up_event = events[0];
+    HANDLE terminate_event = events[1];
+    delete[] events;
+
+    while (true) {
+      // Get the list of handles to wait for. Allocate a prefix of two
+      // extra handles for the 'process added' and 'thread
+      // termination' event objects.
+      HANDLE* handles;

[Søren Gjesse, 2012/02/02 12:26:07]

How about just declaring this as

HANDLE handles[MAXIMUM_WAIT_OBJECTS]

then you don't need to allocate/deallocate.

[Mads Ager (google), 2012/02/02 13:47:45]

Done. It needs to be dealt with in GetHandleArray as well so we do not get
out-of-bounds writes to the array.

+      DWORD number_of_handles;
+      intptr_t prefix_size = 2;
+      ProcessInfoList::GetHandleArray(&handles,
+                                      &number_of_handles,
+                                      prefix_size);
+      handles[0] = wake_up_event;
+      handles[1] = terminate_event;
+
+      // TODO(1450): support more than 63 processes on Windows.
+      if (number_of_handles > MAXIMUM_WAIT_OBJECTS) {
+        FATAL1("Only %d processes supported on Windows at this point\n",
+               MAXIMUM_WAIT_OBJECTS - 1);
+      }
+
+      // Wait for the handles.
+      DWORD result =
+          WaitForMultipleObjects(number_of_handles, handles, FALSE, INFINITE);
+      if (result == WAIT_FAILED) {
+        FATAL("Failed to wait for multiple objects for exit code handling");
+      }
+
+      if (result == 0) {
+        // If the result is 0 the thread woke up because of process
+        // addition. We don't have to do anything we just need to
+        // update the list of handles we are waiting for.
+      } else if (result == 1) {
+        // The termination event was triggered. Free handle array and
+        // exit.
+        delete[] handles;
+        CloseHandle(terminate_event_);
+        CloseHandle(wake_up_event);
+        ExitCodeThreadTerminated();
+        return;
       } else {
-        prev->set_next(current->next());
+        // The result is the index of the process that was
+        // signalled. Get its exit code and communicate it to Dart.
+        ASSERT(result < number_of_handles);
+        int exit_code;
+        BOOL ok = GetExitCodeProcess(handles[result],
+                                     reinterpret_cast<DWORD*>(&exit_code));
+        if (!ok) {
+          FATAL1("GetExitCodeProcess failed %d\n", GetLastError());
+        }
+        int negative = 0;
+        if (exit_code < 0) {
+          exit_code = abs(exit_code);
+          negative = 1;
+        }
+
+        DWORD pid;
+        HANDLE exit_pipe;
+        bool success = ProcessInfoList::LookupProcessByHandle(handles[result],
+                                                              &pid,
+                                                              &exit_pipe);
+        if (!success) {
+          FATAL("Failed to lookup pid and exit pipe from process handle");
+        }
+        int message[2] = { exit_code, negative };
+        DWORD written;
+        ok = WriteFile(exit_pipe, message, sizeof(message), &written, NULL);
+        // If the process has been closed, the read end of the exit
+        // pipe has been closed. It is therefore not a problem that
+        // WriteFile fails with a closed pipe error
+        // (ERROR_NO_DATA). Other errors should not happen.
+        if (ok && written != sizeof(message)) {
+          FATAL("Failed to write entire process exit message");
+        } else if (!ok && GetLastError() != ERROR_NO_DATA) {
+          FATAL1("Failed to write exit code: %d", GetLastError());
+        }
+        ProcessInfoList::RemoveProcess(pid);
       }
-      delete current;
-      return;
+      delete[] handles;
     }
-    prev = current;
-    current = current->next();
   }
-}
+
+  static dart::Mutex mutex_;
+  static bool initialized_;
+  static HANDLE terminate_event_;
+  static bool thread_terminated_;
+  static dart::Monitor thread_terminate_monitor_;
+};
+
+
+dart::Mutex ExitCodeHandler::mutex_;
+bool ExitCodeHandler::initialized_ = false;
+HANDLE ExitCodeHandler::terminate_event_ = 0;

[Søren Gjesse, 2012/02/02 12:26:07]

INVALID_HANDLE_VALUE instead of 0.

[Mads Ager (google), 2012/02/02 13:47:45]

Done.

+bool ExitCodeHandler::thread_terminated_ = false;
+dart::Monitor ExitCodeHandler::thread_terminate_monitor_;
 
 
 // Types of pipes to create.
@@ -201,31 +464,6 @@ static int SetOsErrorMessage(char* os_error_message,
 }
 
 
-static unsigned int __stdcall TerminationWaitThread(void* args) {
-  ProcessInfo* process = reinterpret_cast<ProcessInfo*>(args);
-  WaitForSingleObject(process->process_handle(), INFINITE);
-  int exit_code;
-  BOOL ok = GetExitCodeProcess(process->process_handle(),
-                               reinterpret_cast<DWORD*>(&exit_code));
-  if (!ok) {
-    fprintf(stderr, "GetExitCodeProcess failed %d\n", GetLastError());
-  }
-  int negative = 0;
-  if (exit_code < 0) {
-    exit_code = abs(exit_code);
-    negative = 1;
-  }
-  int message[3] = { process->pid(), exit_code, negative };
-  DWORD written;
-  ok = WriteFile(
-      process->exit_pipe(), message, sizeof(message), &written, NULL);
-  if (!ok || written != sizeof(message)) {
-    fprintf(stderr, "WriteFile failed %d\n", GetLastError());
-  }
-  return 0;
-}
-
-
 int Process::Start(const char* path,
                    char* arguments[],
                    intptr_t arguments_length,
@@ -237,6 +475,14 @@ int Process::Start(const char* path,
                    intptr_t* exit_handler,
                    char* os_error_message,
                    int os_error_message_len) {
+  // Ensure that the process exit handler thread has been started.
+  bool initialized = ExitCodeHandler::EnsureInitialized();
+  if (!initialized) {
+    int error_code = SetOsErrorMessage(os_error_message, os_error_message_len);
+    fprintf(stderr, "Failed to initialize ExitCodeHandler: %d\n", error_code);
+    return error_code;
+  }
+
   HANDLE stdin_handles[2] = { INVALID_HANDLE_VALUE, INVALID_HANDLE_VALUE };
   HANDLE stdout_handles[2] = { INVALID_HANDLE_VALUE, INVALID_HANDLE_VALUE };
   HANDLE stderr_handles[2] = { INVALID_HANDLE_VALUE, INVALID_HANDLE_VALUE };
@@ -361,19 +607,9 @@ int Process::Start(const char* path,
     return error_code;
   }
 
-  ProcessInfo* process = new ProcessInfo(process_info.dwProcessId,
-                                         process_info.hProcess,
-                                         exit_handles[kWriteHandle]);
-  AddProcess(process);
-
-  // TODO(sgjesse): Don't use a separate thread for waiting for each process to
-  // terminate.
-  uint32_t tid;
-  uintptr_t thread_handle =
-      _beginthreadex(NULL, 32 * 1024, TerminationWaitThread, process, 0, &tid);
-  if (thread_handle == -1) {
-    FATAL("Failed to start process termination wait thread");
-  }
+  ProcessInfoList::AddProcess(process_info.dwProcessId,
+                              process_info.hProcess,
+                              exit_handles[kWriteHandle]);
 
   // Connect the three std streams.
   FileHandle* stdin_handle = new FileHandle(stdin_handles[kWriteHandle]);
@@ -391,29 +627,25 @@ int Process::Start(const char* path,
   CloseHandle(process_info.hThread);
 
   // Return process id.
-  *id = process->pid();
+  *id = process_info.dwProcessId;
   return 0;
 }
 
 
 bool Process::Kill(intptr_t id) {
-  ProcessInfo* process = LookupProcess(id);
-  ASSERT(process != NULL);
-  if (process != NULL) {
-    BOOL result = TerminateProcess(process->process_handle(), -1);
-    if (result == 0) {
-      return false;
-    }
+  HANDLE process_handle;
+  HANDLE exit_pipe;
+  bool success =
+      ProcessInfoList::LookupProcess(id, &process_handle, &exit_pipe);
+  ASSERT(success);
+  BOOL result = TerminateProcess(process_handle, -1);
+  if (!result) {
+    return false;
   }
   return true;
 }
 
 
-void Process::Exit(intptr_t id) {
-  RemoveProcess(id);
-}
-
-
 void Process::TerminateExitCodeHandler() {
-  // TODO(ager): Implement.
+  ExitCodeHandler::TerminateExitCodeThread();
 }