Helm + SLY hangs (workaround)

[pouar]

In case you aren't aware, flex completion in Sly when using Helm was working again, at least until this commit, I don't remember whether the fix was on Sly's side or Helm's side.

In d4e52fe7fd31ed408bc60608416f785949b95133, matches sometimes show up and sometimes don't. Reverting the commit seems to fix it.

EDIT BY SLY's AUTHOR: This problem has a solution in this comment in terms of a small ad-hoc fix to Helm's code. EDIT2: The problem now has a fix in SLY proper.

[joaotavora]

Thierry Volpiatto notifications@github.com writes:

João Távora notifications@github.com writes:

Can you at explain to us what is happenning?

With (while (sit-for n)) you block the minibuffer and when helm tries to start it fails at initial update with the computation beeing inside a while-no-input.

What is "blocking the minibuffer" for you? This function, like most functions is "blocking", i.e. it will not return normally until it has fulfilled its function. There are two ways the function can return:

1. The results have arrived from the Lisp process and the function gives them to you. This can take from milliseconds to some seconds depending on many factors.

2. Your hand has touched some key on the keyboard/mouse and the function gives you CANCEL-ON-INPUT-RETVAL immediately, and then Emacs goes on to process your input.

The use of sit-for handles both these things gracefully.

Why does the inhibit-quit fix it?

inhibit-quit makes with-local-quit behaving differently, prevents quitting while helm is updating its candidates. But I don't understand enough the Emacs internal to tell you the interaction with sit-for (and read-event).

Indeed, I don't know what's the matter here either, because

(while-no-input (while (sit-for 30)))

Works perfectly. So if Helm needs to do while-no-input and SLY needs to do sit-for, I don't see why it would break.

Maybe the problem is that Helm is doing its own special version of while-no-input, with different semantics. Or maybe the problem is something else.

Anyhow, I think that if you tweak inhibit-quit to be t in Helm when dealing with processes, I don't see a reason not to tweak it to be t everywhere, as I proposed here (https://github.com/emacs-helm/helm-sly/issues/2#issuecomment-690099041) If you do that in Helm, the bug seems to be gone.

