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

From: Spencer Baugh <[email-masked]>
Date: Mon, 11 Aug 2025 13:20:05 -0400

Eli Zaretskii <[email-masked]> writes:

>> 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.

set-process-thread is the only code which ever sets the .thread field of
fd_callback_info to a non-null value.

>> 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,

No, it does.  Here's the code:

  DEFUN ("set-process-thread", Fset_process_thread, Sset_process_thread,
         2, 2, 0,
         doc: /* Set the locking thread of PROCESS to be THREAD.
  If THREAD is nil, the process is unlocked.  */)
    (Lisp_Object process, Lisp_Object thread)
  {
...
    if (proc->infd >= 0)
      fd_callback_info[proc->infd].thread = tstate;
    eassert (proc->outfd < FD_SETSIZE);
    if (proc->outfd >= 0)
      fd_callback_info[proc->outfd].thread = tstate;
  
    return thread;
  }

I think set-process-thread should not do this, though.  It should only
set the thread member of the process object.  It should not 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.

Yes, sorry, I missed that there are two separate mechanisms.  I think
set-process-thread should continue setting the thread member of the
process object, but not the thread member of the fd_callback_info struct.

>> 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.

Yes.  Let's just make set-process-thread stop setting the thread member
of the fd_callback_info struct.