condition-wait considered harmful

condition-wait considered harmful

[Andreas Politz]

[-- Attachment #1: Type: text/plain, Size: 255 bytes --]


Please consider  adding a  mandatory timeout  (see attached  patch) when
calling condition-wait in the main-thread.  As it currently is, any such
call may make Emacs freeze irrecoverably.  I hope no argument is needed,
why this is a bad thing.

Andreas


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: condition_timedwait.patch --]
[-- Type: text/x-patch, Size: 6047 bytes --]

diff --git a/src/systhread.c b/src/systhread.c
index 6f4de536fb..77603fcbcd 100644
--- a/src/systhread.c
+++ b/src/systhread.c
@@ -51,7 +51,7 @@ sys_cond_init (sys_cond_t *c)
 }
 
 void
-sys_cond_wait (sys_cond_t *c, sys_mutex_t *m)
+sys_cond_wait (sys_cond_t *c, sys_mutex_t *m, int max_seconds)
 {
 }
 
@@ -158,10 +158,20 @@ sys_cond_init (sys_cond_t *cond)
 }
 
 void
-sys_cond_wait (sys_cond_t *cond, sys_mutex_t *mutex)
+sys_cond_wait (sys_cond_t *cond, sys_mutex_t *mutex, int max_seconds)
 {
-  int error = pthread_cond_wait (cond, mutex);
-  eassert (error == 0);
+  int error;
+
+  if (max_seconds < 0)
+    error = pthread_cond_wait (cond, mutex);
+  else
+    {
+      struct timespec timeout;
+      clock_gettime(CLOCK_REALTIME, &timeout);
+      timeout.tv_sec += max_seconds;
+      error = pthread_cond_timedwait (cond, mutex, &timeout);
+    }
+  eassert (error == 0 || error != ETIMEDOUT);
 }
 
 void
@@ -294,9 +304,10 @@ sys_cond_init (sys_cond_t *cond)
 }
 
 void
-sys_cond_wait (sys_cond_t *cond, sys_mutex_t *mutex)
+sys_cond_wait (sys_cond_t *cond, sys_mutex_t *mutex, int max_seconds)
 {
   DWORD wait_result;
+  DWORD max_millis = max_seconds < 0 ? INFINITE : 1000 * max_seconds;
   bool last_thread_waiting;
 
   if (!cond->initialized)
@@ -310,7 +321,7 @@ sys_cond_wait (sys_cond_t *cond, sys_mutex_t *mutex)
   /* Release the mutex and wait for either the signal or the broadcast
      event.  */
   LeaveCriticalSection ((LPCRITICAL_SECTION)mutex);
-  wait_result = WaitForMultipleObjects (2, cond->events, FALSE, INFINITE);
+  wait_result = WaitForMultipleObjects (2, cond->events, FALSE, max_millis);
 
   /* Decrement the wait count and see if we are the last thread
      waiting on the condition variable.  */
diff --git a/src/systhread.h b/src/systhread.h
index 8070bcde75..7a1ca615d7 100644
--- a/src/systhread.h
+++ b/src/systhread.h
@@ -32,6 +32,7 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #ifdef HAVE_PTHREAD
 
 #include <pthread.h>
+#include <errno.h>
 
 /* A system mutex is just a pthread mutex.  This is only used for the
    GIL.  */
@@ -102,7 +103,7 @@ extern void sys_mutex_lock (sys_mutex_t *);
 extern void sys_mutex_unlock (sys_mutex_t *);
 
 extern void sys_cond_init (sys_cond_t *);
-extern void sys_cond_wait (sys_cond_t *, sys_mutex_t *);
+extern void sys_cond_wait (sys_cond_t *, sys_mutex_t *, int);
 extern void sys_cond_signal (sys_cond_t *);
 extern void sys_cond_broadcast (sys_cond_t *);
 extern void sys_cond_destroy (sys_cond_t *);
diff --git a/src/thread.c b/src/thread.c
index e2deadd7a8..92fd6188ec 100644
--- a/src/thread.c
+++ b/src/thread.c
@@ -193,7 +193,7 @@ lisp_mutex_lock_for_thread (lisp_mutex_t *mutex, struct thread_state *locker,
   self->wait_condvar = &mutex->condition;
   while (mutex->owner != NULL && (new_count != 0
 				  || NILP (self->error_symbol)))
-    sys_cond_wait (&mutex->condition, &global_lock);
+    sys_cond_wait (&mutex->condition, &global_lock, -1);
   self->wait_condvar = NULL;
 
   if (new_count == 0 && !NILP (self->error_symbol))
@@ -404,10 +404,18 @@ informational only.  */)
   return result;
 }
 
