Delivering SIGIO when several threads are active?

Delivering SIGIO when several threads are active?

[Eli Zaretskii]

When SIGIO is received by some thread, we currently deliver it to the
main thread, see deliver_input_available_signal and
deliver_process_signal.  Is that appropriate when more than one thread
is active?  The SIGIO handler just sets a global variable, so the
handler itself is okay, I think, but is it okay to interrupt the main
thread with SIGIO at some arbitrary point?  E.g., the main thread
could be in some Xlib or GTK call.

The comments to deliver_process_signal say:

                 POSIX says any thread can receive a signal that is
   associated with a process, process group, or asynchronous event.
   On GNU/Linux that is not true, but for other systems (FreeBSD at
   least) it is.

What does happen on GNU/Linux?  Is there a specific thread on
GNU/Linux which gets signals associated with a process?  And if so,
does this affect what we should do with SIGIO in this case?

Thanks.

Re: Delivering SIGIO when several threads are active?

[Paul Eggert]

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

Eli Zaretskii wrote:
> When SIGIO is received by some thread, we currently deliver it to the
> main thread, see deliver_input_available_signal and
> deliver_process_signal.  Is that appropriate when more than one thread
> is active?

Such signals have been delivered to the main thread for quite some time, and 
this has worked well enough; is there something different about the concurrency 
code that now makes this behavior inappropriate?

> The SIGIO handler just sets a global variable, so the
> handler itself is okay, I think, but is it okay to interrupt the main
> thread with SIGIO at some arbitrary point?  E.g., the main thread
> could be in some Xlib or GTK call.

These library functions are all supposed to work even if such handlers interrupt 
them.

> The comments to deliver_process_signal say:
>
>                  POSIX says any thread can receive a signal that is
>    associated with a process, process group, or asynchronous event.
>    On GNU/Linux that is not true, but for other systems (FreeBSD at
>    least) it is.
>
> What does happen on GNU/Linux?  Is there a specific thread on
> GNU/Linux which gets signals associated with a process?

On GNU/Linux, process signals get sent to the main thread unless the main thread 
is blocking them (or if a few less-common situations arise, e.g., the main 
thread is exiting).

> And if so,
> does this affect what we should do with SIGIO in this case?

I don't see why it would affect SIGIO handling.

I installed the attached to try to explain this better.

Hmm, I see that the recent changes use mixed terminology. Until now, Emacs has 
called the initial thread the "main thread", but the recent code sometimes calls 
it the "primary thread". We shouldn't have two names for the same thing. I'll 
look into fixing that.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-src-sysdep.c-deliver_process_signal-Improve-comment.patch --]
[-- Type: text/x-diff; name="0001-src-sysdep.c-deliver_process_signal-Improve-comment.patch", Size: 1418 bytes --]

From c971e9141d401a5cf4aae96d981d8eb8a9300ec2 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 30 Dec 2016 09:45:46 -0800
Subject: [PATCH] * src/sysdep.c (deliver_process_signal): Improve comment.

---
 src/sysdep.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/sysdep.c b/src/sysdep.c
