[glib/wip/gcleanup: 11/21] gcleanup: Use three passes to do cleanup

[Stefan Walter <stefw src gnome org>]

commit 6c0e25cc4aff04ab1d2997d08e227eb08a119d3c
Author: Stef Walter <stefw gnome org>
Date:   Thu Oct 31 15:07:53 2013 +0100

    gcleanup: Use three passes to do cleanup
    
    This allows certain handlers to postpone their cleanup until later
    as long as these second handlers guarantee not to add anything
    else. Three passes is what's needed to make this scheme work.
    
    This is a simple solution for the GPrivate problem, and does not
    require g_private_reset(), which ended up leaking anyway, when
    shutdown handers used thread specific data.
    
    This patch also fixes crahses when g_mutex_init() or other threading
    xxx_init() functions were used. g_mutex_clear() removes the cleanup
    handler.
    
    Lastly by doing this GCleanupList does not require a lock that itself
    needs to be cleaned up.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=627423

 glib/gcleanup.c      |   78 +++++++++++++++++++++++++++++++++++++++----------
 glib/gthread-posix.c |   56 ++++++++++++++++++++++++++++++++----
 2 files changed, 112 insertions(+), 22 deletions(-)
---
diff --git a/glib/gcleanup.c b/glib/gcleanup.c
index 092bd8b..dc4b629 100644
--- a/glib/gcleanup.c
+++ b/glib/gcleanup.c
@@ -30,8 +30,21 @@
  * NOTE: most glib functions are off limits in this function without
  * careful consideration. In particular error printing and logging functions,
  * use various locks, which would cause issues during cleanup.
+ *
  */
 
+/*
+ * The gcleanup code has three passes.
+ *
+ * This is so things that need to absolutely be cleaned up last, add
+ * themselves to the next pass from within their cleanup handler. These
+ * are used for GMutex, GPrivate and so on.
+ */
+
+#ifndef GLIB_CLEANUP_PASSES
+#define GLIB_CLEANUP_PASSES 3
+#endif
+
 #include <stdio.h>
 
 /* As good a place as any to put this... */
@@ -46,8 +59,6 @@ struct _GCleanupNode
   GCleanupNode   *next;
 };
 
-static GMutex lock;
-
 /**
  * g_cleanup_is_enabled:
  *
@@ -108,10 +119,11 @@ g_cleanup_list_add (GCleanupList *list,
   node->data = user_data;
   node->code_loc = code_loc;
 
-  g_mutex_lock (&lock);
-  node->next = list->priv[0];
-  list->priv[0] = node;
-  g_mutex_unlock (&lock);
+  do
+    {
+      node->next = list->priv[0];
+    }
+  while (!g_atomic_pointer_compare_and_exchange (&list->priv[0], node->next, node));
 
   if (list->vals[0] & G_CLEANUP_VERBOSE)
     fprintf (stderr, "GLib-Cleanup: added: %s %p\n", code_loc, user_data);
@@ -125,9 +137,6 @@ g_cleanup_list_add (GCleanupList *list,
  *
  * This results in all of the previously-added functions being called.
  *
- * This function is not threadsafe.  Nothing else may be accessing the
- * list at the time that this function is called.
- *
  * You usually do not need to call this directly.  G_CLEANUP_DEFINE will
  * emit a destructor function to call this when your library or program
  * is being unloaded.
@@ -138,19 +147,56 @@ void
 g_cleanup_list_clear (GCleanupList *list)
 {
   gboolean verbose;
+  GCleanupNode *clear;
+  gint i;
 
   if (!g_cleanup_enabled)
     return;
 
   verbose = list->vals[0] & G_CLEANUP_VERBOSE;
-  while (list->priv[0])
+  for (i = 0; i < GLIB_CLEANUP_PASSES + 1; i++)
     {
-      GCleanupNode *node = list->priv[0];
-      if (verbose)
-        fprintf (stderr, "GLib-Cleanup: cleanup: %s %p\n", node->code_loc, node->data);
-      (* node->func) (node->data);
-      list->priv[0] = node->next;
-      free (node);
+      do
+        {
+          /* Steal the list pointer */
+          clear = list->priv[0];
+        }
+      while (!g_atomic_pointer_compare_and_exchange (&list->priv[0], clear, NULL));
+
+      if (!clear)
+        {
+          if (verbose)
+            fprintf (stderr, "GLib-Cleanup: cleanup done\n");
+          break;
+        }
+
+      if (i == GLIB_CLEANUP_PASSES)
+        {
+          fprintf (stderr, "GLib-Cleanup: gave up after %d passes\n", GLIB_CLEANUP_PASSES);
+        }
+      else if (i > 0)
+        {
+          if (verbose)
+            fprintf (stderr, "GLib-Cleanup: performing pass %d\n", i + 1);
+        }
+
+
+      while (clear)
+        {
+          GCleanupNode *node = clear;
+          if (i == GLIB_CLEANUP_PASSES)
+            {
+               fprintf (stderr, "GLib-Cleanup: ignoring cleanup for: %s %p\n", node->code_loc, node->data);
+            }
+          else
+            {
+              if (verbose)
+                fprintf (stderr, "GLib-Cleanup: cleanup: %s %p\n", node->code_loc, node->data);
+              (* node->func) (node->data);
+            }
+          clear = node->next;
+          free (node);
+        }
     }
 }
 
