Fix a bug with signaling a thread that waits for condvar

* src/thread.c (lisp_mutex_lock_for_thread): New function,
with all the guts of lisp_mutex_lock.
(lisp_mutex_lock): Call lisp_mutex_lock_for_thread.
(condition_wait_callback): Don't call post_acquire_global_lock
before locking the mutex, as that could cause a signaled thread to
exit prematurely, because the condvar's mutex is recorded to be
not owned by any thread, and with-mutex wants to unlock it as part
of unwinding the stack in response to the signal.

diff --git a/src/thread.c b/src/thread.c
index 6048516659e18f4a49ee6be2f9de7fba6e04dd73..9ea7e121a82fe969a2fd0820da64c00a48ffbb16 100644 (file)
--- a/src/thread.c
+++ b/src/thread.c
@@ -128,11 +128,11 @@ lisp_mutex_init (lisp_mutex_t *mutex)
   sys_cond_init (&mutex->condition);
 }
 
-/* Lock MUTEX setting its count to COUNT, if non-zero, or to 1
-   otherwise.
+/* Lock MUTEX for thread LOCKER, setting its lock count to COUNT, if
+   non-zero, or to 1 otherwise.
 
-   If MUTEX is locked by the current thread, COUNT must be zero, and
-   the MUTEX's lock count will be incremented.
+   If MUTEX is locked by LOCKER, COUNT must be zero, and the MUTEX's
+   lock count will be incremented.
 
    If MUTEX is locked by another thread, this function will release
    the global lock, giving other threads a chance to run, and will
@@ -143,24 +143,25 @@ lisp_mutex_init (lisp_mutex_t *mutex)
    unlocked (meaning other threads could have run during the wait),
    zero otherwise.  */
 static int
-lisp_mutex_lock (lisp_mutex_t *mutex, int new_count)
+lisp_mutex_lock_for_thread (lisp_mutex_t *mutex, struct thread_state *locker,
+                           int new_count)
 {
   struct thread_state *self;
 
   if (mutex->owner == NULL)
     {
-      mutex->owner = current_thread;
+      mutex->owner = locker;
       mutex->count = new_count == 0 ? 1 : new_count;
       return 0;
     }
-  if (mutex->owner == current_thread)
+  if (mutex->owner == locker)
     {
       eassert (new_count == 0);
       ++mutex->count;
       return 0;
     }
 
-  self = current_thread;
+  self = locker;
   self->wait_condvar = &mutex->condition;
   while (mutex->owner != NULL && (new_count != 0
                                  || NILP (self->error_symbol)))
@@ -176,6 +177,12 @@ lisp_mutex_lock (lisp_mutex_t *mutex, int new_count)
   return 1;
 }
 
+static int
+lisp_mutex_lock (lisp_mutex_t *mutex, int new_count)
+{
+  return lisp_mutex_lock_for_thread (mutex, current_thread, new_count);
+}
+
 /* Decrement MUTEX's lock count.  If the lock count becomes zero after
    decrementing it, meaning the mutex is now unlocked, broadcast that
    to all the threads that might be waiting to lock the mutex.  This
@@ -398,16 +405,16 @@ condition_wait_callback (void *arg)
       self->wait_condvar = NULL;
     }
   self->event_object = Qnil;
-  /* Since sys_cond_wait could switch threads, we need to re-establish
-     ourselves as the current thread, otherwise lisp_mutex_lock will
-     record the wrong thread as the owner of the mutex lock.  */
-  post_acquire_global_lock (self);
-  /* Calling lisp_mutex_lock might yield to other threads while this
-     one waits for the mutex to become unlocked, so we need to
-     announce us as the current thread by calling
+  /* Since sys_cond_wait could switch threads, we need to lock the
+     mutex for the thread which was the current when we were called,
+     otherwise lisp_mutex_lock will record the wrong thread as the
+     owner of the mutex lock.  */
+  lisp_mutex_lock_for_thread (&mutex->mutex, self, saved_count);
+  /* Calling lisp_mutex_lock_for_thread might yield to other threads
+     while this one waits for the mutex to become unlocked, so we need
+     to announce us as the current thread by calling
      post_acquire_global_lock.  */
-  if (lisp_mutex_lock (&mutex->mutex, saved_count))
-    post_acquire_global_lock (self);
+  post_acquire_global_lock (self);
 }
 
 DEFUN ("condition-wait", Fcondition_wait, Scondition_wait, 1, 1, 0,