all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#64819: 30.0.50; condition-wait not interruptible
@ 2023-07-24  6:32 Helmut Eller
  2023-07-24 12:10 ` Eli Zaretskii
  0 siblings, 1 reply; 12+ messages in thread
From: Helmut Eller @ 2023-07-24  6:32 UTC (permalink / raw)
  To: 64819

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

When Emacs is blocked in condition-wait, then C-g doesn't seem to work.
E.g. Emacs hangs and pressing C-g has no apparent effect when executing
the attached file with:

  emacs -Q -l deadlock.el -f deadlock

Mabye sys_cond_wait could call pthread_cond_timedwait with a fairly long
timeout, say one second, and repeatedly check if C-g was pressed.


[-- Attachment #2: deadlock.el --]
[-- Type: application/emacs-lisp, Size: 170 bytes --]

[-- Attachment #3: Type: text/plain, Size: 450 bytes --]




Configured using:
 'configure --with-xpm=ifavailable --with-jpeg=ifavailable
 --with-gif=ifavailable --with-tiff=ifavailable'

Configured features:
CAIRO DBUS FREETYPE GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG LIBSELINUX
LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG SECCOMP SOUND
SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XINPUT2 GTK3
ZLIB

Important settings:
  value of $LANG: C.UTF-8
  locale-coding-system: utf-8-unix


^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#64819: 30.0.50; condition-wait not interruptible
  2023-07-24  6:32 bug#64819: 30.0.50; condition-wait not interruptible Helmut Eller
@ 2023-07-24 12:10 ` Eli Zaretskii
  2023-07-24 12:58   ` Helmut Eller
  0 siblings, 1 reply; 12+ messages in thread
From: Eli Zaretskii @ 2023-07-24 12:10 UTC (permalink / raw)
  To: Helmut Eller; +Cc: 64819

> From: Helmut Eller <eller.helmut@gmail.com>
> Date: Mon, 24 Jul 2023 08:32:04 +0200
> 
> When Emacs is blocked in condition-wait, then C-g doesn't seem to work.
> E.g. Emacs hangs and pressing C-g has no apparent effect when executing
> the attached file with:
> 
>   emacs -Q -l deadlock.el -f deadlock
> 
> Mabye sys_cond_wait could call pthread_cond_timedwait with a fairly long
> timeout, say one second, and repeatedly check if C-g was pressed.

How will we know what to do with C-g if some thread runs Lisp, while
one or more other threads are stuck in condition-wait? wouldn't you
expect in this case to have C-g go to the running thread?





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#64819: 30.0.50; condition-wait not interruptible
  2023-07-24 12:10 ` Eli Zaretskii
@ 2023-07-24 12:58   ` Helmut Eller
  2023-07-24 13:34     ` Eli Zaretskii
  0 siblings, 1 reply; 12+ messages in thread
From: Helmut Eller @ 2023-07-24 12:58 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 64819

On Mon, Jul 24 2023, Eli Zaretskii wrote:

> How will we know what to do with C-g if some thread runs Lisp, while
> one or more other threads are stuck in condition-wait? wouldn't you
> expect in this case to have C-g go to the running thread?

We could say that C-g sets quit-flag and causes all blocking calls to
condition-wait to return nil (spurious wakeup).  At that point all
threads are conceptually running.  Then the first thread (unspecified
which one) who calls maybe_quit() finishes handling C-g and clears
quit-flag afterwards.  Those threads who don't feel prepared to handle
C-g can bind inhibit-quit.

