Subject: Re: bug#76969: kill-buffer fails silently when a thread exists where it's current

From: Dmitry Gutov <[email-masked]>
Date: Fri, 18 Jul 2025 03:23:43 +0300

On 17/07/2025 09:25, Eli Zaretskii wrote:
>> Date: Sun, 13 Jul 2025 17:02:07 +0300
>> From: Dmitry Gutov <[email-masked]>
>> Cc: [email-masked]
>>
>> Here's a patch with implementation:
>>
>> * It's a property of the thread, on by default.
>> * Set to nil for the main thread (currently can be changed from Lisp).
>> * Not a buffer-local variable - and looking more at it, that seems not a
>> good idea, because several threads could have the same buffer current,
>> and some might be okay with this behavior, while others not.
>> * A couple of questions about the finer details of the implementation,
>> see comments in 'thread_all_before_buffer_killed'.
>> * Two tests included.
>>
>> Seems to work well in my testing, and in the diff-hl scenario described
>> in the report: step 5 kills the buffer and sends the signal which I can
>> catch inside condition-case in diff-hl--update-safe.
>>
>> WDYT?
> 
> It's a starting point, thanks.  Comments:
> 
>   . I think we should add an optional argument to make-thread, because
>     calling a function to change the buffer_killable_p attribute might
>     be too late (I'm not against having the function as well)

Do you think there is a chance for the thread to be started between the 
'make-thread' and 'thread-set-buffer-killable' calls, even if they go 
one after another?

I considered making it a keyword argument of 'make-thread' but it seems 
like it might not interest most callers, and so clutter the definition 
(I couldn't come up with a less awkward name either). Not a strong 
opinion, though.

>   . I think we should also allow a thread to say it's fine to kill the
>     current buffer, but not to signal the thread when the buffer is
>     killed (this means the buffer_killable_p should be not just a
>     simple boolean)

Sounds good to me. Guess that calls for removing '-p' from the name too.

>   . we need documentation changes; in particular some of these changes
>     should be called out as incompatible

Sure.

>> +  for (iter = all_threads; iter; iter = iter->next_thread)
>> +    {
>> +      if (iter == caller_thread)
>> +	continue;
>> +
>> +      /* Could iter->buffer_killable_p be false now?
>> +	 E.g. changed by hooks.  */
> 
> It is best not to rely on it being true, yes.

Will update in the next revision. It will probably complicate the 
calling convention - and create another condition for keeping the buffer.

>> +	  // Or we could inline the last part of its definition.
>> +	  Fthread_signal (thread, Qthread_buffer_killed, Qnil);
> 
> It is better to make that last part be a function, and call it in both
> places.

I'll probably keep this line as-is for now, and we could revisit this as 
a small optimization later.

> And note that doing that could potentially switch threads, which we
> perhaps want to avoid here?

Are you referring to the 'tstate->wait_condvar' check? Hmm, I suppose it 
could hit if the current thread first releases a mutex held by another 
thread, and then kills its buffer.

If that's realistic, a helper function that doesn't do that, might 
indeed be better.

>> @@ -1174,6 +1247,7 @@ init_threads (void)
>>   #endif /* defined HAVE_ANDROID && !defined ANDROID_STUBIFY */
>>   
>>     main_thread.s.thread_id = sys_thread_self ();
>> +  main_thread.s.buffer_killable_p = false;
> 
> Do we allow calling thread-set-buffer-killable on the main thread?

I didn't want to decide on this myself. Could certainly prohibit it, 
looking from safety's perspective.

Unless somebody thinks of a scenario that would make it useful?

>> +  DEFSYM (Qthread_buffer_killed, "thread-buffer-killed");
>> +  Fput (Qthread_buffer_killed, Qerror_conditions,
>> +	list (Qthread_buffer_killed, Qerror));
>> +  Fput (Qscan_error, Qerror_message,
>             ^^^^^^^^^^^
> Copy/pasta?

Yes, thanks.