Subject: bug#64819: 30.0.50; condition-wait not interruptible

From: Helmut Eller <[email-masked]>
Date: Sun, 03 Sep 2023 21:53:21 +0200

On Sat, Sep 02 2023, Stefan Kangas wrote:

> Did you get any further with this?

Well, a bit.  I'm currently using the attached patch and for testing the
attached shell script.  The patch is good enough to pass these tests.  I
don't know if it breaks something else.

I had to change maybe_reacquire_global_lock to make the tests pass.  The
idea behind maybe_reacquire_global_lock seems dubious: waiting for a
lock in a signal handler seems, well, problematic.

Anyway, the patch is work in progress and only useful to me.

(The patch also includes changes for Windows, but I only tested those
manually and under Wine.  For the Windows console version, C-g doesn't
interrupt condition-wait.  Ordinary endless loops don't seem be
interruptible there either.  So, maybe this is acceptable.)

Helmut

diff --git a/src/keyboard.c b/src/keyboard.c
index 6ab1cdebc12..d8262c779c2 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -8099,6 +8099,11 @@ handle_input_available_signal (int sig)
 
   if (input_available_clear_time)
     *input_available_clear_time = make_timespec (0, 0);
+
+#ifdef THREADS_ENABLED
+  maybe_reacquire_global_lock ();
+  maybe_awake_current_thread ();
+#endif
 }
 
 static void
@@ -12076,7 +12081,10 @@ handle_interrupt (bool in_signal_handler)
      thread, see deliver_process_signal.  So we must make sure the
      main thread holds the global lock.  */
   if (in_signal_handler)
-    maybe_reacquire_global_lock ();
+    {
+      maybe_reacquire_global_lock ();
+      maybe_awake_current_thread ();
+    }
 #endif
   if (waiting_for_input && !echoing)
     quit_throw_to_read_char (in_signal_handler);