index 86d420f..1ba336e 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -1616,14 +1616,17 @@ static pthread_t main_thread;
 #endif
 
 /* SIG has arrived at the current process.  Deliver it to the main
-   thread, which should handle it with HANDLER.
+   thread, which should handle it with HANDLER.  (Delivering the
+   signal to some other thread might not work if the other thread is
+   about to exit.)
 
    If we are on the main thread, handle the signal SIG with HANDLER.
    Otherwise, redirect the signal to the main thread, blocking it from
    this thread.  POSIX says any thread can receive a signal that is
    associated with a process, process group, or asynchronous event.
-   On GNU/Linux that is not true, but for other systems (FreeBSD at
-   least) it is.  */
+   On GNU/Linux the main thread typically gets a process signal unless
+   it's blocked, but other systems (FreeBSD at least) can deliver the
+   signal to other threads.  */
 void
 deliver_process_signal (int sig, signal_handler_t handler)
 {
-- 
2.7.4

Re: Delivering SIGIO when several threads are active?

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 30 Dec 2016 09:54:01 -0800
> 
> Eli Zaretskii wrote:
> > When SIGIO is received by some thread, we currently deliver it to the
> > main thread, see deliver_input_available_signal and
> > deliver_process_signal.  Is that appropriate when more than one thread
> > is active?
> 
> Such signals have been delivered to the main thread for quite some time, and 
> this has worked well enough; is there something different about the concurrency 
> code that now makes this behavior inappropriate?

I don't know, that's why I asked.  For example, what if another thread
is waiting on the keyboard descriptor and wants to handle keyboard
input, or wants to receive X events?

> On GNU/Linux, process signals get sent to the main thread unless the main thread 
> is blocking them (or if a few less-common situations arise, e.g., the main 
> thread is exiting).

And what happens in those exceptional cases?

> Hmm, I see that the recent changes use mixed terminology. Until now, Emacs has 
> called the initial thread the "main thread", but the recent code sometimes calls 
> it the "primary thread". We shouldn't have two names for the same thing. I'll 
> look into fixing that.

"Primary thread" is the name of the main thread.

Re: Delivering SIGIO when several threads are active?

[Paul Eggert]

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

Eli Zaretskii wrote:
> what if another thread
> is waiting on the keyboard descriptor and wants to handle keyboard
> input, or wants to receive X events?

That depends on what is meant by "waiting on the keyboard descriptor" and "wants 
to receive X events". At the low level, if the other thread was not involved in 
delivering the signal and is calling pselect on the keyboard descriptor while 
keyboard input occurs, pselect will return successfully without being 
interrupted. Similarly for X events, I assume.

>> On GNU/Linux, process signals get sent to the main thread unless the main thread
>> is blocking them (or if a few less-common situations arise, e.g., the main
>> thread is exiting).
> And what happens in those exceptional cases?

Typically the signal gets sent to some other thread. (There are exceptions to 
this, too; it's complicated.)

>> Hmm, I see that the recent changes use mixed terminology. Until now, Emacs has
>> called the initial thread the "main thread", but the recent code sometimes calls
>> it the "primary thread". We shouldn't have two names for the same thing. I'll
>> look into fixing that.
> "Primary thread" is the name of the main thread.

How so? Threads don't have names in Emacs.

In POSIX it's called the "initial thread". Emacs has long used "main thread" 
internally to describe the same thing; this is a common alternative name, and 
it's shorter so it's OK as long as we're consistent about it. Using a *third* 
phrase "primary thread" for the same thing in some places adds needless confusion.

Evidently the longstanding variable named main_thread (which is a thread ID) got 
in the way of the names that thread.c should have used, and thread.c instead 
used names like "primary_thread". I'll try to lessen the confusion by changing 
the longstanding variable name to "main_thread_id" (which is more-accurate 
anyway). This will give thread.c the freedom to use names like "main_thread" for 
its own purposes. Also, it'll avoid an unnecessary duplicate of the main thread 
ID in emacs-module.c. I installed the attached patch to do all this.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Rename-main_thread-to-main_thread_id-and-simplify.patch --]
[-- Type: text/x-diff; name="0001-Rename-main_thread-to-main_thread_id-and-simplify.patch", Size: 6484 bytes --]

From 01adadb144b7bdd456ceb38076e6add925ba6e24 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 30 Dec 2016 13:01:39 -0800
Subject: [PATCH] Rename main_thread to main_thread_id and simplify

* src/emacs-module.c: Include syssignal.h, for main_thread_id.
[HAVE_PTHREAD]: Do not include pthread.h.
(main_thread): Remove.  All uses replaced by main_thread_id,
or by dwMainThreadId on NT.  Since the HAVE_PTHREAD code is now using
the main_thread_id established by sysdep.c, there is no need for a
separate copy of the main thread ID here.
(module_init): Remove.  All uses removed.
* src/sysdep.c (main_thread_id) [HAVE_PTHREAD]:
Rename from main_thread.  All uses changed.  Now extern.
---
 src/emacs-module.c | 30 ++++--------------------------
 src/emacs.c        |  6 ------
 src/lisp.h         |  1 -
 src/sysdep.c       | 14 +++++++-------
 src/syssignal.h    |  1 +
 5 files changed, 12 insertions(+), 40 deletions(-)

diff --git a/src/emacs-module.c b/src/emacs-module.c
index 68aeb0c..280c055 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -28,6 +28,7 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include "lisp.h"
 #include "dynlib.h"
 #include "coding.h"
+#include "syssignal.h"
 
 #include <intprops.h>
 #include <verify.h>
@@ -41,15 +42,9 @@ enum { module_has_cleanup = true };
 enum { module_has_cleanup = false };
 #endif
 
-/* Handle to the main thread.  Used to verify that modules call us in
-   the right thread.  */
-#ifdef HAVE_PTHREAD
-# include <pthread.h>
-static pthread_t main_thread;
-#elif defined WINDOWSNT
+#ifdef WINDOWSNT
 #include <windows.h>
 #include "w32term.h"
-static DWORD main_thread;
 #endif
 
 /* True if Lisp_Object and emacs_value have the same representation.
@@ -751,9 +746,9 @@ static void
 check_main_thread (void)
 {
 #ifdef HAVE_PTHREAD
-  eassert (pthread_equal (pthread_self (), main_thread));
+  eassert (pthread_equal (pthread_self (), main_thread_id));
 #elif defined WINDOWSNT
-  eassert (GetCurrentThreadId () == main_thread);
+  eassert (GetCurrentThreadId () == dwMainThreadId);
 #endif
 }
 
@@ -1062,20 +1057,3 @@ syms_of_module (void)
   DEFSYM (Qinternal__module_call, "internal--module-call");
   defsubr (&Sinternal_module_call);
 }
-
-/* Unlike syms_of_module, this initializer is called even from an
-   initialized (dumped) Emacs.  */
-
-void
-module_init (void)
-{
-  /* It is not guaranteed that dynamic initializers run in the main thread,
-     therefore detect the main thread here.  */
-#ifdef HAVE_PTHREAD
-  main_thread = pthread_self ();
-#elif defined WINDOWSNT
-  /* The 'main' function already recorded the main thread's thread ID,
-     so we need just to use it . */
-  main_thread = dwMainThreadId;
-#endif
-}
diff --git a/src/emacs.c b/src/emacs.c
index eff3f9d..d461fe2 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -738,8 +738,6 @@ main (int argc, char **argv)
      non-ASCII file names during startup.  */
   w32_init_file_name_codepage ();
 #endif
-  /* This has to be done before module_init is called below, so that
-     the latter could use the thread ID of the main thread.  */
   w32_init_main_thread ();
 #endif
 
@@ -757,10 +755,6 @@ main (int argc, char **argv)
   init_standard_fds ();
   atexit (close_output_streams);
 
-#ifdef HAVE_MODULES
-  module_init ();
-#endif
-
   sort_args (argc, argv);
   argc = 0;
   while (argv[argc]) argc++;
diff --git a/src/lisp.h b/src/lisp.h
index 1a586ca..8496308 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3880,7 +3880,6 @@ extern bool let_shadows_global_binding_p (Lisp_Object symbol);
 extern Lisp_Object make_user_ptr (void (*finalizer) (void *), void *p);
 
 /* Defined in emacs-module.c.  */
-extern void module_init (void);
 extern void syms_of_module (void);
 #endif
 
diff --git a/src/sysdep.c b/src/sysdep.c
index 1ba336e..1227afc 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -1612,7 +1612,7 @@ emacs_sigaction_init (struct sigaction *action, signal_handler_t handler)
 }
 
 #ifdef FORWARD_SIGNAL_TO_MAIN_THREAD
