Fix a bug in waiting for condition variable

* src/thread.c (lisp_mutex_lock, lisp_mutex_unlock)
(lisp_mutex_unlock_for_wait, condition_wait_callback)
(condition_notify_callback): Improve commentary.
(condition_wait_callback): Call post_acquire_global_lock before
attempting to lock the mutex, to make sure the lock's owner is
recorded correctly.

* test/src/thread-tests.el (threads-condvar-wait): New test.

diff --git a/src/thread.c b/src/thread.c
index 01e8aa736cef2d7730d76eed1ff72269a03acbf8..5498fe5efcbea1a4d0a6a6d72d22f7e2b7b9bd7f 100644 (file)
--- a/src/thread.c
+++ b/src/thread.c
@@ -128,6 +128,20 @@ 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.
+
+   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 another thread, this function will release
+   the global lock, giving other threads a chance to run, and will
+   wait for the MUTEX to become unlocked; when MUTEX becomes unlocked,
+   and will then re-acquire the global lock.
+
+   Return value is 1 if the function waited for the MUTEX to become
+   unlocked (meaning other threads could have run during the wait),
+   zero otherwise.  */
 static int
 lisp_mutex_lock (lisp_mutex_t *mutex, int new_count)
 {
@@ -162,6 +176,12 @@ lisp_mutex_lock (lisp_mutex_t *mutex, int new_count)
   return 1;
 }
 
+/* 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
+   function signals an error if MUTEX is locked by a thread other than
+   the current one.  Return value is 1 if the mutex becomes unlocked,
+   zero otherwise.  */
 static int
 lisp_mutex_unlock (lisp_mutex_t *mutex)
 {
@@ -177,6 +197,8 @@ lisp_mutex_unlock (lisp_mutex_t *mutex)
   return 1;
 }
 
+/* Like lisp_mutex_unlock, but sets MUTEX's lock count to zero
+   regardless of its value.  Return the previous lock count.  */
 static unsigned int
 lisp_mutex_unlock_for_wait (lisp_mutex_t *mutex)
 {
@@ -241,6 +263,10 @@ mutex_lock_callback (void *arg)
   struct Lisp_Mutex *mutex = arg;
   struct thread_state *self = current_thread;
 
+  /* 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
+     post_acquire_global_lock.  */
   if (lisp_mutex_lock (&mutex->mutex, 0))
     post_acquire_global_lock (self);
 }
@@ -280,7 +306,7 @@ mutex_unlock_callback (void *arg)
   struct thread_state *self = current_thread;
 
   if (lisp_mutex_unlock (&mutex->mutex))
-    post_acquire_global_lock (self);
+    post_acquire_global_lock (self); /* FIXME: is this call needed? */
 }
 
 DEFUN ("mutex-unlock", Fmutex_unlock, Smutex_unlock, 1, 1, 0,
@@ -367,12 +393,21 @@ condition_wait_callback (void *arg)
   if (NILP (self->error_symbol))
     {
       self->wait_condvar = &cvar->cond;
+      /* This call could switch to another thread.  */
       sys_cond_wait (&cvar->cond, &global_lock);
       self->wait_condvar = NULL;
     }
-  lisp_mutex_lock (&mutex->mutex, saved_count);
   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
+     post_acquire_global_lock.  */
+  if (lisp_mutex_lock (&mutex->mutex, saved_count))
+    post_acquire_global_lock (self);
 }
 
 DEFUN ("condition-wait", Fcondition_wait, Scondition_wait, 1, 1, 0,
@@ -425,6 +460,10 @@ condition_notify_callback (void *arg)
     sys_cond_broadcast (&na->cvar->cond);
   else
     sys_cond_signal (&na->cvar->cond);
+  /* 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
+     post_acquire_global_lock.  */
   lisp_mutex_lock (&mutex->mutex, saved_count);
   post_acquire_global_lock (self);
 }

diff --git a/test/src/thread-tests.el b/test/src/thread-tests.el
index 2e5a3bcc1fae44387d0e26bc898905c2e7e0d714..71b20185d76c5c7e44964dfe9ecdc3980baae35f 100644 (file)
--- a/test/src/thread-tests.el
+++ b/test/src/thread-tests.el
@@ -244,4 +244,45 @@
     (sit-for 1)
     (should-not (thread-alive-p thread))))
 
+(defvar threads-condvar nil)
+(defun threads-test-condvar-wait ()
+  ;; Wait for condvar to be notified
+  (mutex-lock (condition-mutex threads-condvar))
+  (condition-wait threads-condvar)
+  (mutex-unlock (condition-mutex threads-condvar))
+  ;; Wait again, it will be signaled.
+  (with-mutex (condition-mutex threads-condvar)
+    (condition-wait threads-condvar)))
+
+(ert-deftest threads-condvar-wait ()
+  "test waiting on conditional variable"
+  (let* ((cv-mutex (make-mutex))
+         (nthreads (length (all-threads)))
+         new-thread)
+    (setq threads-condvar (make-condition-variable cv-mutex))
+    (setq new-thread (make-thread #'threads-test-condvar-wait))
+    (while (not (eq (thread--blocker new-thread) threads-condvar))
+      (thread-yield))
+    (should (thread-alive-p new-thread))
+    (should (= (length (all-threads)) (1+ nthreads)))
+    ;; Notify the waiting thread.
+    (with-mutex cv-mutex
+      (condition-notify threads-condvar t))
+
+    ;; Allow new-thread to process the notification.
+    (sleep-for 0.1)
+    ;; Make sure the thread is still there.  This used to fail due to
+    ;; a bug in condition_wait_callback.
+    (should (thread-alive-p new-thread))
+    (should (= (length (all-threads)) (1+ nthreads)))
+    (should (memq new-thread (all-threads)))
+    ;; Make sure the other thread waits at the condition variable again.
+    (should (eq (thread--blocker new-thread) threads-condvar))
+
+    ;; Signal the thread.
+    (thread-signal new-thread 'error '("Die, die, die!"))
+    (sleep-for 0.1)
+    ;; Make sure the thread died.
+    (should (= (length (all-threads)) nthreads))))
+
 ;;; threads.el ends here