diff --git a/src/process.c b/src/process.c
index 08cb810ec13..a25071a482a 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5473,7 +5473,8 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 	update_status (wait_proc);
       if (wait_proc
 	  && ! EQ (wait_proc->status, Qrun)
-	  && ! connecting_status (wait_proc->status))
+	  && ! connecting_status (wait_proc->status)
+	  && ! EQ (wait_proc->status, Qlisten))
 	{
 	  bool read_some_bytes = false;
 
diff --git a/src/systhread.c b/src/systhread.c
index caa4dfd4443..fa652abbb30 100644
--- a/src/systhread.c
+++ b/src/systhread.c
@@ -273,6 +273,9 @@ sys_thread_yield (void)
 #include <mbctype.h>
 #include "w32term.h"
 
+/* From w32xfns.c */
+extern HANDLE interrupt_handle;
+
 /* Cannot include <process.h> because of the local header by the same
    name, sigh.  */
 uintptr_t _beginthread (void (__cdecl *)(void *), unsigned, void *);
@@ -310,7 +313,9 @@ sys_cond_init (sys_cond_t *cond)
   cond->events[CONDV_SIGNAL] = CreateEvent (NULL, FALSE, FALSE, NULL);
   /* Manual-reset event for broadcast.  */
   cond->events[CONDV_BROADCAST] = CreateEvent (NULL, TRUE, FALSE, NULL);
-  if (!cond->events[CONDV_SIGNAL] || !cond->events[CONDV_BROADCAST])
+  cond->events[CONDV_INTERRUPT] = interrupt_handle;
+  if (!cond->events[CONDV_SIGNAL] || !cond->events[CONDV_BROADCAST]
+      || !cond->events[CONDV_INTERRUPT])
     return;
   InitializeCriticalSection ((LPCRITICAL_SECTION)&cond->wait_count_lock);
   cond->initialized = true;
@@ -333,15 +338,15 @@ 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 (CONDV_MAX, cond->events, FALSE, INFINITE);
 
   /* Decrement the wait count and see if we are the last thread
      waiting on the condition variable.  */
   EnterCriticalSection ((LPCRITICAL_SECTION)&cond->wait_count_lock);
   cond->wait_count--;
-  last_thread_waiting =
-    wait_result == WAIT_OBJECT_0 + CONDV_BROADCAST
-    && cond->wait_count == 0;
+  last_thread_waiting = wait_result == WAIT_OBJECT_0 + CONDV_BROADCAST
+                        && cond->wait_count == 0;
   LeaveCriticalSection ((LPCRITICAL_SECTION)&cond->wait_count_lock);
 
   /* Broadcast uses a manual-reset event, so when the last thread is
diff --git a/src/systhread.h b/src/systhread.h
index b8f078df071..7e9ac83f951 100644
--- a/src/systhread.h
+++ b/src/systhread.h
@@ -55,7 +55,13 @@ typedef struct {
   unsigned long SpinCount;
 } w32thread_critsect;
 
-enum { CONDV_SIGNAL = 0, CONDV_BROADCAST = 1, CONDV_MAX = 2 };
+enum
+{
+  CONDV_SIGNAL = 0,
+  CONDV_BROADCAST = 1,
+  CONDV_INTERRUPT = 2,
+  CONDV_MAX = 3
+};
 
 typedef struct {
   /* Count of threads that are waiting for this condition variable.  */
diff --git a/src/thread.c b/src/thread.c
index b8ca56fd372..efe21702b5f 100644
--- a/src/thread.c
+++ b/src/thread.c
@@ -158,21 +158,46 @@ acquire_global_lock (struct thread_state *self)
 void
 maybe_reacquire_global_lock (void)
 {
+  eassert (sys_thread_equal (sys_thread_self (), main_thread.s.thread_id));
+
   /* SIGINT handler is always run on the main thread, see
      deliver_process_signal, so reflect that in our thread-tracking
      variables.  */
-  current_thread = &main_thread.s;
+  struct thread_state *self = &main_thread.s;
 
-  if (current_thread->not_holding_lock)
+  if (self->not_holding_lock)
     {
-      struct thread_state *self = current_thread;
-
       acquire_global_lock (self);
-      current_thread->not_holding_lock = 0;
+      self->not_holding_lock = 0;
+      eassert (current_thread == self);
     }
+
+  if (current_thread == NULL)
+    post_acquire_global_lock (self);
+
+  eassert (!self->not_holding_lock);
+  eassert (current_thread != NULL);
 }
 
-
+/* This is called from keyboard.c when it sets pending_signals=true.
+   If the current thread is waiting, we create a spurious wakeup by
+   broadcasting on wait_condvar.  This is necessary because
+   pthread_cond_wait may or may not return if it was interrupted by a
+   signal (SIGIO).  Without the wakeup, nobody would process a
+   potential C-g.
+*/
+void
+maybe_awake_current_thread (void)
+{
+  eassert (sys_thread_equal (sys_thread_self (), main_thread.s.thread_id));
+  eassert (!main_thread.s.not_holding_lock);
+  eassert (current_thread != NULL);
+
+  struct thread_state *t = current_thread;
+
+  if (t->wait_condvar != NULL)
+    sys_cond_broadcast (t->wait_condvar);
+}
 
 static void
 lisp_mutex_init (lisp_mutex_t *mutex)
diff --git a/src/thread.h b/src/thread.h
index 9b14cc44f35..60f601a6248 100644
--- a/src/thread.h
+++ b/src/thread.h
@@ -311,6 +311,7 @@ extern void finalize_one_thread (struct thread_state *state);
 extern void finalize_one_mutex (struct Lisp_Mutex *);
 extern void finalize_one_condvar (struct Lisp_CondVar *);
 extern void maybe_reacquire_global_lock (void);
+extern void maybe_awake_current_thread (void);
 
 extern void init_threads (void);
 extern void syms_of_threads (void);