Fix hang in srfi-18.test
authorNeil Jerram <neil@ossau.uklinux.net>
Wed, 22 Oct 2008 07:45:42 +0000 (08:45 +0100)
committerNeil Jerram <neil@ossau.uklinux.net>
Fri, 24 Oct 2008 20:51:47 +0000 (21:51 +0100)
commitd2a510879be656b278a58e8110e4f222d0845847
tree325ff23d94f216d275b8865a05f318988fba4abe
parentd8b6e19181ffaf2df29431166cc4ca64c1390fc8
Fix hang in srfi-18.test

* libguile/threads.h (held_mutex): New field.

* libguile/threads.c (enqueue, remqueue, dequeue): Use critical
section to protect access to the queue.
(guilify_self_1): Initialize held_mutex field.
(on_thread_exit): If held_mutex non-null, unlock it.
(fat_mutex_unlock, fat_cond_free, scm_make_condition_variable,
fat_cond_signal, fat_cond_broadcast): Delete now unnecessary uses
of c->lock.
(fat_mutex_unlock): Pass m->lock to block_self() instead of
c->lock; move scm_i_pthread_mutex_unlock(m->lock) call from before
block_self() to after.
(scm_pthread_cond_wait, scm_pthread_cond_timedwait,
scm_i_thread_sleep_for_gc): Set held_mutex before pthread call;
reset it afterwards.

I was seeing a hang in srfi-18.test, when running make check in master,
in the "exception handler installation is thread-safe" test.  It wasn't
100% reproducible, so looked like a race.

The problem is that wait-condition-variable is not actually
atomic in the way that it is supposed to be.  It unlocks the mutex,
then starts waiting on the cond var.  So it is possible for another
thread to lock the same mutex, and signal the cond var, before the
wait-condition-variable thread starts waiting.

In order for wait-condition-variable to be atomic - e.g. in a race
where thread A holds (Scheme-level) mutex M, and calls
(wait-condition-variable C M), and thread B calls (begin (lock-mutex
M) (signal-condition-variable C)) - it needs to call pthread_cond_wait
with the same underlying mutex as is involved in the `lock-mutex'
call.  In terms of the threads.c code, this means that it has to use
M->lock, not C->lock.

block_self() used its mutex arg for two purposes: for protecting
access and changes to the wait queue, and for the pthread_cond_wait
call.  But it wouldn't work reliably to use M->lock to protect C's
wait queue, because in theory two threads can call
(wait-condition-variable C M1) and (wait-condition-variable C M2)
concurrently, with M1 and M2 different.  So we either have to pass
both C->lock and M->lock into block_self(), or use some other mutex to
protect the wait queue.  For this patch, I switched to using the
critical section mutex, because that is a global and so easily
available.  (If that turns out to be a problem for performance, we
could make each queue structure have its own mutex, but there's no
reason to believe yet that it is a problem, because the critical
section mutex isn't used much overall.)

So then we call block_self() with M->lock, and move where M->lock is
unlocked to after the block_self() call, instead of before.

That solves the first hang, but introduces a new one, when a SRFI-18
thread is terminated (`thread-terminate!') between being launched
(`make-thread') and started (`thread-start!').  The problem now is
that pthread_cond_wait is a cancellation point (see man
pthread_cancel), so the pthread_cond_wait call is one of the few
places where a thread-terminate! call can take effect.  If the thread
is cancelled at that point, M->lock ends up still being locked, and
then when do_thread_exit() tries to lock M->lock again, it hangs.

The fix for that is a new `held_mutex' field in scm_i_thread, which is
set to point to the mutex just before a pthread_cond_(timed)wait call,
and set to NULL again afterwards.  If on_thread_exit() finds that
held_mutex is non-NULL, it unlocks that mutex.

A detail is that checking and unlocking held_mutex must be done before
on_thread_exit() calls scm_i_ensure_signal_delivery_thread(), because
the innards of scm_i_ensure_signal_delivery_thread() can do another
pthread_cond_wait() call and so overwrite held_mutex.  But that's OK,
because it's fine for the mutex check and unlock to happen outside
Guile mode.

Lastly, C->lock is then not needed, so I've removed it.
libguile/threads.c
libguile/threads.h