project-kill-buffers and Eglot

[phikal]

Hi,

it seems that Eglot prevents project from killing a project in one go, when a server is running. The first call will kill a few buffers and the server, then the second invocation will kill the remaining buffers. Should this be fixed upstream or here?

[EDIT by @joaotavora: Continued at Emacs bug 58839: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=58839]

[joaotavora]

Hi. If you can demonstrate what you say in somw kind of reproducible example so we can really see what's happening, what preconditions and commands are involved, etc... Then I think it should be fixed here, yes. Eglot should not block other packages' functionality that have nothing to do with its mission.

…

On Sun, Jan 23, 2022, 11:44 phikal ***@***.***> wrote: Hi, it seems that Eglot prevents project from killing a project in one go, when a server is running. The first call will kill a few buffers and the server, then the second invocation will kill the remaining buffers. Should this be fixed upstream or here? — Reply to this email directly, view it on GitHub <#822>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC6PQ3FTWAC5URFULP7HRDUXPSZ5ANCNFSM5MTHWCSA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[phikal]

João Távora ***@***.***> writes:

Hi. If you can demonstrate what you say in somw kind of reproducible example so we can really see what's happening, what preconditions and commands are involved, etc... Then I think it should be fixed here, yes. Eglot should not block other packages' functionality that have nothing to do with its mission.

