Index: glib/gmain.c
===================================================================
RCS file: /cvs/gnome/glib/glib/gmain.c,v
retrieving revision 1.113
diff -u -r1.113 gmain.c
--- glib/gmain.c	4 Mar 2004 10:12:55 -0000	1.113
+++ glib/gmain.c	11 Mar 2004 10:08:52 -0000
@@ -168,12 +168,10 @@
   GSource     source;
   GPid        pid;
   gint        child_status;
-#ifdef G_OS_WIN32
-  GPollFD     poll;
-#else /* G_OS_WIN32 */
-  gint        count;
+#ifndef G_OS_WIN32
   gboolean    child_exited;
 #endif /* G_OS_WIN32 */
+  GPollFD     poll;
 };
 
 struct _GPollRec
@@ -230,12 +228,14 @@
 static gboolean g_timeout_dispatch (GSource     *source,
 				    GSourceFunc  callback,
 				    gpointer     user_data);
-static gboolean g_child_watch_prepare  (GSource     *source,
-				        gint        *timeout);
-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_append_pid (GPid pid);
+static void     g_child_watch_remove_pid (GPid pid);
+static gboolean g_child_watch_prepare    (GSource     *source,
+					  gint        *timeout);
+static gboolean g_child_watch_check      (GSource     *source);
+static gboolean g_child_watch_dispatch   (GSource     *source,
+					  GSourceFunc  callback,
+					  gpointer     user_data);
 static gboolean g_idle_prepare     (GSource     *source,
 				    gint        *timeout);
 static gboolean g_idle_check       (GSource     *source);
@@ -249,14 +249,19 @@
 
 #ifndef G_OS_WIN32
 /* Child status monitoring code */
-enum {
-  CHILD_WATCH_UNINITIALIZED,
-  CHILD_WATCH_INITIALIZED_SINGLE,
-  CHILD_WATCH_INITIALIZED_THREADED
+static gint child_watch_wake_up_pipe[2];
+
+typedef struct _GChildWatchData GChildWatchData;
+struct _GChildWatchData
+{
+  GPid pid; 
+  gint status; 
 };
-static gint child_watch_init_state = CHILD_WATCH_UNINITIALIZED;
-static gint child_watch_count = 0;
-static gint child_watch_wake_up_pipe[2] = {0, 0};
+G_LOCK_DEFINE_STATIC (child_watch_ready);
+static GSList *child_watch_ready = NULL;
+G_LOCK_DEFINE_STATIC (child_watch_pids);
+static GSList *child_watch_pids = NULL;
+
 #endif /* !G_OS_WIN32 */
 G_LOCK_DEFINE_STATIC (main_context_list);
 static GSList *main_context_list = NULL;
@@ -715,7 +720,15 @@
       curr = curr->next;
     }
   g_slist_free (main_contexts_without_pipe);
-  main_contexts_without_pipe = NULL;  
+  main_contexts_without_pipe = NULL;
+  
+#ifndef G_OS_WIN32
+  /* Enforce a stable start of the child_watch_pids list wrt memory
+   * bariers. The first element of the list can now be read by all
+   * threads of the process */
+  if (child_watch_pids == NULL)
+    g_child_watch_append_pid (-1);
+#endif /* G_OS_WIN32 */
 }
 #endif /* G_THREADS_ENABLED */
 
@@ -3409,55 +3422,75 @@
 #else /* G_OS_WIN32 */
 
 static gboolean