Helmut





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#64819: 30.0.50; condition-wait not interruptible
  2023-07-24 12:58   ` Helmut Eller
@ 2023-07-24 13:34     ` Eli Zaretskii
  2023-07-24 14:57       ` Helmut Eller
  0 siblings, 1 reply; 12+ messages in thread
From: Eli Zaretskii @ 2023-07-24 13:34 UTC (permalink / raw)
  To: Helmut Eller; +Cc: 64819

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: 64819@debbugs.gnu.org
> Date: Mon, 24 Jul 2023 14:58:37 +0200
> 
> On Mon, Jul 24 2023, Eli Zaretskii wrote:
> 
> > How will we know what to do with C-g if some thread runs Lisp, while
> > one or more other threads are stuck in condition-wait? wouldn't you
> > expect in this case to have C-g go to the running thread?
> 
> We could say that C-g sets quit-flag and causes all blocking calls to
> condition-wait to return nil (spurious wakeup).  At that point all
> threads are conceptually running.  Then the first thread (unspecified
> which one) who calls maybe_quit() finishes handling C-g and clears
> quit-flag afterwards.  Those threads who don't feel prepared to handle
> C-g can bind inhibit-quit.

I don't think we can allow more than one thread at a time to run the
parts of the Lisp interpreter that lead to maybe_quit.

Also, I don't think what you describe, even if it were possible, is
what users expect: they expect that the thread which is running is
interrupted, and either exits or handles the quit, and all the other
threads still wait for the condition var.

So I think to do anything smarter in the deadlock situation you
describe we'd need to detect the deadlock first.  Once we do that
(which isn't easy: perhaps do that in the signal handler?), we'd need
to decide which of the deadlocked threads to free, which is also not
trivial.  Hmmm...

Btw, did you try your recipe in a Unix TTY?  There, C-g actually
delivers SIGINT to Emacs, so you might see a different behavior (or a
crash ;-).





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#64819: 30.0.50; condition-wait not interruptible
  2023-07-24 13:34     ` Eli Zaretskii
@ 2023-07-24 14:57       ` Helmut Eller
  2023-07-24 16:23         ` Eli Zaretskii
  0 siblings, 1 reply; 12+ messages in thread
From: Helmut Eller @ 2023-07-24 14:57 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 64819

On Mon, Jul 24 2023, Eli Zaretskii wrote:

>> From: Helmut Eller <eller.helmut@gmail.com>
>> We could say that C-g sets quit-flag and causes all blocking calls to
>> condition-wait to return nil (spurious wakeup).  At that point all
>> threads are conceptually running.  Then the first thread (unspecified
>> which one) who calls maybe_quit() finishes handling C-g and clears
>> quit-flag afterwards.  Those threads who don't feel prepared to handle
>> C-g can bind inhibit-quit.
>
> I don't think we can allow more than one thread at a time to run the
> parts of the Lisp interpreter that lead to maybe_quit.

I didn't suggest that.  Nor did I suggest that the thread scheduler
should switch away from the currently running thread.

What I did suggest is that the thread blocked in condition-wait is
considered runnable.  So that the thread scheduler is allowed to pick
this thread the next time when somebody calls thread-yield or
condition-wait.

