Re: Signal thread

[Ludovic Courtès]

Hello fellow Guiler of the Guild!

Andy Wingo <address@hidden> skribis:

> Hello guild-comrade Courtès!
>
> On Mon 27 Jun 2011 18:00, address@hidden (Ludovic Courtès) writes:
>
>> "Andy Wingo" <address@hidden> skribis:
>>
>>> commit 3b971a59b55586a236c3621a55515d9272ee5c80
>>> Author: Andy Wingo <address@hidden>
>>> Date:   Thu Jun 23 11:24:16 2011 +0200
>>>
>>>     don't spawn the signal delivery thread in a thread-exit handler
>>>     
>>>     * libguile/threads.c (do_thread_exit): Remove needless spawn of the
>>>       signal delivery thread.
>>
>> Out of curiosity, what’s the rationale?  Should it be done in stable-2.0
>> as well?
>
> Out of curiosity, I looked :)  The commit that added the spawning code
> was 2e77f7202b11ad0003831fcff94ec7db80cca015 from you, which
> unfortunately has no log.

After a moment of shame, I looked at libguile/ChangeLog-2008 and found
evidence that I’m only confederate at worst:

  2007-10-20  Julian Graham  <address@hidden>

          Add support for thread cancellation and user-defined thread
          cleanup handlers.  Small rework by Ludovic Courtès.

          * null-threads.h (scm_i_pthread_cancel,
          scm_i_pthread_cleanup_push, scm_i_pthread_cleanup_pop): New.
          * pthread-threads.h (scm_i_pthread_cancel,
          scm_i_pthread_cleanup_push, scm_i_pthread_cleanup_pop): New.
          * scmsigs.c (scm_i_signal_delivery_thread,
          signal_delivery_thread_mutex): New.
          (signal_delivery_thread): Leave when `read_without_guile ()'
          returns zero.
          (start_signal_delivery_thread): Acquire SIGNAL_DELIVERY_THREAD
          before spawning the thread.  Initialize
          SCM_I_SIGNAL_DELIVERY_THREAD.
          (ensure_signal_delivery_thread): Renamed to...
          (scm_i_ensure_signal_delivery_thread): this.
          (scm_i_close_signal_pipe): New.
          * scmsigs.h: Updated.
          * threads.c (thread_mark): Mark `t->cleanup_handler'.
          (guilify_self_1): Initialize `t->cleanup_handler' and
          `t->canceled'.
          (do_thread_exit): Invoke `t->cleanup_handler'.
          (on_thread_exit): Call `scm_i_ensure_signal_delivery_thread ()'.
          Call `scm_i_close_signal_pipe ()' when the next-to-last thread
          vanishes.
          (scm_leave_guile_cleanup): New.
          (scm_i_with_guile_and_parent): Use `scm_i_pthread_cleanup_push ()'
          and `scm_leave_guile_cleanup ()' to leave guile mode, rather
          than call `scm_leave_guile ()' after FUNC.
          (scm_cancel_thread, scm_set_thread_cleanup_x,
          scm_threads_cleanup): New.
          (scm_all_threads): Remove SCM_I_SIGNAL_DELIVERY_THREAD from the
          returned list.
          * threads.h (scm_i_thread)[cleanup_handler, canceled]: New
          fields.
          Add declarations of new functions.

Pffew.

The explanation appears to be at
<http://lists.gnu.org/archive/html/guile-devel/2007-10/msg00029.html>:

  * If the signal delivery thread got launched a little bit too late, it
  could be holding its startup mutex and then attempt to grab the
  thread_admin_mutex, which could be held by a thread that was in the
  process of being canceled and which was trying to obtain the signal
  delivery thread's startup mutex.  I've resolved this by forcing the
  signal delivery thread to start (if it hasn't already) during thread
  cancellation of any thread.

At first sight it still holds.

I checked on ‘stable-2.0’ by applying
3b971a59b55586a236c3621a55515d9272ee5c80 and then running threads.test
under Helgrind, which didn’t report any lock order issue.  It could mean
that I was lucky, though.

> Actually, let's take this opportunity to have a moment of silence for
> Arch.

And the terrible tla-cvs-sync!

>> As a side note, why does this area have to change so often?  :-)
>
> I understand the frustration :)  Threads are hard, signals are hard, GC
> and threads and signals is hard.  But I do hope it's getting better.

We have too few tests for the tricky corner cases, but OTOH they are
difficult to write.  The number of simpler thread-related tests has
increased though, which probably means we caught a few bugs already, so
there’s hope!  ;-)

Thanks,
Ludo’.