-static pthread_t main_thread;
+pthread_t main_thread_id;
 #endif
 
 /* SIG has arrived at the current process.  Deliver it to the main
@@ -1636,13 +1636,13 @@ deliver_process_signal (int sig, signal_handler_t handler)
 
   bool on_main_thread = true;
 #ifdef FORWARD_SIGNAL_TO_MAIN_THREAD
-  if (! pthread_equal (pthread_self (), main_thread))
+  if (! pthread_equal (pthread_self (), main_thread_id))
     {
       sigset_t blocked;
       sigemptyset (&blocked);
       sigaddset (&blocked, sig);
       pthread_sigmask (SIG_BLOCK, &blocked, 0);
-      pthread_kill (main_thread, sig);
+      pthread_kill (main_thread_id, sig);
       on_main_thread = false;
     }
 #endif
@@ -1668,12 +1668,12 @@ deliver_thread_signal (int sig, signal_handler_t handler)
   int old_errno = errno;
 
 #ifdef FORWARD_SIGNAL_TO_MAIN_THREAD
-  if (! pthread_equal (pthread_self (), main_thread))
+  if (! pthread_equal (pthread_self (), main_thread_id))
     {
       thread_backtrace_npointers
 	= backtrace (thread_backtrace_buffer, BACKTRACE_LIMIT_MAX);
       sigaction (sig, &process_fatal_action, 0);
-      pthread_kill (main_thread, sig);
+      pthread_kill (main_thread_id, sig);
 
       /* Avoid further damage while the main thread is exiting.  */
       while (1)
