Re: Trouble joining with threads from C

[Andy Wingo <wingo@pobox.com>]

[-- Attachment #1: Type: text/plain, Size: 1291 bytes --]

Hi Mark,

On Wed 09 Mar 2011 07:48, Mark H Weaver <mhw@netris.org> writes:

> Attached are two minimal C programs.  Both create threads that do
> nothing but sleep for 2 seconds and then exit.  The parent tries to join
> with the child thread, with a timeout of 10 seconds.
>
> The only difference is that test1 uses scm_spawn_thread, and test2 uses
> scm_call_with_new_thread.  test1 always times out, and test2 works
> properly.

Indeed, a bug.

> Having looked at the code in threads.c, I'm surprised that either of
> them work.  Both scm_spawn_thread and scm_call_with_new_thread arrange
> for pthread_detach to be called in the child (in spawn_thread and
> launch_thread, respectively).  This is supposed to make the child
> un-joinable.

scm_join_thread_timed doesn't use pthread_join.  I don't recall exactly
why, but its implementation does look OK, at least at first glance.

No, the issue is elsewhere, that the thread-exit handlers were not being
called, which makes me trust our pthreads support much less than before,
if this bug could exist for so long.  I have fixed this in Git with the
attached patch, which fixes your test as well.

Would you mind submitting (and committing :) a test case?  Adding it to
test-suite/standalone is probably the thing to do.

Regards,

Andy


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-fix-thread-cleanup.patch --]
[-- Type: text/x-patch, Size: 6719 bytes --]

From 4732679c36305b886cbb6bd3473154731535cb0b Mon Sep 17 00:00:00 2001
From: Andy Wingo <wingo@pobox.com>
Date: Fri, 18 Mar 2011 13:18:47 +0100
Subject: [PATCH] fix thread cleanup

* libguile/threads.h: Always declare a scm_i_thread_key, for cleanup
  purposes, in the BUILDING_LIBGUILE case.

* libguile/threads.c (scm_i_thread_key): Init with a cleanup handler, so
  any guile-specific info for a thread can be cleaned up reliably.
  (guilify_self_1): Always set the thread key.
  (do_thread_exit_trampoline, on_thread_exit): Enter guile-mode for the
  guile-mode cleanup handler, and trampoline through a
  gc_call_with_stack_base for reasons explained in the code.
  (init_thread_key, scm_i_init_thread_for_guile): Always init the key.
  (scm_i_with_guile_and_parent): No need for pthread_cancel cleanup
  handlers, as the pthread key destructor will take care of that for
  us.
  (really_launch): Remove needless pthread_exit call with incorrect
  comment.
---
 libguile/threads.c |   71 ++++++++++++++++++++++++++++-----------------------
 libguile/threads.h |    5 +++-
 2 files changed, 43 insertions(+), 33 deletions(-)

diff --git a/libguile/threads.c b/libguile/threads.c
index e7347ad..e2e17ac 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -343,6 +343,12 @@ unblock_from_queue (SCM queue)
 /* Getting into and out of guile mode.
  */
 
+/* Key used to attach a cleanup handler to a given thread.  Also, if
+   thread-local storage is unavailable, this key is used to retrieve the
+   current thread with `pthread_getspecific ()'.  */
+scm_i_pthread_key_t scm_i_thread_key;
+
+
 #ifdef SCM_HAVE_THREAD_STORAGE_CLASS
 
 /* When thread-local storage (TLS) is available, a pointer to the
@@ -352,17 +358,7 @@ unblock_from_queue (SCM queue)
    represent.  */
 SCM_THREAD_LOCAL scm_i_thread *scm_i_current_thread = NULL;
 
-# define SET_CURRENT_THREAD(_t)  scm_i_current_thread = (_t)
-
-#else /* !SCM_HAVE_THREAD_STORAGE_CLASS */
-
-/* Key used to retrieve the current thread with `pthread_getspecific ()'.  */
-scm_i_pthread_key_t scm_i_thread_key;
-
-# define SET_CURRENT_THREAD(_t)				\
-  scm_i_pthread_setspecific (scm_i_thread_key, (_t))
-
-#endif /* !SCM_HAVE_THREAD_STORAGE_CLASS */
+#endif /* SCM_HAVE_THREAD_STORAGE_CLASS */
 
 
 static scm_i_pthread_mutex_t thread_admin_mutex = SCM_I_PTHREAD_MUTEX_INITIALIZER;
@@ -428,7 +424,12 @@ guilify_self_1 (SCM_STACKITEM *base)
   t->exited = 0;
   t->guile_mode = 0;
 
-  SET_CURRENT_THREAD (t);
+  scm_i_pthread_setspecific (scm_i_thread_key, t);
+
+#ifdef SCM_HAVE_THREAD_STORAGE_CLASS
+  /* Cache the current thread in TLS for faster lookup.  */
+  scm_i_current_thread = t;
+#endif
 
   scm_i_pthread_mutex_lock (&thread_admin_mutex);
   t->next_thread = all_threads;
@@ -537,6 +538,22 @@ do_thread_exit (void *v)
   return NULL;
 }
 
