GNU bug report logs - #17139
Race condition in complete-in-region: should we be using pre-command-hook, not post-command-hook?

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 17139 in the body.
You can then email your comments to 17139 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: Daniel Colascione <dancol <at> dancol.org>
To: "bug-gnu-ema >> bug-emacs" <bug-gnu-emacs <at> gnu.org>
Subject: Race condition in complete-in-region: should we be using
 pre-command-hook, not post-command-hook?
Date: Fri, 28 Mar 2014 19:18:04 -0700

[Message part 1 (text/plain, inline)]

Say we're using a mode derived from comint that implements completion by
using the comint redirection functionality to send commands to the
process associated with the comint buffer. Say we have TAB bound to
complete-symbol. If the user presses TAB (to create a list of
completions) and then immediately presses RET to run comint-send-input,
we send the input to the subprocess, but don't wait for a reply. Then we
run post-command-hook; completion-in-region--postch is on the list of
hooks to run. This function runs completion-in-region-mode--predicate,
which makes a hidden call to the subprocess; this hidden call involves
writing a command waiting for a reply. But because we just sent the
*user* line in comint-send-input, we might actually read the response to
*that* line instead of the internal completion command, causing an
error. The response to the internal completion command then appears in
the comint buffer.

Why can't we do the completion-in-region--postch stuff in pre-command-hook?

[signature.asc (application/pgp-signature, attachment)]

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

From: Daniel Colascione <dancol <at> dancol.org>
To: 17139 <at> debbugs.gnu.org
Subject: Re: bug#17139: Race condition in complete-in-region: should we be
 using pre-command-hook, not post-command-hook?
Date: Fri, 28 Mar 2014 19:39:28 -0700

[Message part 1 (text/plain, inline)]

On 03/28/2014 07:18 PM, Daniel Colascione wrote:
> Say we're using a mode derived from comint that implements completion by
> using the comint redirection functionality to send commands to the
> process associated with the comint buffer. Say we have TAB bound to
> complete-symbol. If the user presses TAB (to create a list of
> completions) and then immediately presses RET to run comint-send-input,
> we send the input to the subprocess, but don't wait for a reply. Then we
> run post-command-hook; completion-in-region--postch is on the list of
> hooks to run. This function runs completion-in-region-mode--predicate,
> which makes a hidden call to the subprocess; this hidden call involves
> writing a command waiting for a reply. But because we just sent the
> *user* line in comint-send-input, we might actually read the response to
> *that* line instead of the internal completion command, causing an
> error. The response to the internal completion command then appears in
> the comint buffer.
> 
> Why can't we do the completion-in-region--postch stuff in pre-command-hook?

I think this fix works, but I'm not particularly familiar with the
completion code. Can someone review?

=== modified file 'lisp/comint.el'
--- lisp/comint.el	2014-03-22 22:12:52 +0000
+++ lisp/comint.el	2014-03-29 02:36:49 +0000
@@ -1769,6 +1769,12 @@

 Similarly for Soar, Scheme, etc."
   (interactive)