-check_for_child_exited (GSource *source)
-{
-  GChildWatchSource *child_watch_source;
-  gint count;
-
-  /* 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;
-
-  if (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;
-	}
-      child_watch_source->count = count;
-    }
-
-  return child_watch_source->child_exited;
-}
-
-static gboolean
 g_child_watch_prepare (GSource *source,
 		       gint    *timeout)
 {
-  GChildWatchSource *child_watch_source;
   *timeout = -1;
-
-  child_watch_source = (GChildWatchSource *) source;
-
-  return check_for_child_exited (source);
+  return g_child_watch_check (source);
 }
 
 
 static gboolean 
-g_child_watch_check (GSource  *source)
+g_child_watch_check (GSource *source)
 {
-  GChildWatchSource *child_watch_source;
+  GChildWatchSource *child_watch_source = (GChildWatchSource *) source;
+  GSList *curr;
 
-  child_watch_source = (GChildWatchSource *) source;
-
-  return check_for_child_exited (source);
+  if (child_watch_source->poll.revents & G_IO_IN)
+    {
+      gssize bytes; 
+      GChildWatchData *data = g_new0 (GChildWatchData, 1);
+      do
+	{
+	  bytes = read (child_watch_wake_up_pipe[0], data, 
+			sizeof (GChildWatchData));
+	}
+      while (bytes == -1 && errno == EINTR);
+      
+      if (bytes == sizeof (GChildWatchData))
+	{
+	  G_LOCK (child_watch_ready);
+	  child_watch_ready = g_slist_prepend (child_watch_ready, data);
+	  G_UNLOCK (child_watch_ready);
+	  
+	  /* We were woken up.  Wake up all other contexts in all
+	   * other threads */
+	  G_LOCK (main_context_list);
+	  for (curr = main_context_list; curr; curr = curr->next)
+	    {
+	      GMainContext *context = curr->data;
+	      
+	      if (g_atomic_int_get (&context->ref_count) > 0)
+		/* Due to racing conditions we can find ref_count ==
+		 * 0, in that case, however, the context is still not
+		 * destroyed and no poll can be active, otherwise the
+		 * ref_count wouldn't be 0 */
+		g_main_context_wakeup (context);
+	    }
+	  G_UNLOCK (main_context_list);
+	}
+      else
+	g_assert (bytes == -1 && errno == EAGAIN);
+    }
+  
+  
+  G_LOCK (child_watch_ready);
+  for (curr = child_watch_ready; curr; curr = curr->next)
+    {
+      GChildWatchData *data = curr->data;
+      if (data->pid == child_watch_source->pid)
+	{
+	  child_watch_ready = g_slist_remove (child_watch_ready, data);
+	  child_watch_source->child_status = data->status;
+	  child_watch_source->child_exited = TRUE;
+	  g_free (data);
+	  break;
+	}	
+    }
+  
+  G_UNLOCK (child_watch_ready);
+  
+  return child_watch_source->child_exited;
 }
 
 #endif /* G_OS_WIN32 */
