[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Emacs-diffs] master 03e4ab0: Fix a bug in waiting for condition variabl
From: |
Eli Zaretskii |
Subject: |
[Emacs-diffs] master 03e4ab0: Fix a bug in waiting for condition variable |
Date: |
Fri, 13 Jan 2017 09:50:03 +0000 (UTC) |
branch: master
commit 03e4ab0d586069be65e4a17fbf4cd965a9984726
Author: Eli Zaretskii <address@hidden>
Commit: Eli Zaretskii <address@hidden>
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.
---
src/thread.c | 43 +++++++++++++++++++++++++++++++++++++++++--
test/src/thread-tests.el | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 82 insertions(+), 2 deletions(-)
diff --git a/src/thread.c b/src/thread.c
index 01e8aa7..5498fe5 100644
--- 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 2e5a3bc..71b2018 100644
--- 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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- [Emacs-diffs] master 03e4ab0: Fix a bug in waiting for condition variable,
Eli Zaretskii <=