+  ;; If we're currently completing, stop.  We're definitely done
+  ;; completing, and by sending the input, we might cause side effects
+  ;; that will confuse the code running in the completion
+  ;; post-command-hook.
+  (when completion-in-region-mode
+    (completion-in-region-mode -1))
   ;; Note that the input string does not include its terminal newline.
   (let ((proc (get-buffer-process (current-buffer))))
     (if (not proc) (user-error "Current buffer has no process")

[signature.asc (application/pgp-signature, attachment)]

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

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Daniel Colascione <dancol <at> dancol.org>
Cc: 17139 <at> debbugs.gnu.org
Subject: Re: bug#17139: Race condition in complete-in-region: should we be
 using pre-command-hook, not post-command-hook?
Date: Sun, 30 Mar 2014 17:39:19 -0400

>> run post-command-hook; completion-in-region--postch is on the list of
>> hooks to run.  This function runs completion-in-region-mode--predicate,
>> which makes a hidden call to the subprocess;

Using the subprocess from completion-in-region--postch sounds
like a problem/bug, indeed.  Why do we(I?) do that?

>> Why can't we do the completion-in-region--postch stuff in pre-command-hook?

Because pre-command-hook would come even later (it would only disable
completion-in-region-mode at the beginning of the next command after RET).

> +  ;; If we're currently completing, stop.  We're definitely done
> +  ;; completing, and by sending the input, we might cause side effects
> +  ;; that will confuse the code running in the completion
> +  ;; post-command-hook.
> +  (when completion-in-region-mode
> +    (completion-in-region-mode -1))

I see you installed it, thanks, I think it's OK.  But I'd also like to
know why we contact the subprocess from completion-in-region--postch,
because that's risky.


        Stefan

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

From: Daniel Colascione <dancol <at> dancol.org>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 17139 <at> debbugs.gnu.org
Subject: Re: bug#17139: Race condition in complete-in-region: should we be
 using pre-command-hook, not post-command-hook?
Date: Sun, 30 Mar 2014 14:49:59 -0700

[Message part 1 (text/plain, inline)]

On 03/30/2014 02:39 PM, Stefan Monnier wrote:
>>> run post-command-hook; completion-in-region--postch is on the list of
>>> hooks to run.  This function runs completion-in-region-mode--predicate,
>>> which makes a hidden call to the subprocess;
> 
> Using the subprocess from completion-in-region--postch sounds
> like a problem/bug, indeed.  Why do we(I?) do that?
> 
>>> Why can't we do the completion-in-region--postch stuff in pre-command-hook?
> 
> Because pre-command-hook would come even later (it would only disable
> completion-in-region-mode at the beginning of the next command after RET).

You're right: that's a bad solution.  I was thinking we could somehow
detect RET and other side-effect-ful commands, but just augmenting
comint-send-input is better.
> 
>> +  ;; If we're currently completing, stop.  We're definitely done
>> +  ;; completing, and by sending the input, we might cause side effects
>> +  ;; that will confuse the code running in the completion
>> +  ;; post-command-hook.
>> +  (when completion-in-region-mode
>> +    (completion-in-region-mode -1))
> 
> I see you installed it, thanks, I think it's OK.  But I'd also like to
> know why we contact the subprocess from completion-in-region--postch,
> because that's risky.

In my case, I have a JavaScript repl to which Emacs connects over a
socket. Each connection gets a separate JavaScript global environment,
so completion in the comint buffer has to use the same underlying socket
that's connected to the buffer. Completion works by using comint
redirection to send JavaScript over the socket, then waiting for a
reply. completion-in-region--postch needs to use the normal completion
machinery (via the predicate set up in completion-at-point) to detect
whether the completion region has changed, and this machinery contacts
the subprocess to find the completion candidates.

Come to think of it, supplying a function instead of a simple list of
strings as the completion table returned from the completion function
would probably help too, since then completion-in-region--postch could
inspect the first element of the returned list (the completion region
start) without having to actually "force the promise" and resolve the
whole list after every command.

[signature.asc (application/pgp-signature, attachment)]

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

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Daniel Colascione <dancol <at> dancol.org>
Cc: 17139 <at> debbugs.gnu.org
Subject: Re: bug#17139: Race condition in complete-in-region: should we be
 using pre-command-hook, not post-command-hook?
Date: Mon, 31 Mar 2014 08:40:04 -0400

> Come to think of it, supplying a function instead of a simple list of
> strings as the completion table returned from the completion function
> would probably help too, since then completion-in-region--postch could
> inspect the first element of the returned list (the completion region
> start) without having to actually "force the promise" and resolve the
> whole list after every command.

Exactly: completion-in-region--postch does not need to know the
candidates.  If you need a subprocess to get the list of candidates,
then foo-at-point-function is usually not the right place/time to build
this list, you should use completion-table-dynamic,
completion-table-with-cache, or something like that instead.

Some foo-at-point-function get away with building the list directly, but
then caching it so that subsequent calls (e.g. via
completion-in-region--postch) don't need to contact the subprocess again.

Of course, there could be cases where the star&end positions are
themselves computed by a subprocess, in which case
completion-in-region--postch would have to contact the subprocess.


        Stefan

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

From: Daniel Colascione <dancol <at> dancol.org>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 17139 <at> debbugs.gnu.org
Subject: Re: bug#17139: Race condition in complete-in-region: should we be
 using pre-command-hook, not post-command-hook?
Date: Mon, 31 Mar 2014 11:20:21 -0700

[Message part 1 (text/plain, inline)]

On 03/31/2014 05:40 AM, Stefan Monnier wrote:
>> Come to think of it, supplying a function instead of a simple list of
>> strings as the completion table returned from the completion function
>> would probably help too, since then completion-in-region--postch could
>> inspect the first element of the returned list (the completion region
>> start) without having to actually "force the promise" and resolve the
>> whole list after every command.
> 
> Exactly: completion-in-region--postch does not need to know the
> candidates.  If you need a subprocess to get the list of candidates,
> then foo-at-point-function is usually not the right place/time to build
> this list, you should use completion-table-dynamic,
> completion-table-with-cache, or something like that instead.

I've added some recommendations to this effect to the manual.

[signature.asc (application/pgp-signature, attachment)]

Message #25 received at 17139-done <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: 17139-done <at> debbugs.gnu.org
Cc: Daniel Colascione <dancol <at> dancol.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#17139: Race condition in complete-in-region: should we be
 using pre-command-hook, not post-command-hook?
Date: Sat, 4 Jun 2016 18:17:49 -0400

Closing since this seems to have been resolved.

Previous Next

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