Note that if you are affraid using inhibit-quit, using (while (accept-process-output nil 30)) fixes the bug as well (seems it doesn't block the minibuffer but block input), I see you are already using it in next cond clause, perhaps you can use it in this clause as well? (but perhaps I miss something).

Yes, you miss the fact that it doesn't return immediately when the user inputs something. Read CANCEL-ON-INPUT's docstring.

(company etc... not tested) Obviously that's not practical What is not practical?

To break every other UI using SLY.

[joaotavora]

I just added a commit that doesn't change much, but could. This was advised once to me by Dmitry Gutov of company-mode but I never got around to doing it, because it doesn't change much in practice. I have now. Anyway, company-mode (and maybe other UIs) tweak the value of non-essential to tell the lower layers, such as the process function, if they needn't worry terribly about returning precise results. This is exactly what SLY now uses to activate the CANCEL-ON-RETVAL behaviour. It works fine with Company obviously, but stubbornly, still fails with Helm.

That's because Helm also uses non-essential bound to t. Which means that it's telling lower layers that they can return early with imprecise results, but then hangs when they use sit-for for doing so.

I've tested with Helm setting non-essential to nil and it "fixes" the bug, sort of. Helm+SLY will not be as good as Company+SLY, but at least it won't hang forever.

[thierryvolpiatto]

João Távora notifications@github.com writes:

Thierry Volpiatto notifications@github.com writes:

(while (accept-process-output nil 30)) is working fine with sly-symbol-completion-mode, helm-mode and company-mode and also regular emacs vanilla completion of course.

It's not, Thierry, it's not working "fine" becasue it will not return immediately when the user presses a key.

I don't understand what you are talking about. Things are simple for me, with the different changes I have proposed (see https://github.com/thierryvolpiatto/sly/tree/Fix_completion_on_helm) I can use sly and its completion with helm, company, regular emacs completion, without emacs hang and fail to complete after some time.

With the changes you have proposed for helm, it break helm in other places.

I personally don't need Sly, I just try from time to time to verify if helm is working with different packages, so far helm works everywhere but in sly, we clearly show you where the bug was located, if you want to fix it, you can.

And that's, pardon the pun, "key" for responsive behaviour.

Didn't you find it curious that by doing that, then the documented prominently documented CANCEL-ON-INPUT in the function's docstring would be completely useless?

Yes, because you are trying to reinvent what inhibit-quit does. If I was you I would consider rewriting sly-eval from scratch.

-- Thierry

[thierryvolpiatto]

João Távora notifications@github.com writes:

I just added a commit that doesn't change much, but could. This was advised once to me by Dmitry Gutov of company-mode but I never got around to doing it, because it doesn't change much in practice. I have now. Anyway, company-mode (and maybe other UIs) tweak the value of non-essential to tell the lower layers, such as the process function, if they needn't worry terribly about returning precise results. This is exactly what SLY now uses to activate the CANCEL-ON-RETVAL behaviour. It works fine with Company obviously, but stubbornly, still fails with Helm.

Looks even more hackish, sorry but I prefer keeping helm far from this.

I've tested with Helm setting non-essential to nil and it "fixes" the bug

No, it is not acceptable, sorry.

-- Thierry

[monnier]

Why does the inhibit-quit fix it? inhibit-quit makes with-local-quit behaving differently, prevents quitting while helm is updating its candidates. But I don't understand enough the Emacs internal to tell you the interaction with sit-for (and read-event). Indeed, I don't know what's the matter here either, because

I strongly suspect that nobody understands this well enough to answer without doing a fair bit of debugging here.

(while-no-input (while (sit-for 30)))

Works perfectly. So if Helm needs to do while-no-input and SLY needs to do sit-for, I don't see why it would break.

But the while-no-input above seems redundant. Presumably

(while-no-input (while t (accept-process-output <proc>)))

or (while-no-input (while t (accept-process-output nil 30)))

should give you similar behavior, BTW (and the first is arguably preferable: I think accept-process-output should almost always be provided with the process for in which we're interested).

Maybe the problem is that Helm is doing its own special version of while-no-input, with different semantics. Or maybe the problem is something else.

I wouldn't be surprised if there's no clear blame because it's a subtle interaction between different elements each with subtle semantics. [ Reminded of a longstanding bug in CVS/SSH/libc where each one "DTRT" yet the result was still buggy. ]

Maybe we should start by adding comments about the assumptions we make you the respective parts of the code (both in Helm and SLY), then try and determine whether the problem is that some of those assumptions aren't actually satisfied, or whether it's that those assumptions aren't actually sufficient to provide the behavior we're looking for.

Let-binding inhibit-quit to t is "dangerous" so we should have a clear and documented reason to do that each time we do it.

    Stefan

[monnier]

Anyway, company-mode (and maybe other UIs) tweak the value of non-essential to tell the lower layers, such as the process function, if they needn't worry terribly about returning precise results.

Note: it's not really about returning precise results or not but about not bothering the user (because we're performing a computation that's not been explicitly requested by the user, so it's better to return incomplete results than to prompt the user "out of nowhere").

If binding it causes or avoids a hang, it's a sign that we have a bug somewhere, IMO.

[joaotavora]

I don't understand what you are talking about. Things are simple for me, with the different changes I have proposed (see https://github.com/thierryvolpiatto/sly/tree/Fix_completion_on_helm) I can use sly and its completion with helm, company, regular emacs completion,

No you are not seeing the whole picture. With the changes that you propose SLY's completion sort of works with company, for example, but it becomes unbearably unresponsive. So your changes are invalid.

With the changes you have proposed for helm, it break helm in other places.

Which other places? Where else does it break if you set inhibit-quit to t?

Yes, because you are trying to reinvent what inhibit-quit does. If I was you I would consider rewriting sly-eval from scratch.

The semantics of sly-eval have nothing to do with inhibit-quit. "quitting" does not enter into it, except if you type C-g of course, in which case it works as with other function. If you can write sly-eval from scratch, fine. Show it to me. As long as it honours the protocol.

Looks even more hackish, sorry but I prefer keeping helm far from this.

In general saying that the other package's code is "hackish" doesn't advance the discussion in any way: the other side can throw that argument right back at you. For example, you already bind non-essential in helm some 10 or 15 times. And you already bind inhibit-quit three times, as well. As you have tens of advice-add/advice-remove. So in terms of "hackishness" I don't know who's the winner here.

To recap: I'm just trying to find a solution here, I don't mind if it's in SLY at all: but it has to be a solution.

1. What you propose for SLY isn't valid, and I've shown to you why it isn't (it makes Company and other modes slow);

2. What I propose for Helm seems to fix the problem. You tell me it's invalid - and I believe you - but you haven't told me why, i.e. you haven't told me what else it breaks. In particular, it seems you set inhibit-quit to t every time you speak with an external process (lines similar to (inhibit-quit (assoc-default 'candidates-process source)) Why is that? Again, I'm not saying this is the correct fix but the problem could be that in Helm's view it doesn't know that the underlying function is also using a process. If you unconditionally bind inhibit-quit to t in that line, the bug is gone.

