Signal thread

Signal thread

[Ludovic Courtès]

Hello!

"Andy Wingo" <wingo@pobox.com> skribis:

> commit 3b971a59b55586a236c3621a55515d9272ee5c80
> Author: Andy Wingo <wingo@pobox.com>
> 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?

As a side note, why does this area have to change so often?  :-)

Thanks,
Ludo’.

Re: Signal thread

[Andy Wingo]

Hello guild-comrade Courtès!

On Mon 27 Jun 2011 18:00, ludo@gnu.org (Ludovic Courtès) writes:

> "Andy Wingo" <wingo@pobox.com> skribis:
>
>> commit 3b971a59b55586a236c3621a55515d9272ee5c80
>> Author: Andy Wingo <wingo@pobox.com>
>> 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.  Actually, let's take this opportunity to have
a moment of silence for Arch.





Ah.  I remember those days and am quite happy to live in the age of git
:-)

Anyway, spawning a thread just to shut it down didn't make any sense to
me.  I looked and looked and couldn't justify it.  It seems that the
only case that might matter, closing the signal pipe, worked just fine
whether the thread had been spawned or not.  So I removed that code.

Should it apply to 2.0?  Perhaps, but I didn't want it to block 2.0.2.

> 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.

Cheers,

Andy
-- 
http://wingolog.org/

Re: Signal thread

[Ludovic Courtès]

Hello fellow Guiler of the Guild!

Andy Wingo <wingo@pobox.com> skribis:

> Hello guild-comrade Courtès!
>
> On Mon 27 Jun 2011 18:00, ludo@gnu.org (Ludovic Courtès) writes:
>
>> "Andy Wingo" <wingo@pobox.com> skribis:
>>
>>> commit 3b971a59b55586a236c3621a55515d9272ee5c80
>>> Author: Andy Wingo <wingo@pobox.com>
>>> 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  <joolean@gmail.com>

          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’.

Re: Signal thread

[Andy Wingo]

Greets!

On Tue 28 Jun 2011 18:46, ludo@gnu.org (Ludovic Courtès) writes:

>   2007-10-20  Julian Graham  <joolean@gmail.com>
>
>           Add support for thread cancellation and user-defined thread
>           cleanup handlers.  Small rework by Ludovic Courtès.

Ah, right.  I suppose it's disrespectful to have a moment of silence for
CVS, given that people still use it, but I don't miss its inability to
distinguish authors from committers.

> <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.

Currently scm_cancel_thread is called without any locks held, so there
should be no issues there.

We might have other locking order problems -- do_thread_exit takes the
admin lock and then the lock of a fat mutex, whereas fat_mutex_lock does
the other way around.  (Yuk, fat mutexes.)  Perhaps not, though.

Cheers,

Andy
-- 
http://wingolog.org/

Re: Signal thread

[Ludovic Courtès]

Andy Wingo <wingo@pobox.com> skribis:

> We might have other locking order problems -- do_thread_exit takes the
> admin lock and then the lock of a fat mutex, whereas fat_mutex_lock does
> the other way around.  (Yuk, fat mutexes.)  Perhaps not, though.

I think I fixed this one yesterday (thanks, Helgrind!).

Thanks,
Ludo’.