GNU bug report logs - #79334
[PATCH] Don't release thread select lock unnecessarily

Previous Next

Full log

View this message in rfc822 format

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 79334 <at> debbugs.gnu.org, eggert <at> cs.ucla.edu, dmitry <at> gutov.dev
Subject: bug#79334: [PATCH] Don't release thread select lock unnecessarily, [PATCH] Defer closing file descriptors used by other threads
Date: Mon, 01 Sep 2025 12:18:37 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> >> +static bool
>> >> +other_thread_is_waiting_for (struct fd_callback_data* elem)
>> >> +{
>> >> +  return elem->waiting_thread != NULL && elem->waiting_thread != current_thread;
>> >> +}
>> >
>> > This should also check that the waiting thread is still alive.  If it
>> > isn't, it's okay to close the descriptor.  Otherwise, descriptors
>> > belonging to threads that exited might never be closed.
>> 
>> waiting_thread always points to a living thread, because it's only set
>> while a thread is inside wait_reading_process_output.
>> 
>> (In fact, when a thread sees waiting_thread set to a value which is
>> neither NULL nor current_thread, it can only ever point to a thread
>> which is currently inside thread_select)
>
> You assume that the waiting_thread field will be cleared when a thread
> dies, but that is not guaranteed.  Why isn't it better to be defensive
> and make sure the thread is still alive?  If you are right, that test
> will always be true, but what if you are wrong in some rare cases?

Fair enough; the patch has changed a lot so this
other_thread_is_waiting_for function doesn't exist anymore, but if I
re-add it or similar checks, I will check that the thread is alive.

>> >> +	  if (fd_callback_info[fd].waiting_thread == current_thread
>> >> +	      && (fd_callback_info[fd].flags == WAITING_FOR_CLOSE_FD))
>> >> +	    {
>> >> +	      fprintf (stderr, "closing deferred fd %d\n", fd);
>> >> +	      emacs_close (fd);
>> >> +	      /* Ignore any events which happened on this fd. */
>> >> +	      FD_CLR (fd, &Available);
>> >> +	      FD_CLR (fd, &Writeok);
>> >> +	      fd_callback_info[fd].flags = 0;
>> >
>> > Why do it here and not when we deactivate processes?
>> 
>> Because that would close file descriptors that another thread is
>> currently selecting on, causing EBADF.
>
> No, I mean to deactivate only the processes whose I/O channels are
> locked to the current thread.
>
>> > Until now we never closed any descriptors inside
>> > wait_reading_process_output.
>> 
>> Just to be clear, we can call deactivate_process from
>> wait_reading_process_output, through status_notify, through process
>> filters, or other similar.  That closes descriptors.
>
> Yes, and so I suggest doing this logic of deactivating only the
> processes that are locked to the current thread there.
>
>> > More generally, why this design and not a simpler change which only
>> > deactivates processes whose I/O is waited by the current thread, as
>> > discussed previously?
>> 
>> I looked into that, but I determined that it's too complicated.  It
>> would require us to keep track of a new kind of process state: a process
>> which has been deleted, but hasn't been deactivated yet.
>
> What I had in mind was neither delete it nor deactivate it.  IOW, the
> loop which goes through the list of processes should only pay
> attention to processes locked to this thread and those that are not
> locked to any live thread.  It should leave the other processes to
> when their threads call status_notify.

There are too many ways in which we delete or deactivate processes to do
that.  In particular, Lisp programs should be able to call
delete-process on a process without getting an error just because some
thread is currently calling select and happens to be monitoring that
process.  To emit such an error would make programming with processes
and threads extremely difficult.

>> > Looking at all the callers of close_process_fd, it sounds like it's
>> > too low-level to do this.  For example, it is called from
>> > create_process (including in the forked child) to close the unneeded
>> > ends of the pipe, where we probably don't want this.  And
>> > clear_fd_callback_data is also used for the keyboard input descriptor,
>> > which is probably also not relevant.
>> 
>> True.
>> 
>> I looked into fixing these, and that suggested to me an altogether
>> simpler approach.  We can keep the logic entirely contained in
>> wait_reading_process_output, just adding a bit of code around the select
>> call.
>> 
>> And we don't need to defer deleting the fds at all: just recognize when
>> it's happened, and be careful to not touch those fds afterwards.
>
> Hmm... I don't understand: why do we need to do this?  The calls to
> compute_input_wait_mask, compute_non_keyboard_wait_mask etc., which
> set bits in the Available mask passed top pselect already avoid
> setting bits for descriptors on which some other thread waits.  So
> pselect cannot possibly indicate any output ready on any descriptors
> belonging to other threads, and this loop you suggest to add:
>
>> +      /* Save the fds we're currently waiting_thread for.  */
>> +      fd_set waiting_fds;
>> +      FD_ZERO (&waiting_fds);
>> +      int max_waiting_fds = 0;
>> +      for (int fd = 0; fd <= max_desc; ++fd)
>> +        {
>> +	  if (fd_callback_info[fd].waiting_thread == current_thread)
>> +	    {
>> +	      FD_SET (fd, &waiting_fds);
>> +	      max_waiting_fds = fd;
>> +	    }
>> +	}
>
> should be unnecessary.  Or what am I missing?

That's exactly the problem.  A file descriptor for which we set
waiting_thread can have delete_read_fd or delete_write_fd called on it
by another thread, or by a signal handler, while we're waiting in
pselect.

We need to catch the fact that the file descriptor has been removed from
the set of file descriptors we're supposed to monitor, and avoid
reacting to events on it.  We can catch this easily by seeing that
waiting_thread has been cleared or changed while we were blocked in
select.

BTW, the fact that this problematic case is also possible via signal
handlers (namely the SIGCHLD handler) is a separate bug introduced by
the refactoring which added fd_callback_info.  It means that with a
certain interleaving, I think it should be possible to get buggy
behavior by just using processes without threads.  It's quite rare
though and I haven't been able to figure out what exact path that would
cause a problem.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.