@@ -1796,7 +1796,7 @@ handle_sigsegv (int sig, siginfo_t *siginfo, void *arg)
   bool fatal = gc_in_progress;
 
 #ifdef FORWARD_SIGNAL_TO_MAIN_THREAD
-  if (!fatal && !pthread_equal (pthread_self (), main_thread))
+  if (!fatal && !pthread_equal (pthread_self (), main_thread_id))
     fatal = true;
 #endif
 
@@ -1888,7 +1888,7 @@ init_signals (bool dumping)
   sigemptyset (&empty_mask);
 
 #ifdef FORWARD_SIGNAL_TO_MAIN_THREAD
-  main_thread = pthread_self ();
+  main_thread_id = pthread_self ();
 #endif
 
 #if !HAVE_DECL_SYS_SIGLIST && !defined _sys_siglist
diff --git a/src/syssignal.h b/src/syssignal.h
index 62704fc..215aafe 100644
--- a/src/syssignal.h
+++ b/src/syssignal.h
@@ -32,6 +32,7 @@ extern void unblock_tty_out_signal (sigset_t const *);
 
 #ifdef HAVE_PTHREAD
 #include <pthread.h>
+extern pthread_t main_thread_id;
 /* If defined, asynchronous signals delivered to a non-main thread are
    forwarded to the main thread.  */
 #define FORWARD_SIGNAL_TO_MAIN_THREAD
-- 
2.7.4

Re: Delivering SIGIO when several threads are active?

[Paul Eggert]

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

Paul Eggert wrote:
> This will give thread.c the freedom to use names like "main_thread" for its own
> purposes.

I installed the attached further patch to do that.

[-- Attachment #2: 0001-Rename-primary_thread-to-main_thread.patch --]
[-- Type: text/x-diff, Size: 4738 bytes --]

From ddb1ab4b661358f6f91b96fef132fbb82b7d25dc Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 30 Dec 2016 13:42:38 -0800
Subject: [PATCH] Rename primary_thread to main_thread

This avoids the confusion of using two different phrases "main thread"
and "primary thread" internally to mean the same thing.  See:
http://lists.gnu.org/archive/html/emacs-devel/2016-12/msg01142.html
* src/thread.c (main_thread): Rename from primary_thread,
since the new name no longer clashes with main_thread_id
and Emacs internals normally call this the "main thread".
(init_main_thread): Rename from init_primary_thread.
(main_thread_p): Rename from primary_thread_p.
All uses changed.
---
 src/alloc.c  |  4 ++--
 src/thread.c | 40 ++++++++++++++++++++--------------------
 src/thread.h |  2 +-
 src/w32.h    |  2 +-
 4 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 121d704..d74c4be 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -6404,7 +6404,7 @@ mark_object (Lisp_Object arg)
 
 #ifdef GC_CHECK_MARKED_OBJECTS
 	m = mem_find (po);
-	if (m == MEM_NIL && !SUBRP (obj) && !primary_thread_p (po))
+	if (m == MEM_NIL && !SUBRP (obj) && !main_thread_p (po))
 	  emacs_abort ();
 #endif /* GC_CHECK_MARKED_OBJECTS */
 
@@ -6416,7 +6416,7 @@ mark_object (Lisp_Object arg)
 
 	if (pvectype != PVEC_SUBR
 	    && pvectype != PVEC_BUFFER
-	    && !primary_thread_p (po))
+	    && !main_thread_p (po))
 	  CHECK_LIVE (live_vector_p);
 
 	switch (pvectype)
