[glib/wip/gmain: 1/8] gmain: Move child watch waitpid() into helper thread

[Colin Walters <walters src gnome org>]

commit 3976f5ef2ca34b0546a2bf5c63fd81c5ff50dc77
Author: Colin Walters <walters verbum org>
Date:   Mon Jun 6 18:46:05 2011 -0400

    gmain: Move child watch waitpid() into helper thread
    
    This further lines up the SIGCHLD handling with the other UNIX
    code.  We keep track of all the child watch sources, and
    call waitpid() in the helper thread, rather than calling it
    repeatedly in each context.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=652072

 glib/gmain.c |  109 +++++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 78 insertions(+), 31 deletions(-)
---
diff --git a/glib/gmain.c b/glib/gmain.c
index 621215c..11a2494 100644
--- a/glib/gmain.c
+++ b/glib/gmain.c
@@ -373,6 +373,7 @@ static gboolean g_child_watch_check    (GSource     *source);
 static gboolean g_child_watch_dispatch (GSource     *source,
 					GSourceFunc  callback,
 					gpointer     user_data);
+static void g_child_watch_finalize (GSource    *source);
 #ifdef G_OS_UNIX
 static gpointer unix_signal_helper_thread (gpointer data) G_GNUC_NORETURN;
 static void g_unix_signal_handler (int signum);
@@ -426,6 +427,7 @@ typedef struct {
 static UnixSignalState unix_signal_state;
 static gint unix_signal_wake_up_pipe[2];
 GSList *unix_signal_watches;
+GSList *unix_child_watches;
 
 /* Not guarded ( FIXME should it be? ) */
 static gint child_watch_count = 1;
@@ -454,7 +456,7 @@ GSourceFuncs g_child_watch_funcs =
   g_child_watch_prepare,
   g_child_watch_check,
   g_child_watch_dispatch,
-  NULL
+  g_child_watch_finalize
 };
 
 GSourceFuncs g_idle_funcs =
@@ -4262,33 +4264,46 @@ g_child_watch_check (GSource  *source)
 
 #else /* G_OS_WIN32 */
 
+static void
+unix_check_for_child_exited_unlocked (GChildWatchSource *source)
+{
+  gint child_status;
+  
+  if (waitpid (source->pid, &child_status, WNOHANG) > 0)
+    {
+      source->child_status = child_status;
+      source->child_exited = TRUE;
+    }
+}
+
 static gboolean
 check_for_child_exited (GSource *source)
 {
   GChildWatchSource *child_watch_source;
   gint count;
+  gboolean ret;
 
   /* protect against another SIGCHLD in the middle of this call */
   count = child_watch_count;
 
   child_watch_source = (GChildWatchSource *) source;
 
-  if (child_watch_source->child_exited)
-    return TRUE;
+  G_LOCK (unix_signal_lock);
 
-  if (child_watch_source->count < count)
+  /* If we have threads, the helper will call waitpid() for us */
+  if (!child_watch_source->child_exited
+      && unix_signal_init_state == UNIX_SIGNAL_INITIALIZED_SINGLE
+      && child_watch_source->count < count)
     {
-      gint child_status;
-
-      if (waitpid (child_watch_source->pid, &child_status, WNOHANG) > 0)
-	{
-	  child_watch_source->child_status = child_status;
-	  child_watch_source->child_exited = TRUE;
-	}
+      unix_check_for_child_exited_unlocked (child_watch_source);
       child_watch_source->count = count;
     }
 
-  return child_watch_source->child_exited;
+  ret = child_watch_source->child_exited;
+  
+  G_UNLOCK (unix_signal_lock);
+  
+  return ret;
 }
 
 static gboolean
@@ -4491,6 +4506,16 @@ g_unix_signal_watch_finalize (GSource    *source)
 
 #endif /* G_OS_WIN32 */
 
+static void 
+g_child_watch_finalize (GSource    *source)
+{
+#ifdef G_OS_UNIX
+  G_LOCK (unix_signal_lock);
+  unix_child_watches = g_slist_remove (unix_child_watches, source);
+  G_UNLOCK (unix_signal_lock);
+#endif
+}
+
 static gboolean
 g_child_watch_dispatch (GSource    *source, 
 			GSourceFunc callback,
@@ -4543,7 +4568,9 @@ g_unix_signal_handler (int signum)
 	  /* Shouldn't happen */
 	  return;
 	}
