unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#17139: Race condition in complete-in-region: should we be using pre-command-hook, not post-command-hook?
@ 2014-03-29  2:18 Daniel Colascione
  2014-03-29  2:39 ` Daniel Colascione
  2016-06-04 22:17 ` Noam Postavsky
  0 siblings, 2 replies; 7+ messages in thread
From: Daniel Colascione @ 2014-03-29  2:18 UTC (permalink / raw)
  To: 17139

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#17139: Race condition in complete-in-region: should we be using pre-command-hook, not post-command-hook?
  2014-03-29  2:18 bug#17139: Race condition in complete-in-region: should we be using pre-command-hook, not post-command-hook? Daniel Colascione
@ 2014-03-29  2:39 ` Daniel Colascione
  2014-03-30 21:39   ` Stefan Monnier
  2016-06-04 22:17 ` Noam Postavsky
  1 sibling, 1 reply; 7+ messages in thread
From: Daniel Colascione @ 2014-03-29  2:39 UTC (permalink / raw)
  To: 17139

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#17139: Race condition in complete-in-region: should we be using pre-command-hook, not post-command-hook?
  2014-03-29  2:39 ` Daniel Colascione
@ 2014-03-30 21:39   ` Stefan Monnier
  2014-03-30 21:49     ` Daniel Colascione
  0 siblings, 1 reply; 7+ messages in thread
From: Stefan Monnier @ 2014-03-30 21:39 UTC (permalink / raw)
  To: Daniel Colascione; +Cc: 17139

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





^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#17139: Race condition in complete-in-region: should we be using pre-command-hook, not post-command-hook?
  2014-03-30 21:39   ` Stefan Monnier
@ 2014-03-30 21:49     ` Daniel Colascione
  2014-03-31 12:40       ` Stefan Monnier
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel Colascione @ 2014-03-30 21:49 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 17139

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#17139: Race condition in complete-in-region: should we be using pre-command-hook, not post-command-hook?
  2014-03-30 21:49     ` Daniel Colascione
@ 2014-03-31 12:40       ` Stefan Monnier
  2014-03-31 18:20         ` Daniel Colascione
  0 siblings, 1 reply; 7+ messages in thread
From: Stefan Monnier @ 2014-03-31 12:40 UTC (permalink / raw)
  To: Daniel Colascione; +Cc: 17139

> 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





^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#17139: Race condition in complete-in-region: should we be using pre-command-hook, not post-command-hook?
  2014-03-31 12:40       ` Stefan Monnier
@ 2014-03-31 18:20         ` Daniel Colascione
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Colascione @ 2014-03-31 18:20 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 17139

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#17139: Race condition in complete-in-region: should we be using pre-command-hook, not post-command-hook?
  2014-03-29  2:18 bug#17139: Race condition in complete-in-region: should we be using pre-command-hook, not post-command-hook? Daniel Colascione
  2014-03-29  2:39 ` Daniel Colascione
@ 2016-06-04 22:17 ` Noam Postavsky
  1 sibling, 0 replies; 7+ messages in thread
From: Noam Postavsky @ 2016-06-04 22:17 UTC (permalink / raw)
  To: 17139-done; +Cc: Stefan Monnier

Closing since this seems to have been resolved.





^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-06-04 22:17 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-29  2:18 bug#17139: Race condition in complete-in-region: should we be using pre-command-hook, not post-command-hook? Daniel Colascione
2014-03-29  2:39 ` Daniel Colascione
2014-03-30 21:39   ` Stefan Monnier
2014-03-30 21:49     ` Daniel Colascione
2014-03-31 12:40       ` Stefan Monnier
2014-03-31 18:20         ` Daniel Colascione
2016-06-04 22:17 ` Noam Postavsky

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).