To the thread it will look like a spurious wakeup (i.e. condition-wait
returned but the condition isn't actually true) but Lisp code must
already be prepared for such a situation.

The bytecode interpreter calls maybe_quit before every call or backward
branch, so maybe_quit will be called very soon after the spurious
wakeup.

> Also, I don't think what you describe, even if it were possible, is
> what users expect: they expect that the thread which is running is
> interrupted, and either exits or handles the quit, and all the other
> threads still wait for the condition var.

Maybe we can agree on this: when only one thread exists and it is
blocked in condition-wait, then condition-wait should be interruptible
by C-g.

For the situation where some threads are blocked in condition-wait and
one other thread is running, I think that running thread would call
maybe_quit and clear quite-flag before calling thread-yield.  The other
threads would observe spurious wakeups as soon as they are allowed to
run.

> So I think to do anything smarter in the deadlock situation you
> describe we'd need to detect the deadlock first.  Once we do that
> (which isn't easy: perhaps do that in the signal handler?), we'd need
> to decide which of the deadlocked threads to free, which is also not
> trivial.  Hmmm...

I doubt that deadlock detection is possible in the general case.
E.g. how could we possibly know that a timer is or isn't going to call
condition-notify in 5 seconds?

> Btw, did you try your recipe in a Unix TTY?  There, C-g actually
> delivers SIGINT to Emacs, so you might see a different behavior (or a
> crash ;-).

When I run the recipe with: "emacs -nw -l deadlock.el -f deadlock" then
I see the emergency escape feature kick in.  Only after the second C-g
(of course).  A single C-g doesn't seem to do anything.

Helmut





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#64819: 30.0.50; condition-wait not interruptible
  2023-07-24 14:57       ` Helmut Eller
@ 2023-07-24 16:23         ` Eli Zaretskii
  2023-07-25  8:06           ` Helmut Eller
  0 siblings, 1 reply; 12+ messages in thread
From: Eli Zaretskii @ 2023-07-24 16:23 UTC (permalink / raw)
  To: Helmut Eller; +Cc: 64819

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: 64819@debbugs.gnu.org
> Date: Mon, 24 Jul 2023 16:57:05 +0200
> 
> On Mon, Jul 24 2023, Eli Zaretskii wrote:
> 
> >> From: Helmut Eller <eller.helmut@gmail.com>
> >> We could say that C-g sets quit-flag and causes all blocking calls to
> >> condition-wait to return nil (spurious wakeup).  At that point all
> >> threads are conceptually running.  Then the first thread (unspecified
> >> which one) who calls maybe_quit() finishes handling C-g and clears
> >> quit-flag afterwards.  Those threads who don't feel prepared to handle
> >> C-g can bind inhibit-quit.
> >
> > I don't think we can allow more than one thread at a time to run the
> > parts of the Lisp interpreter that lead to maybe_quit.
> 
> I didn't suggest that.  Nor did I suggest that the thread scheduler
> should switch away from the currently running thread.

You did say "At that point all threads are conceptually running."
Perhaps I misunderstood what you meant.

> What I did suggest is that the thread blocked in condition-wait is
> considered runnable.  So that the thread scheduler is allowed to pick
> this thread the next time when somebody calls thread-yield or
> condition-wait.

We don't have a scheduler.  The threads "schedule themselves", in the
sense that the first thread that succeeds to grab the global lock gets
to run, and the others wait for the lock to be released.

So for this to work, the C-g handler will have to release some thread.

> Maybe we can agree on this: when only one thread exists and it is
> blocked in condition-wait, then condition-wait should be interruptible
> by C-g.

Yes, this is simpler.

> For the situation where some threads are blocked in condition-wait and
> one other thread is running, I think that running thread would call
> maybe_quit and clear quite-flag before calling thread-yield.  The other
> threads would observe spurious wakeups as soon as they are allowed to
> run.

I'm not sure I follow here.  First, no one guarantees that a running
thread will call thread-yield; it could call sit-for or somesuch
instead.  And second if C-g only sets quit-flag, then it will not be
able to release a stuck thread; and if it does release a stuck thread,
it will somehow need to stop the running one, or we will have more
than one thread running.

> > So I think to do anything smarter in the deadlock situation you
> > describe we'd need to detect the deadlock first.  Once we do that
> > (which isn't easy: perhaps do that in the signal handler?), we'd need
> > to decide which of the deadlocked threads to free, which is also not
> > trivial.  Hmmm...
> 
> I doubt that deadlock detection is possible in the general case.
> E.g. how could we possibly know that a timer is or isn't going to call
> condition-notify in 5 seconds?

We are talking about a situation where the user typed C-g, so we have
some indication that the situation is not normal.

> > Btw, did you try your recipe in a Unix TTY?  There, C-g actually
> > delivers SIGINT to Emacs, so you might see a different behavior (or a
> > crash ;-).
> 
> When I run the recipe with: "emacs -nw -l deadlock.el -f deadlock" then
> I see the emergency escape feature kick in.  Only after the second C-g
> (of course).  A single C-g doesn't seem to do anything.

As expected, thanks.





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#64819: 30.0.50; condition-wait not interruptible
  2023-07-24 16:23         ` Eli Zaretskii
@ 2023-07-25  8:06           ` Helmut Eller
  2023-07-25 12:18             ` Eli Zaretskii
  0 siblings, 1 reply; 12+ messages in thread
From: Helmut Eller @ 2023-07-25  8:06 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 64819

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


On Mon, Jul 24 2023, Eli Zaretskii wrote:

[...]
> So for this to work, the C-g handler will have to release some thread.

Here is a patch that works good enough for me:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-condition-wait-interruptible-by-C-g.patch --]
[-- Type: text/x-diff, Size: 3180 bytes --]

From 4de3198c10c4efaeaffdf43ba5e5b0f1729a7f09 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Tue, 25 Jul 2023 10:03:53 +0200
Subject: [PATCH] Make condition-wait interruptible by C-g

Code like

  (let* ((mutex (make-mutex))
         (cvar (make-condition-variable mutex)))
    (with-mutex mutex
      (condition-wait cvar)))

will block in pthread_cond_wait.  The problem is that
pthread_cond_wait may or may not return when it gets interrupted
by a signal (SIGIO).  On Linux it doesn't return and so even if a
signal handler sets pending_signals=true nobody processes those
pending signals.

The patch modifies the signal handler so that it will force a
spurious wakeup when the current thread is blocked in
condition-wait.

* src/keyboard.c (handle_input_available_signal)
(handle_interrupt): Call maybe_awake_current_thread.
* src/thread.c (maybe_awake_current_thread): New.
* src/thread.h (maybe_awake_current_thread): New prototype.
---
 src/keyboard.c |  8 +++++++-
 src/thread.c   | 16 ++++++++++++++++
 src/thread.h   |  1 +
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index 41cda2e65de..f45bafa96c0 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -7745,6 +7745,10 @@ handle_input_available_signal (int sig)
 
   if (input_available_clear_time)
     *input_available_clear_time = make_timespec (0, 0);
+
+#ifdef THREADS_ENABLED
+  maybe_awake_current_thread ();
+#endif
 }
 
 static void