diff --git a/src/thread.c b/src/thread.c
index 560d2cf..9a1198a 100644
--- a/src/thread.c
+++ b/src/thread.c
@@ -26,11 +26,11 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include "coding.h"
 #include "syssignal.h"
 
-static struct thread_state primary_thread;
+static struct thread_state main_thread;
 
-struct thread_state *current_thread = &primary_thread;
+struct thread_state *current_thread = &main_thread;
 
-static struct thread_state *all_threads = &primary_thread;
+static struct thread_state *all_threads = &main_thread;
 
 static sys_mutex_t global_lock;
 
@@ -927,41 +927,41 @@ thread_check_current_buffer (struct buffer *buffer)
 \f
 
 static void
-init_primary_thread (void)
+init_main_thread (void)
 {
-  primary_thread.header.size
+  main_thread.header.size
     = PSEUDOVECSIZE (struct thread_state, m_stack_bottom);
-  XSETPVECTYPE (&primary_thread, PVEC_THREAD);
-  primary_thread.m_last_thing_searched = Qnil;
-  primary_thread.m_saved_last_thing_searched = Qnil;
-  primary_thread.name = Qnil;
-  primary_thread.function = Qnil;
-  primary_thread.error_symbol = Qnil;
-  primary_thread.error_data = Qnil;
-  primary_thread.event_object = Qnil;
+  XSETPVECTYPE (&main_thread, PVEC_THREAD);
+  main_thread.m_last_thing_searched = Qnil;
+  main_thread.m_saved_last_thing_searched = Qnil;
+  main_thread.name = Qnil;
+  main_thread.function = Qnil;
+  main_thread.error_symbol = Qnil;
+  main_thread.error_data = Qnil;
+  main_thread.event_object = Qnil;
 }
 
 bool
-primary_thread_p (void *ptr)
+main_thread_p (void *ptr)
 {
-  return (ptr == &primary_thread) ? true : false;
+  return ptr == &main_thread;
 }
 
 void
 init_threads_once (void)
 {
-  init_primary_thread ();
+  init_main_thread ();
 }
 
 void
 init_threads (void)
 {
-  init_primary_thread ();
-  sys_cond_init (&primary_thread.thread_condvar);
+  init_main_thread ();
+  sys_cond_init (&main_thread.thread_condvar);
   sys_mutex_init (&global_lock);
   sys_mutex_lock (&global_lock);
-  current_thread = &primary_thread;
-  primary_thread.thread_id = sys_thread_self ();
+  current_thread = &main_thread;
+  main_thread.thread_id = sys_thread_self ();
 }
 
 void
diff --git a/src/thread.h b/src/thread.h
index 9472ae3..e6dc668 100644
--- a/src/thread.h
+++ b/src/thread.h
@@ -285,7 +285,7 @@ extern void maybe_reacquire_global_lock (void);
 extern void init_threads_once (void);
 extern void init_threads (void);
 extern void syms_of_threads (void);
-extern bool primary_thread_p (void *);
+extern bool main_thread_p (void *);
 
 typedef int select_func (int, fd_set *, fd_set *, fd_set *,
 			 const struct timespec *, const sigset_t *);
diff --git a/src/w32.h b/src/w32.h
index c73ff30..03dee09 100644
--- a/src/w32.h
+++ b/src/w32.h
@@ -89,7 +89,7 @@ typedef struct _child_process
      terminate it by sys_kill.  */
   HWND                hwnd;
   /* Information about subprocess returned by CreateProcess.  Includes
-     handles to the subprocess and its primary thread, and the
+     handles to the subprocess and its main thread, and the
      corresponding process ID and thread ID numbers.  The PID is
      mirrored by the 'pid' member above.  The process handle is used
      to wait on it.  */
-- 
2.7.4