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 #107 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: Thu, 04 Sep 2025 12:41:04 -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 13:07:08 -0400
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> > +  /* 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.
>
> That's a bit dangerous, since the I/O descriptors of the new process
> are not yet set, so the call to pset_thread there, in case ps->thread
> is non-nil, would do a partial job.  We could instead move the
> pset_thread call to later, where set_proc_thread is called, but I
> preferred to undo what make_process did ASAP.

I'm confused.

- It's just duplicating what make_process did, so I expect it's
  essentially a no-op, just simpler code.

- Why would it be dangerous even if that wasn't the case?  We're still
  setting up this process, it's not visible to any Lisp code, so what
  bad event could even happen?

>> 
>> >    /* 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.
>
> I don't mind moving it out.

Great.

>> 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.
>
> The assertions compile to nothing in a production build.

Then maybe we should turn this assertion on by default in production
builds, since threads are still encountering problems, and it would help
catch problems.  Zhengyi says he saw in a debugger this field was set to
non-NULL at this point, which indicates a bug, so it would help tracking
that down.

> Also, I don't like the caller to assume too much about what
> set_proc_thread does, it's not future-proof.  So I'd prefer to leave
> that call in place.

That's fair.  Though, I think the way set_proc_thread works is a bit
confusing, so I'm inclined to refactor it in the near future anyway.

But, the reason I'd like to take out the call is that it will mask
errors created elsewhere by clearing the field.

Previous Next

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