GNU bug report logs - #79367
31.0.50; magit-commit sometimes doesn't work if diff-hl-update-async is t

Previous Next

Full log

Message #92 received at 79367 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: i <at> fuzy.me, 79367 <at> debbugs.gnu.org, dmitry <at> gutov.dev
Subject: Re: bug#79367: 31.0.50; magit-commit sometimes doesn't work if
 diff-hl-update-async is t
Date: Wed, 03 Sep 2025 13:07:08 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Cc: i <at> fuzy.me,  79367 <at> debbugs.gnu.org,  dmitry <at> gutov.dev
>> Date: Wed, 03 Sep 2025 12:03:42 -0400
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> >> >> But I think we need to call set_proc_thread when the server process
>> >> >> is not locked as well.  Otherwise the `thread' member of the
>> >> >> fd_callback_info may still be a dangling pointer.
>> >> >
>> >> > Oh, you mean the below?  I agree, it is safer.
>> >> 
>> >> We should not do that - if the thread member of fd_callback_info is a
>> >> dangling pointer, that indicates there's a bug elsewhere.
>> >
>> > So how about adding an assertion before we clear it?
>> 
>> Sure.  fd_callback_info.thread specifically should always be NULL at the
>> time we're using the fd_callback_info slot for some new fd, so we should
>> assert that.
>
> So, this:
>
> diff --git a/src/process.c b/src/process.c
> index d6efac5..8f5be5e 100644
> --- a/src/process.c
> +++ b/src/process.c
> @@ -5078,6 +5078,10 @@ server_accept_connection (Lisp_Object server, int channel)
>    fcntl (s, F_SETFL, O_NONBLOCK);
>  
>    p = XPROCESS (proc);
> +  /* make_process calls pset_thread, but if the server process is not
> +     locked to any thread, we need to undo what make_process did.  */
> +  if (NILP (ps->thread))
> +    pset_thread (p, Qnil);

We should probably just unconditionally do:
  pset_thread (p, ps->thread)
here.

>    /* Build new contact information for this setup.  */
>    contact = Fcopy_sequence (ps->childp);
> @@ -5117,6 +5121,19 @@ server_accept_connection (Lisp_Object server, int channel)
>      add_process_read_fd (s);
>    if (s > max_desc)
>      max_desc = s;
> +  /* If the server process is locked to this thread, lock the client
> +     process to the same thread, otherwise clear the thread of its I/O
> +     descriptors.  */
> +  if (NILP (ps->thread))
> +    {
> +      eassert (!fd_callback_info[p->infd].thread);
> +      set_proc_thread (p, NULL);

Yes, this is the right assertion.  But it should be outside the if/else
because it's true even if ps->thread isn't nil.

This whole if/else conditional should probably be just:

  eassert (!fd_callback_info[p->infd].thread);
  if (!NILP (p->thread))
    {
      eassert (XTHREAD (ps->thread) == current_thread);
      set_proc_thread (p, XTHREAD (p->thread));
    }

It's not necessary to call set_proc_thread(p, NULL) in the other case
since it will just set .thread to NULL, which we're already asserting
anyway.

> +    }
> +  else
> +    {
> +      eassert (XTHREAD (ps->thread) == current_thread);
> +      set_proc_thread (p, XTHREAD (ps->thread));
> +    }

Previous Next

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