@@ -11556,8 +11560,10 @@ handle_interrupt (bool in_signal_handler)
   /* If we were called from a signal handler, we must be in the main
      thread, see deliver_process_signal.  So we must make sure the
      main thread holds the global lock.  */
-  if (in_signal_handler)
+  if (in_signal_handler) {
     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/thread.c b/src/thread.c
index b8ca56fd372..0bd949f5779 100644
--- a/src/thread.c
+++ b/src/thread.c
@@ -172,6 +172,22 @@ maybe_reacquire_global_lock (void)
     }
 }
 
+/* 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)
+{
+  if (current_thread->wait_condvar != NULL)
+    {
+      sys_cond_broadcast (current_thread->wait_condvar);
+    }
+}
+
 \f
 
 static void
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);
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#64819: 30.0.50; condition-wait not interruptible
  2023-07-25  8:06           ` Helmut Eller
@ 2023-07-25 12:18             ` Eli Zaretskii
  2023-07-25 12:59               ` Helmut Eller
  0 siblings, 1 reply; 12+ messages in thread
From: Eli Zaretskii @ 2023-07-25 12:18 UTC (permalink / raw)
  To: Helmut Eller; +Cc: 64819

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: 64819@debbugs.gnu.org
> Date: Tue, 25 Jul 2023 10:06:40 +0200
> 
> On Mon, Jul 24 2023, Eli Zaretskii wrote:
> 
> [...]
> > So for this to work, the C-g handler will have to release some thread.
> 
> Here is a patch that works good enough for me:

Thanks.  If you are currently working on or using some packages that
use Lisp threads, please run with this patch for a while and come back
in a couple of weeks if you see no problems with it; then we can
install this on master.  (It will need a small addition for
MS-Windows, but that can be handled later.)  A test for this, if
possible, would also be appreciated, even if it can only be run
manually (we have the test/manual/ directory for those).

If you do not intend to use threads any time soon, I guess we can
install this on master and let someone else be the guinea pig...

A couple of minor stylistic comments:

> -  if (in_signal_handler)
> +  if (in_signal_handler) {
>      maybe_reacquire_global_lock ();
> +    maybe_awake_current_thread();
> +  }

Please use the GNU style of braces:

  if (something)
    {
      do_1;
      do_2;
    }

> +void
> +maybe_awake_current_thread (void)
> +{
> +  if (current_thread->wait_condvar != NULL)
> +    {
> +      sys_cond_broadcast (current_thread->wait_condvar);
> +    }

We don't use braces when the body of 'if' has just one statement.





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#64819: 30.0.50; condition-wait not interruptible
  2023-07-25 12:18             ` Eli Zaretskii
@ 2023-07-25 12:59               ` Helmut Eller
  2023-09-02 21:58                 ` Stefan Kangas
  0 siblings, 1 reply; 12+ messages in thread
From: Helmut Eller @ 2023-07-25 12:59 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 64819

On Tue, Jul 25 2023, Eli Zaretskii wrote:

> Thanks.  If you are currently working on or using some packages that
> use Lisp threads, please run with this patch for a while and come back
> in a couple of weeks if you see no problems with it; then we can
> install this on master.  (It will need a small addition for
> MS-Windows, but that can be handled later.)  A test for this, if
> possible, would also be appreciated, even if it can only be run
> manually (we have the test/manual/ directory for those).

I will try to write a simple multi-threaded HTTP client or proxy to
exercise the threading primitives a bit.

> If you do not intend to use threads any time soon, I guess we can
> install this on master and let someone else be the guinea pig...

The patch doesn't handle the situation where current_thread==NULL.  Not
sure how that can happen, but the patch is definitely not ready for
master.

Helmut





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#64819: 30.0.50; condition-wait not interruptible
  2023-07-25 12:59               ` Helmut Eller
@ 2023-09-02 21:58                 ` Stefan Kangas
  2023-09-03 19:53                   ` Helmut Eller
  0 siblings, 1 reply; 12+ messages in thread
From: Stefan Kangas @ 2023-09-02 21:58 UTC (permalink / raw)
  To: Helmut Eller, Eli Zaretskii; +Cc: 64819

Helmut Eller <eller.helmut@gmail.com> writes:

> On Tue, Jul 25 2023, Eli Zaretskii wrote:
>
>> Thanks.  If you are currently working on or using some packages that
>> use Lisp threads, please run with this patch for a while and come back
>> in a couple of weeks if you see no problems with it; then we can
>> install this on master.  (It will need a small addition for
>> MS-Windows, but that can be handled later.)  A test for this, if
>> possible, would also be appreciated, even if it can only be run
>> manually (we have the test/manual/ directory for those).
>
> I will try to write a simple multi-threaded HTTP client or proxy to
> exercise the threading primitives a bit.
>
>> If you do not intend to use threads any time soon, I guess we can
>> install this on master and let someone else be the guinea pig...
>
> The patch doesn't handle the situation where current_thread==NULL.  Not
> sure how that can happen, but the patch is definitely not ready for
> master.

Did you get any further with this?





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#64819: 30.0.50; condition-wait not interruptible
  2023-09-02 21:58                 ` Stefan Kangas
@ 2023-09-03 19:53                   ` Helmut Eller
  2023-09-06  9:35                     ` Stefan Kangas
  0 siblings, 1 reply; 12+ messages in thread
From: Helmut Eller @ 2023-09-03 19:53 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: Eli Zaretskii, 64819

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

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: interruptible-condition-wait.patch --]
[-- Type: text/x-diff, Size: 6004 bytes --]

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);
 }
 
-\f
+/* 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);

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: cnd-wait-test.sh --]
[-- Type: text/x-sh, Size: 1627 bytes --]

#!/bin/bash

# Test whether condition-wait is interruptible by C-g.
#
# This script requires two tools: xdotool and dotool.
#
# xdotool: is included in Debian
#
# dotool: I installed it manually from https://git.sr.ht/~geb/dotool.
# As dotool requires permissions to access /dev/input, I use sudo to
# run it.
#
# The script output follows TAP syntax (https://testanything.org/) and
# could be parsed for example with prove(1).

set -o errexit

# Ask sudo password now and remember it for later
sudo dotool --version >/dev/null

TMPFILE=$(mktemp)

TESTNR=0

function check {
  local PROG="$1" FUN="$2" READYMSG="$3"
  $PROG -Q -l wait.el --eval "($FUN \"$TMPFILE\")" &
  local EMACSPID=$!
  trap "rm -f $TMPFILE; kill $EMACSPID 2>/dev/null || :" EXIT  

  # Wait until Emacs is ready
  until [ "$(cat $TMPFILE)" == "$READYMSG" ] ; do
    sleep 0.2
  done

  # Set focus on Emacs
  xdotool search --sync --onlyvisible --all \
	  --pid $EMACSPID --name '' windowfocus

  # Send C-g to input device
  echo 'key ctrl+g' | sudo dotool

  wait $EMACSPID

  DESC="$TESTNR - $PROG $FUN"
  case "$(cat $TMPFILE)" in
    QUIT | QUIT1 | QUIT2)
      echo ok $DESC;;
    *)
      cat $TMPFILE >2
      echo not ok $DESC ;;
  esac
}

declare -a PROGS
PROGS=(emacs 'xterm -e emacs -nw')

declare -A READYMSGS
READYMSGS=(
  [wait-forever]=WAITING 
  [spawn-child-then-wait]='WAITING1 TERMINATING2'
  [two-threads-waiting]='WAITING1 WAITING2'
)

echo 1..$((${#PROGS[@]} * ${#READYMSGS[@]}))
for prog in "${PROGS[@]}" ; do
  for fun in "${!READYMSGS[@]}" ; do
    TESTNR=$(($TESTNR + 1))
    check "$prog" $fun "${READYMSGS[$fun]}"
  done
done

[-- Attachment #4: wait.el --]
[-- Type: text/plain, Size: 1483 bytes --]

;; -*- lexical-binding:t -*-

(defun wait-forever (filename)
  (unwind-protect
      (condition-case e
	  (let* ((mutex (make-mutex))
		 (cvar (make-condition-variable mutex)))
	    (with-mutex mutex
	      (write-region "WAITING" nil filename nil 0)
	      (while t
		(condition-wait cvar))))
	(quit
	 (write-region "QUIT" nil filename nil 0)))
    (kill-emacs)))

(defun spawn-child-then-wait (filename)
  (make-thread (lambda ()
		 (write-region " TERMINATING2" nil filename t 0)))
  (unwind-protect
      (condition-case e
	  (let* ((mutex (make-mutex))
		 (cvar (make-condition-variable mutex)))
	    (with-mutex mutex
	      (write-region "WAITING1" nil filename nil 0)
	      (while t
		(condition-wait cvar))))
	(quit
	 (write-region "QUIT1" nil filename nil 0)))
    (kill-emacs)))

(defun two-threads-waiting (filename)
  (make-thread
   (lambda ()
     (unwind-protect
	 (condition-case e
	     (let* ((mutex (make-mutex))
		    (cvar (make-condition-variable mutex)))
	       (with-mutex mutex
		 (write-region " WAITING2" nil filename t 0)
		 (while t
		   (condition-wait cvar))))
	   (quit
	    (write-region "QUIT2" nil filename nil 0)))
       (kill-emacs))))
  (unwind-protect
      (condition-case e
	  (let* ((mutex (make-mutex))
		 (cvar (make-condition-variable mutex)))
	    (with-mutex mutex
	      (write-region "WAITING1" nil filename nil 0)
	      (while t
		(condition-wait cvar))))
	(quit
	 (write-region " QUIT1" nil filename t 0)))
    (kill-emacs)))


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#64819: 30.0.50; condition-wait not interruptible
  2023-09-03 19:53                   ` Helmut Eller
@ 2023-09-06  9:35                     ` Stefan Kangas
  0 siblings, 0 replies; 12+ messages in thread
From: Stefan Kangas @ 2023-09-06  9:35 UTC (permalink / raw)
  To: Helmut Eller; +Cc: Eli Zaretskii, 64819

Helmut Eller <eller.helmut@gmail.com> writes:

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

Thanks, it's good to see that you're still working on this.  Please do
let us know when you have something that you feel is closer to being
ready for install.

I'll leave commenting on your patch to other people, as I'm not familiar
with this code.





^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-09-06  9:35 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-24  6:32 bug#64819: 30.0.50; condition-wait not interruptible Helmut Eller
2023-07-24 12:10 ` Eli Zaretskii
2023-07-24 12:58   ` Helmut Eller
2023-07-24 13:34     ` Eli Zaretskii
2023-07-24 14:57       ` Helmut Eller
2023-07-24 16:23         ` Eli Zaretskii
2023-07-25  8:06           ` Helmut Eller
2023-07-25 12:18             ` Eli Zaretskii
2023-07-25 12:59               ` Helmut Eller
2023-09-02 21:58                 ` Stefan Kangas
2023-09-03 19:53                   ` Helmut Eller
2023-09-06  9:35                     ` Stefan Kangas

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.