@@ -3467,11 +3500,9 @@
 			GSourceFunc callback,
 			gpointer    user_data)
 {
-  GChildWatchSource *child_watch_source;
+  GChildWatchSource *child_watch_source = (GChildWatchSource *) source;
   GChildWatchFunc child_watch_callback = (GChildWatchFunc) callback;
 
-  child_watch_source = (GChildWatchSource *) source;
-
   if (!callback)
     {
       g_warning ("Child watch source dispatched without callback\n"
@@ -3479,7 +3510,10 @@
       return FALSE;
     }
 
-  (child_watch_callback) (child_watch_source->pid, child_watch_source->child_status, user_data);
+  g_child_watch_remove_pid (child_watch_source->pid);
+
+  (child_watch_callback) (child_watch_source->pid, 
+			  child_watch_source->child_status, user_data);
 
   /* We never keep a child watch source around as the child is gone */
   return FALSE;
@@ -3490,113 +3524,128 @@
 static void
 g_child_watch_signal_handler (int signum)
 {
-  child_watch_count ++;
+  GChildWatchData data;
+  GSList *curr = child_watch_pids;
 
-  if (child_watch_init_state == CHILD_WATCH_INITIALIZED_THREADED)
-    {
-      write (child_watch_wake_up_pipe[1], "B", 1);
-    }
-  else
+  /* No locking is needed here for reading the child_watch_pids list,
+   * because:
+   * - The only change to the list child_watch_pids is to append an
+   *   element and to change the value of the pid.
+   * - child_watch_pids always points to a non-NULL element, which has
+   *   been constructed before a new thread has been started, thus
+   *   child_watch_pids points to valid memory
+   * - It is pretty safe to assume, that waitpid acts as a memory
+   *   barrier. Thus next is really pointing to valid memory.
+   * - Thus curr always points to valid memory.
+   * - curr->pid is always -1 or a valid pid. We might encounter a pid
+   *   twice or a stale pid, but thats not a problem for calls to
+   *   waitpid.
+   * - waitpid is save to be called inside a signal handler.
+   */
+  while (curr)
     {
-      /* We count on the signal interrupting the poll in the same thread.
-       */
-    }
-}
- 
-static void
-g_child_watch_source_init_single (void)
-{
-  g_assert (! g_thread_supported());
-  g_assert (child_watch_init_state == CHILD_WATCH_UNINITIALIZED);
+      GSList *next = curr->next;
+      data.pid = GPOINTER_TO_INT (curr->data);
 
-  child_watch_init_state = CHILD_WATCH_INITIALIZED_SINGLE;
+      if (data.pid == -1)
+	return;
 
-  signal (SIGCHLD, g_child_watch_signal_handler);
+      if (waitpid (data.pid, &data.status, WNOHANG) > 0)
+	{
+	  write (child_watch_wake_up_pipe[1], &data, sizeof (GChildWatchData));
+	  return;
+	}
+      curr = next;
+    }
 }
 
-static gpointer
-child_watch_helper_thread (gpointer data)
+static void
+g_child_watch_append_pid (GPid pid)
 {
-  GPollFD fds;
-  GPollFunc poll_func;
-
-#ifdef HAVE_POLL
-      poll_func = (GPollFunc)poll;
-#else
-      poll_func = g_poll;
-#endif
+  GSList *curr, *new_pid;
 
-  fds.fd = child_watch_wake_up_pipe[0];
-  fds.events = G_IO_IN;
-
-  while (1)
+  G_LOCK (child_watch_pids);
+  if (child_watch_pids == NULL)
     {
-      gchar b[20];
-      GSList *list;
-
-      read (child_watch_wake_up_pipe[0], b, 20);
+      child_watch_pids = g_slist_prepend (NULL, GINT_TO_POINTER (pid));
+      G_UNLOCK (child_watch_pids);
+      return;
+    }
 
-      /* We were woken up.  Wake up all other contexts in all other threads */
-      G_LOCK (main_context_list);
-      for (list = main_context_list; list; list = list->next)
+  for (curr = child_watch_pids; curr; curr = curr->next)
+    {
+      if (GPOINTER_TO_INT (curr->data) == -1)
 	{
-	  GMainContext *context;
-
-	  context = list->data;
-	  if (g_atomic_int_get (&context->ref_count) > 0)
-	    /* Due to racing conditions we can find ref_count == 0, in
-	     * that case, however, the context is still not destroyed
-	     * and no poll can be active, otherwise the ref_count
-	     * wouldn't be 0 */
-	    g_main_context_wakeup (context);
+	  curr->data = GINT_TO_POINTER (pid);
+	  G_UNLOCK (child_watch_pids);
+	  return;
 	}
-      G_UNLOCK (main_context_list);
     }
-  return NULL;
+  
+  new_pid = g_slist_alloc ();
+  new_pid->data = GINT_TO_POINTER (pid);
+  new_pid->next = NULL;
+
+  /* The aim of this seemingly unnecessary lock and unlock is to have a
+   * memory barrier between writing the memory of new_pid and
+   * appending it to the list. This function is not time critical, so
+   * this is OK. */
+#ifdef G_ATOMIC_OP_MEMORY_BARRIER_NEEDED 
+  {
+    G_LOCK_DEFINE_STATIC (child_watch_enforce_memory_barrier);
+    G_LOCK (child_watch_enforce_memory_barrier);
+    G_UNLOCK (child_watch_enforce_memory_barrier);
+  }
+#endif /* G_ATOMIC_OP_MEMORY_BARRIER_NEEDED */
+
+  g_slist_last (child_watch_pids)->next = new_pid;
+  G_UNLOCK (child_watch_pids);
 }
 
 static void
-g_child_watch_source_init_multi_threaded (void)
+g_child_watch_remove_pid (GPid pid)
 {
-  GError *error = NULL;
+  GSList *curr;
+  
+  /* We can not simply leave "-1" holes in the list, as we need the
+   * waitpid call in the signal handler as a memory barrier, so it has
+   * to stop at the first "-1" it encounters. */
+  G_LOCK (child_watch_pids);
+  for (curr = child_watch_pids; curr; curr = curr->next)
+    {
+      if (GPOINTER_TO_INT (curr->data) == pid)
+	{
+	  while (curr->next && curr->data != GINT_TO_POINTER (-1))
+	    {
+	      curr->data = curr->next->data;
+	      curr = curr->next;
+	    }
+	  curr->data = GINT_TO_POINTER (-1);	  
+	  
+	  G_UNLOCK (child_watch_pids);
+	  return;
+	}
+    }
+  g_assert_not_reached ();
+}
 
-  g_assert (g_thread_supported());
+static gpointer
+g_child_watch_source_init (gpointer ignore)
+{
+  struct sigaction signal_action;
+  signal_action.sa_handler = g_child_watch_signal_handler;
+  sigemptyset (&signal_action.sa_mask);
+  signal_action.sa_flags = 0;
 
   if (pipe (child_watch_wake_up_pipe) < 0)
     g_error ("Cannot create wake up pipe: %s\n", g_strerror (errno));
-  fcntl (child_watch_wake_up_pipe[1], F_SETFL, O_NONBLOCK | fcntl (child_watch_wake_up_pipe[1], F_GETFL));
+  fcntl (child_watch_wake_up_pipe[0], F_SETFL, 
+	 O_NONBLOCK | fcntl (child_watch_wake_up_pipe[0], F_GETFL));
 
-  /* We create a helper thread that polls on the wakeup pipe indefinitely */
-  /* FIXME: Think this through for races */
-  if (g_thread_create (child_watch_helper_thread, NULL, FALSE, &error) == NULL)
-    g_error ("Cannot create a thread to monitor child exit status: %s\n", error->message);
-  child_watch_init_state = CHILD_WATCH_INITIALIZED_THREADED;
-  signal (SIGCHLD, g_child_watch_signal_handler);
-}
+  sigaction (SIGCHLD, &signal_action, NULL);
 
-static void
-g_child_watch_source_init_promote_single_to_threaded (void)
-{
-  g_child_watch_source_init_multi_threaded ();
+  return NULL;  
 }
-
-static void
-g_child_watch_source_init (void)
-{
-  if (g_thread_supported())
-    {
-      if (child_watch_init_state == CHILD_WATCH_UNINITIALIZED)
-	g_child_watch_source_init_multi_threaded ();
-      else if (child_watch_init_state == CHILD_WATCH_INITIALIZED_SINGLE)
-	g_child_watch_source_init_promote_single_to_threaded ();
-    }
-  else
-    {
-      if (child_watch_init_state == CHILD_WATCH_UNINITIALIZED)
-	g_child_watch_source_init_single ();
-    }
-}
-
 #endif /* !G_OS_WIN32 */
 
 /**
@@ -3621,7 +3670,9 @@
 GSource *
 g_child_watch_source_new (GPid pid)
 {
-  GSource *source = g_source_new (&g_child_watch_funcs, sizeof (GChildWatchSource));
+  static GOnce once = G_ONCE_INIT;
+  GSource *source = g_source_new (&g_child_watch_funcs, 
+				  sizeof (GChildWatchSource));
   GChildWatchSource *child_watch_source = (GChildWatchSource *)source;
 
 #ifdef G_OS_WIN32
@@ -3629,10 +3680,17 @@
   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 ();
+#else /* G_OS_WIN32 */  
+  g_once (&once, g_child_watch_source_init, NULL);
+
+  child_watch_source->poll.fd = child_watch_wake_up_pipe[0];
+  child_watch_source->poll.events = G_IO_IN;
+
+  g_source_add_poll (source, &child_watch_source->poll);
+  g_child_watch_append_pid (pid);
 #endif /* G_OS_WIN32 */
 
+  child_watch_source->child_exited = FALSE;
   child_watch_source->pid = pid;
 
   return source;
Index: tests/child-test.c
===================================================================
RCS file: /cvs/gnome/glib/tests/child-test.c,v
retrieving revision 1.7
diff -u -r1.7 child-test.c
--- tests/child-test.c	9 Mar 2004 00:12:36 -0000	1.7
+++ tests/child-test.c	11 Mar 2004 10:08:53 -0000
@@ -89,7 +89,7 @@
 #endif /* G_OS_WIN32 */
 }
 