[joaotavora]

Let-binding inhibit-quit to t is "dangerous" so we should have a clear and documented reason to do that each time we do it.

Indeed, that's why I stopped using it: I don't have any reason to.
And I don't think I should start just because it pleases Helm (though it does please it, for some unknown reason). Since it seems to please only Helm and no-one else, and Helm already does it quite heavily, I think Helm should do it.

Note: it's not really about returning precise results or not but about not bothering the user (because we're performing a computation that's not been explicitly requested by the user, so it's better to return incomplete results than to prompt the user "out of nowhere").

I didn't use it before, and the fact that I'm consulting it now is just out an idea. Dmitry once suggested I use it to decide whether to activate "responsive mode" in sly-eval, and I think it's a decent suggestion. The docstring says one should bind it when the user is not be disturbed and "responsive mode" is about not disturbing the user (by blocking his input).

But the while-no-input above seems redundant.

Yes I know. I was just demonstrating that this is how I think how the calls stack up when Helm is used with SLY (though Helm uses its own while-no-input.)

(while-no-input (while t (accept-process-output nil 30)))

I used this for a while and found it much less responsive than sit-for.
I think it has something to do with timers. There's a thread in emacs-devel where I discuss this with Eli.

But in this very thread, I have already experimented with that again, see here

https://github.com/joaotavora/sly/issues/303#issuecomment-579363711

It didn't fix the problem with Helm. Besides that it led to some magical magical behaviour even outside Helm.

[monnier]

Note: it's not really about returning precise results or not but about not bothering the user (because we're performing a computation that's not been explicitly requested by the user, so it's better to return incomplete results than to prompt the user "out of nowhere"). I didn't use it before, and ...

My comment above was only a reaction to what you wrote. I haven't looked at the corresponding code.

(while-no-input (while t (accept-process-output nil 30))) I used this for a while and found it much less responsive than sit-for.
I think it has something to do with timers. There's a thread in emacs-devel where I discuss this with Eli.

Sounds like a bug then. Could you make a bug report for that?

It didn't fix the problem with Helm. Besides that it led to some magical magical behaviour even outside Helm.

My crystal ball suggests disabling harry-potter-mode!?

[joaotavora]

I used this for a while

I've just experimented with this again with SLY+company and simulating a 1 sec delay on the server side. The loss in responsiveness is enormous.

Sounds like a bug then. Could you make a bug report for that?

Sure. I'll look for the thread again, but I remember Eli explaining to me that expecting it to return immediately on input isn't exactly warranted because of something about a "clean room" situation.

You can try it, don't you see a visible difference after evaluating

 (while-no-input (while t (accept-process-output nil 30)))

and typing, say, x, versus doing the same after eval'ing

(while (sit-for 30))

?

[monnier]

I used this for a while I've just experimented with this again with SLY+company and simulating a 1 sec delay on the server side. The loss in responsiveness is enormous.

Not surprised: I don't think there's been any change in this part of Emacs's C code recently.

You can try it, don't you see a visible difference after evaluating

 (while-no-input (while t (accept-process-output nil 30)))

and typing, say, x, versus doing the same after eval'ing

(while (sit-for 30))

?

No, for me both return instantly (I'm on Debian testing). (BTW, you don't need the while for that test ;-)

