Subject: Re: bug#79334: [PATCH] Don't release thread select lock unnecessarily

From: Eli Zaretskii <[email-masked]>
Date: Sat, 30 Aug 2025 09:18:12 +0300

> From: Spencer Baugh <[email-masked]>
> Cc: [email-masked],  [email-masked],  [email-masked]
> Date: Fri, 29 Aug 2025 15:50:00 -0400
> 
> Many different pieces of code in Emacs can close file descriptors.  We
> need to be generically careful to not close file descriptors which some
> other thread is currently waiting on.  In my patch, we do this by
> letting the waiting_thread itself close those file descriptors, after it
> returns from select.

Not literally "when returning from pselect", I hope, but when the
process is being deleted _and_ the waiting thread returns from
pselect, right?  IOW, we don't close the descriptor each time pselecft
returns in the right thread, then reopen it before the next call to
pselect, because that won't work.

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

> @@ -2113,7 +2131,11 @@ close_process_fd (int *fd_addr)
>    if (0 <= fd)
>      {
>        *fd_addr = -1;
> -      emacs_close (fd);
> +      if (other_thread_is_waiting_for (&fd_callback_info[fd]))
> +	/* Let waiting_thread handle closing the fd. */
                                                   ^^
Style: 2 spaces between the period and "*/".

> +      /* Close fds which other threads wanted to close.  */
> +      for (int fd = 0; fd <= max_desc; ++fd)
> +	{
> +	  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?  Until now we
never closed any descriptors inside wait_reading_process_output.  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 think this alternative would be easier to
understand and reason about, because (as established in another
discussion) wait_reading_process_output frequently loops more than we
expect it to.

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.  So I wonder whether it's clean
to have this logic at such a low level, where the purpose of the
descriptor is barely known, and not at a higher level, where we know
what each descriptor is used for, and therefore the purpose and the
intent of the logic is much more clear.