-gboolean
+void
 child_watch_callback (GPid pid, gint status, gpointer data)
 {
   gint ttl = GPOINTER_TO_INT (data);
@@ -101,9 +101,10 @@
   if (--alive == 0)
     g_main_loop_quit (main_loop);
 
-  return TRUE;
 }
 
+#define TEST_THREAD 1
+
 #ifdef TEST_THREAD
 static gpointer
 test_thread (gpointer data)
@@ -150,9 +151,8 @@
    * implementation is available.
    */
 #if defined(G_THREADS_ENABLED) && ! defined(G_THREADS_IMPL_NONE)
-#ifdef TEST_THREAD
   g_thread_init (NULL);
-#else
+#ifndef TEST_THREAD
   GPid pid;
 #endif
   main_loop = g_main_loop_new (NULL, FALSE);
@@ -160,18 +160,34 @@
 #ifdef G_OS_WIN32
   system ("ipconfig /all");
 #else
-  system ("/bin/true");
+  //system ("/bin/true");
 #endif
 
-  alive = 2;
 #ifdef TEST_THREAD
-  g_thread_create (test_thread, GINT_TO_POINTER (10), FALSE, NULL);
-  g_thread_create (test_thread, GINT_TO_POINTER (20), FALSE, NULL);
+  alive = 4;
+  g_thread_create (test_thread, GINT_TO_POINTER (4), FALSE, NULL);
+  g_thread_create (test_thread, GINT_TO_POINTER (3), FALSE, NULL);
+  g_thread_create (test_thread, GINT_TO_POINTER (2), FALSE, NULL);
+  g_thread_create (test_thread, GINT_TO_POINTER (5), FALSE, NULL);
+  
+  g_main_loop_run (main_loop);
+
+  alive = 3;
+  g_thread_create (test_thread, GINT_TO_POINTER (6), FALSE, NULL);
+  g_thread_create (test_thread, GINT_TO_POINTER (2), FALSE, NULL);
+  g_thread_create (test_thread, GINT_TO_POINTER (4), FALSE, NULL);
+
+  g_main_loop_run (main_loop);
+
+  alive = 2;
+  g_thread_create (test_thread, GINT_TO_POINTER (2), FALSE, NULL);
+  g_thread_create (test_thread, GINT_TO_POINTER (2), FALSE, NULL);
 #else
-  pid = get_a_child (10);
-  g_child_watch_add (pid, child_watch_callback, GINT_TO_POINTER (10));
+  alive = 2;
   pid = get_a_child (20);
   g_child_watch_add (pid, child_watch_callback, GINT_TO_POINTER (20));
+  pid = get_a_child (10);
+  g_child_watch_add (pid, child_watch_callback, GINT_TO_POINTER (10));
 #endif
   
   g_main_loop_run (main_loop);