+static void *
+do_thread_exit_trampoline (struct GC_stack_base *sb, void *v)
+{
+  void *ret;
+  int registered;
+
+  registered = GC_register_my_thread (sb);
+
+  ret = scm_with_guile (do_thread_exit, v);
+
+  if (registered == GC_SUCCESS)
+    GC_unregister_my_thread ();
+
+  return ret;
+}
+
 static void
 on_thread_exit (void *v)
 {
@@ -551,19 +568,20 @@ on_thread_exit (void *v)
       t->held_mutex = NULL;
     }
 
-  SET_CURRENT_THREAD (v);
+  /* Reinstate the current thread for purposes of scm_with_guile
+     guile-mode cleanup handlers.  Only really needed in the non-TLS
+     case but it doesn't hurt to be consistent.  */
+  scm_i_pthread_setspecific (scm_i_thread_key, t);
 
   /* Ensure the signal handling thread has been launched, because we might be
      shutting it down.  */
   scm_i_ensure_signal_delivery_thread ();
 
   /* Unblocking the joining threads needs to happen in guile mode
-     since the queue is a SCM data structure.  */
-
-  /* Note: Since `do_thread_exit ()' uses allocates memory via `libgc', we
-     assume the GC is usable at this point, and notably that thread-local
-     storage (TLS) hasn't been deallocated yet.  */
-  do_thread_exit (v);
+     since the queue is a SCM data structure.   Trampoline through
+     GC_call_with_stack_base so that the GC works even if it already
+     cleaned up for this thread.  */
+  GC_call_with_stack_base (do_thread_exit_trampoline, v);
 
   /* Removing ourself from the list of all threads needs to happen in
      non-guile mode since all SCM values on our stack become
@@ -590,21 +608,17 @@ on_thread_exit (void *v)
 
   scm_i_pthread_mutex_unlock (&thread_admin_mutex);
 
-  SET_CURRENT_THREAD (NULL);
+  scm_i_pthread_setspecific (scm_i_thread_key, NULL);
 }
 
-#ifndef SCM_HAVE_THREAD_STORAGE_CLASS
-
 static scm_i_pthread_once_t init_thread_key_once = SCM_I_PTHREAD_ONCE_INIT;
 
 static void
 init_thread_key (void)
 {
-  scm_i_pthread_key_create (&scm_i_thread_key, NULL);
+  scm_i_pthread_key_create (&scm_i_thread_key, on_thread_exit);
 }
 
-#endif
-
 /* Perform any initializations necessary to make the current thread
    known to Guile (via SCM_I_CURRENT_THREAD), initializing Guile itself,
    if necessary.
@@ -625,9 +639,7 @@ init_thread_key (void)
 static int
 scm_i_init_thread_for_guile (SCM_STACKITEM *base, SCM parent)
 {
-#ifndef SCM_HAVE_THREAD_STORAGE_CLASS
   scm_i_pthread_once (&init_thread_key_once, init_thread_key);
-#endif
 
   if (SCM_I_CURRENT_THREAD)
     {
@@ -790,9 +802,7 @@ scm_i_with_guile_and_parent (void *(*func)(void *), void *data, SCM parent)
       /* We are in Guile mode.  */
       assert (t->guile_mode);
 
-      scm_i_pthread_cleanup_push (scm_leave_guile_cleanup, NULL);
       res = scm_c_with_continuation_barrier (func, data);
-      scm_i_pthread_cleanup_pop (0);
 
       /* Leave Guile mode.  */
       t->guile_mode = 0;
@@ -880,9 +890,6 @@ really_launch (void *d)
   else
     t->result = scm_catch (SCM_BOOL_T, thunk, handler);
 
-  /* Trigger a call to `on_thread_exit ()'.  */
-  pthread_exit (NULL);
-
   return 0;
 }
 
diff --git a/libguile/threads.h b/libguile/threads.h
index b5e3c21..475af32 100644
--- a/libguile/threads.h
+++ b/libguile/threads.h
@@ -192,6 +192,10 @@ SCM_API void scm_dynwind_critical_section (SCM mutex);
 
 #ifdef BUILDING_LIBGUILE
 
+/* Though we don't need the key for SCM_I_CURRENT_THREAD if we have TLS,
+   we do use it for cleanup purposes.  */
+SCM_INTERNAL scm_i_pthread_key_t scm_i_thread_key;
+
 # ifdef SCM_HAVE_THREAD_STORAGE_CLASS
 
 SCM_INTERNAL SCM_THREAD_LOCAL scm_i_thread *scm_i_current_thread;
@@ -199,7 +203,6 @@ SCM_INTERNAL SCM_THREAD_LOCAL scm_i_thread *scm_i_current_thread;
 
 # else /* !SCM_HAVE_THREAD_STORAGE_CLASS */
 
-SCM_INTERNAL scm_i_pthread_key_t scm_i_thread_key;
 #  define SCM_I_CURRENT_THREAD						\
     ((scm_i_thread *) scm_i_pthread_getspecific (scm_i_thread_key))
 
-- 
1.7.3.4


[-- Attachment #3: Type: text/plain, Size: 25 bytes --]

-- 
http://wingolog.org/