From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Andy Wingo Newsgroups: gmane.lisp.guile.devel Subject: Re: Trouble joining with threads from C Date: Fri, 18 Mar 2011 13:23:21 +0100 Message-ID: References: <87y64rzd5p.fsf@netris.org> <8762rswzzf.fsf@netris.org> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: dough.gmane.org 1300451468 19001 80.91.229.12 (18 Mar 2011 12:31:08 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Fri, 18 Mar 2011 12:31:08 +0000 (UTC) Cc: guile-devel@gnu.org To: Mark H Weaver Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Fri Mar 18 13:31:04 2011 Return-path: Envelope-to: guile-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Q0Ypb-00076R-8L for guile-devel@m.gmane.org; Fri, 18 Mar 2011 13:31:04 +0100 Original-Received: from localhost ([127.0.0.1]:55117 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q0Ypa-0002Mm-L0 for guile-devel@m.gmane.org; Fri, 18 Mar 2011 08:31:02 -0400 Original-Received: from [140.186.70.92] (port=37395 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q0YpX-0002Kz-Aj for guile-devel@gnu.org; Fri, 18 Mar 2011 08:31:00 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q0YpU-0002lG-M7 for guile-devel@gnu.org; Fri, 18 Mar 2011 08:30:59 -0400 Original-Received: from a-pb-sasl-sd.pobox.com ([64.74.157.62]:37034 helo=sasl.smtp.pobox.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q0YpU-00029a-GL for guile-devel@gnu.org; Fri, 18 Mar 2011 08:30:56 -0400 Original-Received: from sasl.smtp.pobox.com (unknown [127.0.0.1]) by a-pb-sasl-sd.pobox.com (Postfix) with ESMTP id 69BFC3DCB; Fri, 18 Mar 2011 08:30:26 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; s=sasl; bh=2XB5cebqlVXQcgHLCdYdWA+PJpI=; b=PE5ZUA LzLWVvzFHklWHVJHWYYaSP8Rbwil+SipaA96liUdJgi7GShbVmk64z4XDsl4Az9U IoYQuHlPvEY9Pki6D8pusvtlMzOCgKVF5Ff3gNTBTZR6UtHEt23PFUWtQu4jfnvg DdRd9eRFaP4LT5MDMErwQSnzzNewPLqXkafMM= DomainKey-Signature: a=rsa-sha1; c=nofws; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; q=dns; s=sasl; b=eKBEdDIZQANv1wQRycIayGUZvHS74uMW TJ0W+h4IGgYYohLf2i/QTkv0Pl5xAADCOMcaQnCI0iqo0vIfPJgFqpHvttLcHBn1 xe+YQ4l9von1V/cYSEowjcyJROnj0fcP0KV8HxYhSvY3690adlBHOGz0IvN1uLXS TU24lVQYtvY= Original-Received: from a-pb-sasl-sd.pobox.com (unknown [127.0.0.1]) by a-pb-sasl-sd.pobox.com (Postfix) with ESMTP id 533CD3DCA; Fri, 18 Mar 2011 08:30:24 -0400 (EDT) Original-Received: from unquote.localdomain (unknown [90.164.198.39]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by a-pb-sasl-sd.pobox.com (Postfix) with ESMTPSA id 533E73DC9; Fri, 18 Mar 2011 08:30:21 -0400 (EDT) In-Reply-To: <8762rswzzf.fsf@netris.org> (Mark H. Weaver's message of "Wed, 09 Mar 2011 01:48:52 -0500") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) X-Pobox-Relay-ID: 7F662070-515B-11E0-AFCA-E8AB60295C12-02397024!a-pb-sasl-sd.pobox.com X-detected-operating-system: by eggs.gnu.org: Solaris 10 (beta) X-Received-From: 64.74.157.62 X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Errors-To: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.lisp.guile.devel:11905 Archived-At: --=-=-= Hi Mark, On Wed 09 Mar 2011 07:48, Mark H Weaver 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 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0001-fix-thread-cleanup.patch >From 4732679c36305b886cbb6bd3473154731535cb0b Mon Sep 17 00:00:00 2001 From: Andy Wingo 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 --=-=-= -- http://wingolog.org/ --=-=-=--