1. Open any Eglot-capable project 2. Open a few files (at least two) 3. Attempt to close all buffers (C-x p k runs the command project-kill-buffers) More often than not, I get an issue like this (I believe the order of opening files and enabling eglot might have something to do with it, but I haven't manged to pin-point it yet): ~~~ Kill 14 buffers in ~/src/sgo/? (y or n) y [eglot] error sending textDocument/didClose: (error Process EGLOT (sgo/c-mode) not running) [5 times] [eglot] Asking EGLOT (sgo/c-mode) politely to terminate [jsonrpc] Server exited with status 13 down-list: Process EGLOT (sgo/c-mode) not running ~~~ and this stack trace: ~~~ Debugger entered--Lisp error: (error "Process EGLOT (sgo/c-mode) not running") process-send-string(#<process EGLOT (sgo/c-mode)> "Content-Length: 123\15\n\15\n{\"jsonrpc\":\"2.0\",\"method\":\"...") #f(compiled-function (arg1 &rest rest) "Send MESSAGE, a JSON object, to CONNECTION." #<bytecode -0xa9aa719e0332102>)(#<eglot-lsp-server eglot-lsp-server-155a370eac2e> :method :textDocument/didClose :params (:textDocument (:uri "file:///home/me/src/sgo/sgo.c"))) apply(#f(compiled-function (arg1 &rest rest) "Send MESSAGE, a JSON object, to CONNECTION." #<bytecode -0xa9aa719e0332102>) #<eglot-lsp-server eglot-lsp-server-155a370eac2e> (:method :textDocument/didClose :params (:textDocument (:uri "file:///home/me/src/sgo/sgo.c")))) jsonrpc-connection-send(#<eglot-lsp-server eglot-lsp-server-155a370eac2e> :method :textDocument/didClose :params (:textDocument (:uri "file:///home/me/src/sgo/sgo.c"))) jsonrpc-notify(#<eglot-lsp-server eglot-lsp-server-155a370eac2e> :textDocument/didClose (:textDocument (:uri "file:///home/me/src/sgo/sgo.c"))) (condition-case err (jsonrpc-notify (eglot--current-server-or-lose) :textDocument/didClose (list ':textDocument (eglot--TextDocumentIdentifier))) ((debug error) (message "[eglot] error sending textDocument/didClose: %s" err) nil)) eglot--signal-textDocument/didClose() kill-buffer(#<buffer sgo.c>) project-kill-buffers() funcall-interactively(project-kill-buffers) call-interactively(project-kill-buffers nil nil) command-execute(project-kill-buffers) ~~~ And not all buffers were closed.

…

On Sun, Jan 23, 2022, 11:44 phikal ***@***.***> wrote: > Hi, > > it seems that Eglot prevents project from killing a project in one go, > when a server is running. The first call will kill a few buffers and the > server, then the second invocation will kill the remaining buffers. Should > this be fixed upstream or here? > > — > Reply to this email directly, view it on GitHub > <#822>, or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAC6PQ3FTWAC5URFULP7HRDUXPSZ5ANCNFSM5MTHWCSA> > . > Triage notifications on the go with GitHub Mobile for iOS > <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> > or Android > <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. > > You are receiving this because you are subscribed to this thread.Message > ID: ***@***.***> >

-- Philip Kaludercic

[manuel-uberti]

Out of curiosity, do you have the same problem if you kill the project buffers from Ibuffer instead of using project-kill-buffers?

[phikal]

No, ibuffer closes all buffers cleanly.

[manuel-uberti]

Interesting. I don't know if this is an Eglot problem then, because the same happens to me when I use project-kill-buffers in a Clojure project managed by CIDER.

[dgutov]

Most likely project-kill-buffers tries to kill a bunch of hidden buffers that Ibuffer doesn't show to begin with.

[yugaego]

@phikal, do you have eglot-autoshutdown enabled?
I experienced the issue you had described after enabling this option.

Just in case, before enabling eglot-autoshutdown I tested this advice worked neatly for me:

(advice-add 'project-kill-buffers
              :before
              (lambda ()
                (ignore-errors (eglot-shutdown (eglot-current-server))))
              '((name . "my-project-kill-buffers")))

[phikal]

I do, and disabling it does fix the issue at the expense of having a zombie lsp server lying around.

Just in case, before enabling eglot-autoshutdown I tested this advice worked neatly for me:

If this is the cause, it might make sense to patch this upstream, by adding something like a project-kill-buffer-hook that eglot could modify.

[dgutov]

Would it be able to do something that's not possible to implement via kill-buffer-query-functions in Eglot's own buffers? Hidden buffers, that is.

[phikal]

Now that Eglot has been merged upstream, has the situation improved to fix this issue?

[joaotavora]

Having Eglot merged has no bearing on this, but I've taken another look:

The problem is that project.el is killing Eglot's internal state buffers just because their default-directory matches the project prefix.

This is wrong. These shouldn't be killed by it:

• if eglot-autoshutdown is nil no visible bug happens, but a useless auto-reconnection has happened and the buffer is restored. In other words, project.el is trying (and failing) to defeat the user preference, but it shouldn't try to. BTW @phikal the process is not a "zombie": it's a functional server connection that will continue managing any buffer that you re-open in the project.

• If eglot-autoshutdown is t, then the reported error happens.

I see two solutions:

1. Have Eglot set default-directory differently to evade project.el's heuristic. This would probably work but is hacky and it might break stuff in the future (or at least confuse people).
2. Have Eglot tell project.el that it doesn't want that buffer killed. Maybe kill-buffer-query-functions suffices if there's a way for Eglot to ask in the function if it's being killed by project.el's project-kill-buffers function specifically.
3. Same as 3, but have the internal Eglot buffer locally set some project-dont-kill-me variable, that project.el consults.

[joaotavora]

Or maybe:

4. project.el stops killing hidden buffers. They are an implementation detail. By definition, since they are hidden, the user can't have meant to kill them with M-x project-kill-buffers, since she doesn't know about their existence.

Yeah, I think this makes the most sense.

[phikal]

João Távora ***@***.***> writes:

Or maybe: 4. `project.el` stops killing hidden buffers. They are an implementation detail. By definition, since they are hidden, the user can't have meant to kill them with `M-x project-kill-buffers`, since she doesn't know about their existence.

I don't think so, when using C-x p k I usually want to get rid of everything that Emacs has loaded that is related to a project, that would include background processes that I would expect to have killed. Perhaps `project-kill-buffers' is the wrong name for what I am thinking of, but it certainly was the intention when the command was added.

…

Yeah, I think this makes the most sense.

[joaotavora]

I don't think so, when using C-x p k I usually want to get rid of
everything that Emacs has loaded that is related to a project, that

Do you also want it to wipe out elements of file-name-history just because they too are related to a project? Probably not.

Perhaps `project-kill-buffers' is the wrong name for what I am thinking
of, but it certainly was the intention when the command was added.

For me it makes much more sense for this command to kill the buffers that would clog up C-x b and ibuffer and so on.
Eglot's hidden buffers are not a member of this set.

Also, your intended use is in very nature contrary to eglot-autoreconnect set to nil, which is the default and a very useful feature for slow-to-start servers.

So you may very well set eglot-autoreconnect to t, which you do, and if project.el stops concerning itself with internal data structures everything will work the way you want.

[phikal]

João Távora ***@***.***> writes:

> I don't think so, when using C-x p k I usually want to get rid of > everything that Emacs has loaded that is related to a project, that Do you also want it to wipe out elements of `file-name-history` just because they too are related to a project? Probably not.

No, as you imply this is obviously not what I am talking about. I was precisely trying to avoid implying this with my phrasing.

> Perhaps `project-kill-buffers' is the wrong name for what I am thinking > of, but it certainly was the intention when the command was added. For me it makes much more sense for this command to kill the buffers that would clog up `C-x b` and `ibuffer` and so on. Eglot's hidden buffers are not a member of this set.

These processes can grow to use a lot of RAM or CPU power, which is not something I want to have lying around in the background.

Also, your intended use is in very nature with having `eglot-autoreconnect` set to `nil`, which is the default and a very useful feature for slow-to-start servers. So you may very well set `eglot-autoreconnect` to `t`, which you do, and if `project.el` stops concerning itself with internal data structures everything will work the way you want.

What I think should be done is that "C-x p k" should be able to communicate with Eglot (e.g. vie a hook) and prompt the user if they want to keep the server around or not. If they decide to kill the server, it should be done before the buffers are killed so that killing a project doesn't fail midway. If they decide to keep the server, then the server buffer shouldn't be killed.

[joaotavora]

These processes can grow to use a lot of RAM or CPU power, which is not
something I want to have lying around in the background.

It's the eternal space-time tradeoff. If you prefer spending more time when
you re-visit a file set eglot-autoshutdown to t!

Everything will work exactly the way you want (so long as project.el
does what it should).

What I think should be done is that "C-x p k" should be able to
communicate with Eglot (e.g. vie a hook) and prompt the user if they
want to keep the server around or not. If they decide to kill the
server, it should be done before the buffers are killed so that killing
a project doesn't fail midway. If they decide to keep the server, then
the server buffer shouldn't be killed.

The way for Eglot to communicate to everybody (which includes project.el)
the message "don't mess with this private buffer" exists already: hidden
buffers.

[joaotavora]

If they decide to kill the
server, it should be done before the buffers are killed so that killing
a project doesn't fail midway. If they decide to keep the server, then
the server buffer shouldn't be killed.

Also, why should we bother the user with the very thing that she
has already decided in setting eglot-autoshutdown? This
second prompting seems completely unnecessary.

[phikal]

João Távora ***@***.***> writes:

> These processes can grow to use a lot of RAM or CPU power, which is not > something I want to have lying around in the background. It's the eternal space-time tradeoff. If you like prefer spending more time when you re-visit a file set `eglot-autoshutdown` to `t`! Everything will work exactly the way you want (so long as project.el does what it should).

I have that variable customised to t, but I still experience this issue. This might indicate that the behaviour I am describing is a bug, in which case I'd move the discussion from here to a new Emacs bug report.

> What I think should be done is that "C-x p k" should be able to > communicate with Eglot (e.g. vie a hook) and prompt the user if they > want to keep the server around or not. If they decide to kill the > server, it should be done before the buffers are killed so that killing > a project doesn't fail midway. If they decide to keep the server, then > the server buffer shouldn't be killed. The way for Eglot to communicate to *everybody* (which includes project.el) the message "_don't mess with this private buffer_" exists already: hidden buffers.

I am uncertain if hidden buffer are necessarily "private", as they are part of the list that `buffer-list' returns.

[joaotavora]

I have that variable customised to t, but I still experience this issue.
This might indicate that the behaviour I am describing is a bug, in

Yes: project.el is buggy in this specific aspect.

which case I'd move the discussion from here to a new Emacs bug report.

That's a good idea regardless since Eglot is now in core too.

I am uncertain if hidden buffer are necessarily "private", as they are
part of the list that `buffer-list' returns.

Sure, and mapatoms maps over all symbols, private or external. "private" doesn't mean inaccessible, just means you don't own it, so you shouldn't touch it.

[dgutov]

Perhaps you will want to introduce a major mode, or a global variable, which you would set in your "really private" buffers, and it would be consulted in project-kill-buffer-conditions.

First you will need to come to an agreement with Philip, though, about what the desired behavior will be. Myself, I'm inclined to say that if I want to close the project, no related background processes should stay around.

[joaotavora]

No, i don't want to introduce that. This is a bug in project.el. Just consider that it also breaks CIDER, and probably SLY/SLIME, or any other tool that, like Eglot, manages processes with buffers.

The manual is totally right in this: Hidden buffers (especially those without buffer-file-name) are "generally uninteresting to the user" so project.el needn't concern itself with them.

First you will need to come to an agreement with Philip, though, about what the desired behavior will be. Myself, I'm inclined to say that if I > want to close the project, no related background processes should stay around.

The desired behaviour is whatever the user chooses in eglot-autoshutdown. You can argue about the default value of that variable. I like to have it set to nil, because I often kill all by buffers of a project (with ibuffer) and then reopen a single one. But setting it to t works perfectly well if project.el didn't mess with buffers outside the view of the user and that it has nothing to do with.

There is a simple fix:

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index ac278edd40..4d4ef1ec84 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -357,9 +357,11 @@ project-buffers
   (let ((root (expand-file-name (file-name-as-directory (project-root project))))
         bufs)
     (dolist (buf (buffer-list))
-      (when (string-prefix-p root (expand-file-name
-                                   (buffer-local-value 'default-directory buf)))
-        (push buf bufs)))
+      (unless (and (string-match "^ " (buffer-name buf))
+                   (null (buffer-file-name buf)))
+        (when (string-prefix-p root (expand-file-name
+                                     (buffer-local-value 'default-directory buf)))
+          (push buf bufs))))
     (nreverse bufs)))
 
 (defgroup project-vc nil
@@ -680,11 +682,13 @@ project-buffers
          dd
          bufs)
     (dolist (buf (buffer-list))
-      (setq dd (expand-file-name (buffer-local-value 'default-directory buf)))
-      (when (and (string-prefix-p root dd)
-                 (not (cl-find-if (lambda (module) (string-prefix-p module dd))
-                                  modules)))
-        (push buf bufs)))
+      (unless (and (string-match "^ " (buffer-name buf))
+                   (null (buffer-file-name buf)))
+        (setq dd (expand-file-name (buffer-local-value 'default-directory buf)))
+        (when (and (string-prefix-p root dd)
+                   (not (cl-find-if (lambda (module) (string-prefix-p module dd))
+                                    modules)))
+          (push buf bufs))))
     (nreverse bufs)))
 
 

(It's quite simple but the duplication of code between implementations of project-buffers is a existing problem that should be also be fixed).

[dgutov]

The manual is totally right in this: Hidden buffers (especially those without buffer-file-name) are "generally uninteresting to the user" so project.el needn't concern itself with them.

From previous discussions, I think we do want to kill background/jump/utilitary buffers that service a particular project. The exceptions seem rare.

Honoring eglot-autoshutdown sounds fine, but you'd have to convey that information to project.el somehow.

Has the option to use kill-buffer-query-functions already been considered and discarded?

[joaotavora]

From previous discussions, I think we do want to kill background/jump/utilitary buffers that service a particular project. The exceptions seem rare.

Well, here's a case where we do not want the current behavior and I've not yet seen real cases where it makes sense to kill hidden buffers. So afaict it looks like it's the precisely the other way round.

Has the option to use kill-buffer-query-functions already been considered and discarded?

I go for the more generic solutions with less code first. kill-buffer-query-functions approach is per package and wouldn't fix the problem with CIDER that @manuel-uberti reported.

[joaotavora]

Continue discussion at: Emacs bug 58839. https://debbugs.gnu.org/cgi/bugreport.cgi?bug=58839