I do see that the accept-process-output version behaves differently w.r.t redisplay (the display is not updated while we're waiting).

    Stefan

[joaotavora]

monnier notifications@github.com writes:

No, for me both return instantly (I'm on Debian testing).

In Emacs -Q it's mostly the same, the situation deteriorates once you start processes, like Gnus, SLY, eww and stuff. sit-for doesn't suffer from this.

(BTW, you don't need the while for that test ;-)

I don't know, you might be a veeeryry slow typist :-)

I do see that the accept-process-output version behaves differently w.r.t redisplay (the display is not updated while we're waiting).

That's not a problem for me, I think. Either would be fine.

BTW, I found the Emacs bug where I talk exactly about this: it's https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32986

Here's a key exchange:

JT I would expect while-no-input to break out of accept-process-output very JT quickly after user keyboard input. These expectations are met except JT for some situations.

EZ I think your expectations are incorrect. My expectations are that if EZ you call accept-process-output with a timeout of 30 sec, then EZ while-no-input will return after 30 sec plus a small delay. It's just EZ that in order to see this in action, your experiment must be done in a EZ "clean room", and that is not easy.

Eli then goes on to validate the sit-for approach as a valid way to achieve this with as little delay as possible. This is what I use in Emacs's own lisp/jsonrpc.el and then I ported it to SLY.

João

[joaotavora]

Just to cap this off, here is a workaround/solution. Add this advice to your .emacs to get Helm+SLY working again:

(advice-add 'helm-comp-read-get-candidates :around
        (lambda (oldfun &rest args)
          (let ((inhibit-quit t))
        (apply oldfun args)))
        '((name . helm-sly-workaround)))

In my testing this fixes the problem, though I'm not sure why, as the sane value of inhibit-quit should always be nil. Helm's author has not yet commented on why this would be problematic. Anyway, the Helm repo has been archived recently and development has effectively stopped.

[Ambrevar]

I've tried the above workaround: seems to work for me indeed! I'll keep testing this for 1 day, see if breaks anything.

At first glance, it even seems to make Helm faster... Note that I've disabled Helm's cache, which might not work with this fix.

[Ambrevar]

Sadly it chokes when completing-read large lists, like describe-variable. Any other suggestion?

[joaotavora]

Maybe you can adjust the workaround to only kick in with SLY. Maybe just check sly-mode. Or investigate the root problem (which I've done too much of, and lost too much time).

[Ambrevar]

The problem with patching a function as general as helm-comp-read-get-candidates is that it applies to all completion. So even if I check that the current buffer is in sly-mrepl-mode or lisp-mode, the (inhibit-quit t) will be set when calling unrelated functions like describe-variable which then chokes.

In the end, I tried your initial hack https://github.com/emacs-helm/helm-sly/issues/2#issuecomment-689827171. Because helm--completion-in-region is a behemoth function, we can't override it easily without copy-pasting 190 lines(!) so instead I made an :around advice:

  (defun ambrevar/helm--completion-in-region (oldfun &rest args)
    (let ((joaot/helm-original-buffer (current-buffer)))
      (apply oldfun args)))
  (advice-add 'helm--completion-in-region :around
              #'ambrevar/helm--completion-in-region
              '((name . helm-sly-workaround)))

The above seems to work perfectly.

I don't think I can come up with something better without spending many more hours on it.

How shall we move forward? I can add these 2 advises to Helm-SLY and enable them with helm-sly-mode. Would that be OK with you?

[joaotavora]

I don't know if you noticed that I've since fixed this in Sly directly, so no workaround should be needed at all if you're using the latest version of SLY.

But, if I'm wrong:

• if you needed to use the inhibit-quit hack, it would have been a question of checking sly-mode in it.
• if you needed to use your alternative technique, it looks good

[Ambrevar]

I don't know if you noticed that I've since fixed this in Sly directly, so no workaround should be needed at all if you're using the latest version of SLY.

Shoot, I did miss it!

It works now indeed, thanks a lot!

The only odd thing is that now I don't almost never have the annotation anymore (the type and the percentage). I did see it once during a 2 minute test session.

Any clue? Possibly unrelated to the original issue, and it's possible that it's just me who is unfamiliar with Emacs 27 'backend completion-styles.

• if you needed to use the inhibit-quit hack, it would have been a question of checking sly-mode in it.

I did, didn't work.