GNU bug report logs -
#79201
30.1.90; set-process-thread can permanently break fd_callback_info slots
Previous Next
Full log
Message #20 received at 79201 <at> debbugs.gnu.org (full text, mbox):
> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Cc: 79201 <at> debbugs.gnu.org, dmitry <at> gutov.dev, johnw <at> gnu.org,
> app-emacs-dev <at> janestreet.com
> 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.
This bug report was last modified today.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.