Subject: bug#79201: 30.1.90; set-process-thread can permanently break fd_callback_info slots

From: Eli Zaretskii <[email-masked]>
Date: Mon, 11 Aug 2025 19:51:08 +0300

> From: Spencer Baugh <[email-masked]>
> Cc: [email-masked],  [email-masked],  [email-masked],
>    [email-masked]
> Date: Mon, 11 Aug 2025 12:03:16 -0400
> 
> > And finally, IMO it is wrong for the add_*_fd functions to overwrite
> > these two members, because they are set and reset by other functions,
> > mainly those which compute the wait mask for the pselect call.
> > add_*_fd should not touch these members.
> 
> I think we should at least add an eassert in those functions which
> checks that .waiting_thread is null.  It is never valid for that to be
> non-null in add_*_fd.

Maybe, maybe not.  I'm not yet sure.  Let's discuss this later.

> > I believe in this case update_processes_for_thread_death doesn't do
> > its job because your scenario kills the process before the thread
> > dies, and update_processes_for_thread_death only looks at the
> > processes that are in Vprocess_alist.  So it is possible that all
> > that's missing is that delete_read_fd and delete_write_fd (which are
> > called from deactivate_process) should clear the .thread member of
> > fd_callback_info corresponding to the process's input and output
> > descriptors.
> >
> > If that doesn't fix the issue with your reproducer, please investigate
> > why, and let's take it from there.
> 
> Yes, that does fix the issue with my reproducer.

Then we should do that, I think.

> But I think it may cause other issues.  delete_read_fd and
> delete_write_fd are called from many other places.  For example,
> delete_write_fd is called before calling add_process_read_fd on (I
> think?) the same fd in wait_reading_process_output.

I don't see these calls to delete an fd before adding it, please point
to the code you had in mind.  When a descriptor is deleted, its
information is completely forgotten, so there's no reason to keep the
.thread and .waiting_thread members.

> Clearing the .thread there would break the guarantees of
> set-process-thread.

I don't think set-process-thread has anything to do with this, see
below.

> Further complicating the issue: I think there's deeper issues with
> set-process-thread.  There's also incorrect information in the manual
> about it:
> 
> processes.texi:
> > by default a process is locked to the thread that created it.  When a
> > process is locked to a thread, output from the process can only be
> > accepted by that thread.
> ...
> > @defun set-process-thread process thread
> > Set the locking thread of @var{process} to @var{thread}.  @var{thread}
> > may be @code{nil}, in which case the process is unlocked.
> > @end defun
> 
> This is not true.  By default .thread is NULL, i.e., processes are not
> locked to any thread.

set-process-thread doesn't set the .thread member of the descriptors,
it sets the .thread member of the process object.  Here's the code
from make-process that processes.texi has in mind:

  static Lisp_Object
  make_process (Lisp_Object name)
  {
    struct Lisp_Process *p = allocate_process ();
    /* Initialize Lisp data.  Note that allocate_process initializes all
       Lisp data to nil, so do it only for slots which should not be nil.  */
    pset_status (p, Qrun);
    pset_mark (p, Fmake_marker ());
    pset_thread (p, Fcurrent_thread ());  <<<<<<<<<<<<<<<<<<<<

And here's accept-process-output heeding this:

  if (! NILP (process))
    {
      CHECK_PROCESS (process);
      struct Lisp_Process *proc = XPROCESS (process);

      /* Can't wait for a process that is dedicated to a different
	 thread.  */
      if (!NILP (proc->thread) && !BASE_EQ (proc->thread, Fcurrent_thread ()))
	{
	  Lisp_Object proc_thread_name = XTHREAD (proc->thread)->name;

	  error ("Attempt to accept output from process %s locked to thread %s",
		 SDATA (proc->name),
		 STRINGP (proc_thread_name)
		 ? SDATA (proc_thread_name)
		 : SDATA (Fprin1_to_string (proc->thread, Qt, Qnil)));
	}

The .thread and .waiting_thread are set when we prepare descriptors
for passing them to pselect, they are a different (although related)
mechanism.

> I think this behavior was just never implemented.  But, it doesn't seem
> to have been needed, since no-one has complained about the lack of it in
> the last 13 years since threads were released.

See above: it's both implemented and working.  You are welcome to add
tests for that.

> So I think there are two good options.  Either:
> 
> A. Make the code match the manual by locking processes to threads by
>    default, and somehow fix the issues this causes with breaking
>    fd_callback_info slots.
> 
> Or:
> B. Deprecate set-process-thread and make it a no-op.  Simply delete this
>    incorrect section of the manual.
> 
> My preference is for B, since this also reduces complexity and will
> slightly improve performance.

Given the code I show above, do you still see a problem?  If yes,
please tell the details.