-      write (unix_signal_wake_up_pipe[1], buf, 1);
+      while (write (unix_signal_wake_up_pipe[1], buf, 1) == -1
+	     && errno == EINTR)
+	;
     }
   else
     {
@@ -4588,6 +4615,25 @@ deliver_unix_signal (int signum)
   G_UNLOCK (unix_signal_lock);
 }
 
+/* Should only be called from the UNIX signal helper thread */
+static void
+deliver_sigchld (void)
+{
+  GSList *iter;
+
+  G_LOCK (unix_signal_lock);
+  for (iter = unix_child_watches; iter; iter = iter->next)
+    {
+      GChildWatchSource *source = iter->data;
+      
+      if (source->child_exited)
+	continue;
+
+      unix_check_for_child_exited_unlocked (source);
+    }
+  G_UNLOCK (unix_signal_lock);
+}
+
 /*
  * This thread is created whenever anything in GLib needs
  * to deal with UNIX signals; at present, just SIGCHLD
@@ -4603,6 +4649,7 @@ unix_signal_helper_thread (gpointer data)
     {
       gchar b[128];
       ssize_t i, bytes_read;
+      gboolean sigchld_received = FALSE;
       gboolean sigterm_received = FALSE;
       gboolean sigint_received = FALSE;
       gboolean sighup_received = FALSE;
@@ -4623,14 +4670,12 @@ unix_signal_helper_thread (gpointer data)
 	  switch (b[i])
 	    {
 	    case _UNIX_SIGNAL_PIPE_SIGCHLD_CHAR:
-	      /* The child watch source will call waitpid() in its
-	       * prepare() and check() methods; however, we don't
-	       * know which pid exited, so we need to wake up
-	       * all contexts.  Note: actually we could get the pid
-	       * from the "siginfo_t" via the handler, but to pass
-	       * that info down the pipe would require a more structured
-	       * data stream (as opposed to a single byte).
+	      /* Note: actually we could get the pid from the
+	       * "siginfo_t" via the handler, but to pass that info
+	       * down the pipe would require a more structured data
+	       * stream (as opposed to a single byte).
 	       */
+	      sigchld_received = TRUE;
 	      break;
 	    case _UNIX_SIGNAL_PIPE_SIGTERM_CHAR:
 	      sigterm_received = TRUE;
@@ -4651,19 +4696,13 @@ unix_signal_helper_thread (gpointer data)
 	    deliver_unix_signal (SIGINT);
 	  if (sighup_received)
 	    deliver_unix_signal (SIGHUP);
+	  if (sigchld_received)
+	    deliver_sigchld ();
 	  _g_main_wake_up_all_contexts ();
 	}
     }
 }
 
-static void
-g_child_watch_source_init (void)
-{
-  G_LOCK (unix_signal_lock);
-  ensure_unix_signal_handler_installed_unlocked (SIGCHLD);
-  G_UNLOCK (unix_signal_lock);
-}
-
 #endif /* !G_OS_WIN32 */
 
 /**
@@ -4701,17 +4740,25 @@ g_child_watch_source_new (GPid pid)
   GSource *source = g_source_new (&g_child_watch_funcs, sizeof (GChildWatchSource));
   GChildWatchSource *child_watch_source = (GChildWatchSource *)source;
 
+  child_watch_source->pid = pid;
+
 #ifdef G_OS_WIN32
   child_watch_source->poll.fd = (gintptr) pid;
   child_watch_source->poll.events = G_IO_IN;
 
   g_source_add_poll (source, &child_watch_source->poll);
 #else /* G_OS_WIN32 */
-  g_child_watch_source_init ();
+  G_LOCK (unix_signal_lock);
+  ensure_unix_signal_handler_installed_unlocked (SIGCHLD);
+  unix_child_watches = g_slist_prepend (unix_child_watches, source);
+  /* In order to avoid a race condition where SIGCHLD may have been
+   * delivered before we create this source, we check to see if the
+   * child has already exited.
+   */
+  unix_check_for_child_exited_unlocked (child_watch_source);
+  G_UNLOCK (unix_signal_lock);
 #endif /* G_OS_WIN32 */
 
-  child_watch_source->pid = pid;
-
   return source;
 }