When using helm from terminal emacs, typing too quickly may result in helm unexpectedly closing

[mullikine]

Expected behavior

Helm should not quit when typing too quickly in terminal emacs, when using a package such as helm-dash, which generates candidates using an external process, but who's source does not say that it uses a process.

Actual behavior (from emacs-helm.sh if possible, see note above)

Helm will unexpectedly quit after a bell is generated from typing too fast. Inhibit-quit is not enabled by helm-get-candidates because helm doesn't know the source uses an external process.

Steps to reproduce (recipe)

Install helm-dash.
Run emacs in the terminal like emacs -nw.
Install a docset using helm-dash-install-docset.
Run helm-dash and type playbook really quickly, or mash keys.
At this stage the helm has probably quit unexpectedly with no error message.

in *messages*, you may see the following:

Waiting for process to die...(type C-g again to kill it instantly)
Waiting for process to die...done

Describe versions of Helm, Emacs, operating system, etc.

• helm 20210320.1405
• GNU Emacs 28.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.20, cairo version 1.16.0) of 2021-03-23
• helm-dash 20190527.1118

Are you using emacs-helm.sh to reproduce this bug? (yes/no):

no

Potential fix:

Change inhibit-quit to be permanently t, or do a check for terminal emacs frame.

(defun helm-get-candidates (source)
  "Retrieve and return the list of candidates from SOURCE."
  (let* ((candidate-fn (assoc-default 'candidates source))
         (candidate-proc (assoc-default 'candidates-process source))
         ;; See comment in helm-get-cached-candidates (Bug#2113).
         ;; (inhibit-quit candidate-proc)
         (inhibit-quit t)
...

[thierryvolpiatto]

Shane Mulligan ***@***.***> writes:

Steps to reproduce (recipe) Install helm-dash.

I don't know what is helm-dash, please report bug to the helm-dash maintainer. Thanks.

…

-- Thierry

[mullikine]

Some packages using helm implement a source without specifying that it is a process. So every package that does this is broken. Because most users are using GUI then there are many helm packages broken for terminal emacs. But fixing it here will fix it for all.

I will also report to the helm-dash maintainer.

[mullikine]

Here is some more information on the issue. I think there are two problems here and the issue should be fixed in both helm and in helm-dash. This is because some packages may handle the emacs process directly, instead of letting helm handle it. Since the problem only happens with terminal emacs, it makes sense to me to fix it here too when the frame is a terminal. Thank you very much!

dash-docs-el/helm-dash#198

[thierryvolpiatto]

Shane Mulligan ***@***.***> writes:

[[PGP Encrypted Part:OK]] Some packages using helm implement a source without specifying that it is a process. So every package that does this is broken. Because most users are using GUI then there are many helm packages broken for terminal emacs. But fixing it here will fix it for all.

Thanks for explanations, however I don't think it is a good idea to set inhibit-quit for all when running helm in a terminal, I would prefer people create their sources properly. IOW I don't think it is a good idea to make a fix in helm for badly written helm sources. (defun helm-dash--build-source (docset) "Build a Helm source for DOCSET." (lexical-let ((docset docset)) (helm-build-sync-source (car docset) :action-transformer #'helm-dash-actions :candidates (lambda () (cl-loop for row in (helm-dash--run-query docset helm-pattern) collect (helm-dash--candidate docset row))) :volatile t :persistent-help "View doc" :requires-pattern helm-dash-min-length))) Clearly this source is badly written, first of all it uses `lexical-let` please stop using this even if it is unrelated to this bug. Then indeed a better implementation would use `helm-build-async-source` instead and use :candidates-process with a function returning a process. If one really want to make such source sync, probably it is a better idea to cache the result of the function calling the process (assume it is a call-process and not start-process) and use the cache in the :candidates function instead of calling constantly a process each time input changes, but it seems it is not possible here (don't know exactly what dash is and do). Thanks.

…

-- Thierry

[thierryvolpiatto]

I can easily reproduce this bug with one of our old source helm-source-google-suggest badly implemented; Using inhibit-quit doesn't help.

[thierryvolpiatto]

A workaround is to use a smaller helm-input-idle-delay:

(defun helm-dash (&optional input-pattern)
  "Bring up a `helm-dash' search interface.
If INPUT-PATTERN is non-nil, use it as an initial input in helm search."
  (interactive)
  (helm-dash-initialize-debugging-buffer)
  (helm-dash-create-common-connections)
  (helm-dash-create-buffer-connections)
  (helm :sources (helm-dash--sources-narrowed-docsets)
        :buffer "*helm-dash*"
        :prompt "Doc for: "
        :history 'helm-dash-history-input
        :input input-pattern
        :candidate-number-limit 1000
        :input-idle-delay 0.6))

[mullikine]

With helm-source-google-suggest I have discovered another workaround. By setting (setq helm-net-prefer-curl t) and setting inhibit-quit to t inside the candidates function. I think this is because call-process no longer receives the C-g. However, when using the default url-retrieve-synchronously, something is breaking but in a different way, which I have not figured out yet.

(setq helm-net-prefer-curl t)
(setq helm-source-google-suggest
  (helm-build-sync-source "Google Suggest"
    :candidates (lambda () (let ((inhibit-quit t)) (funcall helm-google-suggest-default-function)))
    :action 'helm-google-suggest-actions
    :volatile t
    :keymap helm-map
    :requires-pattern 3))

That's all I know so far.

[mullikine]

A helm-while-no-input is required somewhere around the url-retrieve-synchronously. This is due to the bug described in url-retrieve-synchronously.

https://github.com/emacs-mirror/emacs/blob/7124598068ed267c34e1d70b49361f499fc31c9f/lisp/url/url.el#L292

I'm not sure where the best placement is. That would be up to a helm maintainer.
For example, this avoids the error but it's not a final solution because the timer is giving errors as you are typing now and may not display all characters entered.

(defun helm-net--url-retrieve-sync (request parser)
  (if helm-net-prefer-curl
      (with-temp-buffer
        (apply #'call-process "curl"
               nil `(t ,helm-net-curl-log-file) nil request helm-net-curl-switches)
        (funcall parser))
    (helm-while-no-input
      (with-current-buffer
          (progn
            (url-retrieve-synchronously request)
            (funcall parser))))))

[mullikine]

So I'd just like to reiterate, this is two separate problems, both related to terminal emacs. 1) Pending keyboard input is interrupting accept-process-output and can be avoided with helm-while-no-input. 2) bell (C-g) generated from terminal is interrupting call-process when typing quickly and can be avoided with inhibit-quit.

[thierryvolpiatto]

Shane Mulligan ***@***.***> writes:

So I'd just like to reiterate, this is two separate problems, both related to terminal emacs.

Thanks for looking into this and summarize problems, I will look into this in the next days.

…

-- Thierry

[mullikine]

Hey @thierryvolpiatto . Sadly, I think the url-retrieve-synchronously bug might be upstream (emacs). I tried but was unable to make it work without making the change in url.el. If you want to try fix it from helm, best of luck. I will attempt to raise an issue on emacs. I closed my attempted fix because I hadn't fixed the issue. #2419

[mullikine]

I have sent a bug report to emacs.

[thierryvolpiatto]

Shane Mulligan ***@***.***> writes:

I have sent a bug report to emacs.

I think it is now fixed, now pushing to devel branch.

…

-- Thierry

[mullikine]

This fixes the problem for me, too. Thank you.

[mullikine]

If an emacs maintainer sees this as I noted this issue in the bug report. please still consider the fix to url-retrieve-synchronously as possibly other projects are affected.

                     (while-no-input
                       (with-local-quit
                         (accept-process-output proc 1)))

[mullikine]

Although the with-helm-quittable fix works to prevent helm from quitting in both cases, the text [quit] still appears when typing as you can see here. This is for the inhibit-quit sub-issue. You can see that adding inhibit-quit makes it visually clean.

https://asciinema.org/a/tOJreP91sLT2Eam1efxBWiJfo

[mullikine]

Hey @thierryvolpiatto ,

I may have resolved this issue upstream in emacs with the following patch to url-retrieve-synchronously.What this achieves is to trigger a quit in a controlled environment rather than allowing it to occur when accept-process-output is run.
It's not always wanted to trigger a quit when (input-pending-p) is t. But I noticed from placing while-no-input around accept-process-output to avoid the quit that url-retrieve-synchronously would then hang but with the controlled quit happening beforehand, accept-process-output no longer needs while-no-input around it. The end result is buttery smooth helm with no accidental quit from typing too fast. I think this may have resulted in GUI helm faster too. But this is a change to url.el, not helm.

<       (let ((proc (get-buffer-process asynch-buffer)))
---
>       (let ((proc (get-buffer-process asynch-buffer))
>             (counter 0))
71a73,78
>             (with-local-quit
>               (if (input-pending-p)
>                   (progn
>                     (setq counter (1+ counter))
>                     (if (> counter 20)
>                         (keyboard-quit)))))

https://asciinema.org/a/VAn7LrFeJbVzNTWM2Pkc90BRD

I also submitted a PR to helm-google to fix the google parser there. Buttery smooth helm googling now :D

https://framagit.org/steckerhalter/helm-google/-/merge_requests/21

[thierryvolpiatto]

Shane Mulligan ***@***.***> writes:

I may have resolved this issue upstream in emacs with the following patch to url-retrieve-synchronously.What this achieves is to trigger a quit in a controlled environment rather than allowing it to occur when accept-process-output is run. It's not always wanted to trigger a quit when (input-pending-p) is t. But I noticed from placing while-no-input around accept-process-output to avoid the quit that url-retrieve-synchronously would then hang but with the controlled quit happening beforehand, accept-process-output no longer needs while-no-input around it.

while-no-input is itself wrapping body inside a with-local-quit, that is maybe the reason.

The end result is buttery smooth helm with no accidental quit from typing too fast.

With last changes I made in helm, I have no unexpected exits even when using url-retrieve-synchronously, have a try. Thanks.

…

-- Thierry