+struct wait_args
+{
+  struct Lisp_CondVar *cvar;
+  int max_seconds;
+};
+
 static void
-condition_wait_callback (void *arg)
+condition_wait_callback (void *wait_arg)
 {
-  struct Lisp_CondVar *cvar = arg;
+  struct wait_args *arg = wait_arg;
+  struct Lisp_CondVar *cvar = arg->cvar;
+  int max_seconds = arg->max_seconds;
   struct Lisp_Mutex *mutex = XMUTEX (cvar->mutex);
   struct thread_state *self = current_thread;
   unsigned int saved_count;
@@ -421,7 +429,7 @@ condition_wait_callback (void *arg)
     {
       self->wait_condvar = &cvar->cond;
       /* This call could switch to another thread.  */
-      sys_cond_wait (&cvar->cond, &global_lock);
+      sys_cond_wait (&cvar->cond, &global_lock, max_seconds);
       self->wait_condvar = NULL;
     }
   self->event_object = Qnil;
@@ -437,9 +445,26 @@ condition_wait_callback (void *arg)
   post_acquire_global_lock (self);
 }
 
-DEFUN ("condition-wait", Fcondition_wait, Scondition_wait, 1, 1, 0,
+#define MAIN_THREAD_CONDITION_WAIT_MAX 16
+
+static int
+condition_wait_get_timeout (Lisp_Object specified)
+{
+  if (NILP (specified))
+    XSETINT(specified, -1);
+  else
+    CHECK_INTEGER (specified);
+
+  if (main_thread_p (current_thread))
+    return max (MAIN_THREAD_CONDITION_WAIT_MAX, XFIXNUM (specified));
+
+  return XFIXNUM (specified);
+}
+
+DEFUN ("condition-wait", Fcondition_wait, Scondition_wait, 1, 2, 0,
        doc: /* Wait for the condition variable COND to be notified.
-COND is the condition variable to wait on.
+COND is the condition variable to wait on.  Wait at most SECONDS
+seconds. If SECONDS is negative, wait without limit.
 
 The mutex associated with COND must be held when this is called.
 It is an error if it is not held.
@@ -448,10 +473,11 @@ This releases the mutex and waits for COND to be notified or for
 this thread to be signaled with `thread-signal'.  When
 `condition-wait' returns, COND's mutex will again be locked by
 this thread.  */)
-  (Lisp_Object cond)
+  (Lisp_Object cond, Lisp_Object seconds)
 {
   struct Lisp_CondVar *cvar;
   struct Lisp_Mutex *mutex;
+  struct wait_args args;
 
   CHECK_CONDVAR (cond);
   cvar = XCONDVAR (cond);
@@ -460,7 +486,9 @@ this thread.  */)
   if (!lisp_mutex_owned_p (&mutex->mutex))
     error ("Condition variable's mutex is not held by current thread");
 
-  flush_stack_call_func (condition_wait_callback, cvar);
+  args.cvar = cvar;
+  args.max_seconds = condition_wait_get_timeout (seconds);
+  flush_stack_call_func (condition_wait_callback, &args);
 
   return Qnil;
 }
@@ -961,7 +989,7 @@ thread_join_callback (void *arg)
   self->event_object = thread;
   self->wait_condvar = &tstate->thread_condvar;
   while (thread_live_p (tstate) && NILP (self->error_symbol))
-    sys_cond_wait (self->wait_condvar, &global_lock);
+    sys_cond_wait (self->wait_condvar, &global_lock, -1);
 
   self->wait_condvar = NULL;
   self->event_object = Qnil;