diff --git a/glib/gthread-posix.c b/glib/gthread-posix.c
index bc0da94..d5b9a2e 100644
--- a/glib/gthread-posix.c
+++ b/glib/gthread-posix.c
@@ -119,16 +119,23 @@ g_mutex_impl_free (pthread_mutex_t *mutex)
   free (mutex);
 }
 
+static void
+g_mutex_impl_cleanup (pthread_mutex_t *mutex)
+{
+  /* Do mutexes near the end, so other cleanup can use them */
+  G_CLEANUP_ADD (mutex, g_mutex_impl_free);
+}
+
 static pthread_mutex_t *
 g_mutex_get_impl (GMutex *mutex)
 {
-  pthread_mutex_t *impl = g_atomic_pointer_get (&mutex->p);
+  gpointer impl = g_atomic_pointer_get (&mutex->p);
 
   if G_UNLIKELY (impl == NULL)
     {
       impl = g_mutex_impl_new ();
       if (g_atomic_pointer_compare_and_exchange (&mutex->p, NULL, impl))
-        G_CLEANUP_ADD (impl, g_mutex_impl_free);
+        G_CLEANUP_ADD (impl, g_mutex_impl_cleanup);
       else
         g_mutex_impl_free (impl);
       impl = mutex->p;
@@ -290,6 +297,13 @@ g_rec_mutex_impl_free (pthread_mutex_t *mutex)
   g_slice_free (pthread_mutex_t, mutex);
 }
 
+static void
+g_rec_mutex_impl_cleanup (pthread_mutex_t *mutex)
+{
+  /* Perform actual cleanup in the next pass */
+  G_CLEANUP_ADD (mutex, g_rec_mutex_impl_free);
+}
+
 static pthread_mutex_t *
 g_rec_mutex_get_impl (GRecMutex *rec_mutex)
 {
@@ -299,7 +313,7 @@ g_rec_mutex_get_impl (GRecMutex *rec_mutex)
     {
       impl = g_rec_mutex_impl_new ();
       if (g_atomic_pointer_compare_and_exchange (&rec_mutex->p, NULL, impl))
-        G_CLEANUP_ADD (impl, g_rec_mutex_impl_free);
+        G_CLEANUP_ADD (impl, g_rec_mutex_impl_cleanup);
       else
         g_rec_mutex_impl_free (impl);
       impl = rec_mutex->p;
@@ -452,6 +466,12 @@ g_rw_lock_impl_free (pthread_rwlock_t *rwlock)
   free (rwlock);
 }
 
+static void
+g_rw_lock_impl_cleanup (pthread_rwlock_t *rwlock)
+{
+  G_CLEANUP_ADD (rwlock, g_rw_lock_impl_free);
+}
+
 static pthread_rwlock_t *
 g_rw_lock_get_impl (GRWLock *lock)
 {
@@ -461,7 +481,7 @@ g_rw_lock_get_impl (GRWLock *lock)
     {
       impl = g_rw_lock_impl_new ();
       if (g_atomic_pointer_compare_and_exchange (&lock->p, NULL, impl))
-        G_CLEANUP_ADD (impl, g_rw_lock_impl_free);
+        G_CLEANUP_ADD (impl, g_rw_lock_impl_cleanup);
       else
         g_rw_lock_impl_free (impl);
       impl = lock->p;
@@ -671,6 +691,12 @@ g_cond_impl_free (pthread_cond_t *cond)
   free (cond);
 }
 
+static void
+g_cond_impl_cleanup (pthread_cond_t *impl)
+{
+  G_CLEANUP_ADD (impl, g_cond_impl_free);
+}
+
 static pthread_cond_t *
 g_cond_get_impl (GCond *cond)
 {
@@ -680,7 +706,7 @@ g_cond_get_impl (GCond *cond)
     {
       impl = g_cond_impl_new ();
       if (g_atomic_pointer_compare_and_exchange (&cond->p, NULL, impl))
-        G_CLEANUP_ADD (impl, g_cond_impl_free);
+        G_CLEANUP_ADD (impl, g_cond_impl_cleanup);
       else
         g_cond_impl_free (impl);
       impl = cond->p;
@@ -991,6 +1017,24 @@ g_private_impl_free (pthread_key_t *key)
   free (key);
 }
 
+static void
+g_private_cleanup (GPrivate *key)
+{
+  pthread_key_t *impl = g_atomic_pointer_get (&key->p);
+  gpointer old;
+  gint status;
+
+  old = pthread_getspecific (*impl);
+  if (old && key->notify)
+    key->notify (old);
+
+  if G_UNLIKELY ((status = pthread_setspecific (*impl, NULL)) != 0)
+    g_thread_abort (status, "pthread_setspecific");
+
+  /* Cleanup as the very last thing */
+  G_CLEANUP_ADD (impl, g_private_impl_free);
+}
+
 static pthread_key_t *
 g_private_get_impl (GPrivate *key)
 {
@@ -1001,7 +1045,7 @@ g_private_get_impl (GPrivate *key)
       impl = g_private_impl_new (key->notify);
       if (g_atomic_pointer_compare_and_exchange (&key->p, NULL, impl))
         {
-          G_CLEANUP_ADD (impl, g_private_impl_free);
+          G_CLEANUP_ADD (key, g_private_cleanup);
         }
       else
         {