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

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

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]

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?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

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

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 1919 bytes --]

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")




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

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

[Stefan Monnier]

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

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

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 2292 bytes --]

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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

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

[Stefan Monnier]

> 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

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

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]

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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

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

[Noam Postavsky]

Closing since this seems to have been resolved.