Ignore errors when shutting down all servers

edkolev commented 1 year ago

eglot-shutdown-all has a bug that results in partially stopping servers - when a server shutdown timeouts (1.5s), an error is propagated and eglot-shutdown-all does not try to shutdown the rest of the servers.

eglot.el (eglot-shutdown-all): Ignore errors when shutting down servers, converting errors to messages.

edkolev commented 1 year ago

Comparison of old VS new behaviour is below.

Steps to reproduce:

start 2 different language servers, the servers should be slow to stop (I use gopls in a monorepo, so slow startup time is guaranteed)
(optional) if your language server is not slow to stop, change the timeout in eglot-shutdown from 1.5 sec to 0.1 sec to force a timeout
run eglot-shutdown-all

Before: only one of the 2 servers is stopped - 1st one times out, 2nd one is not asked to stop at all. Messages:

[eglot] Asking EGLOT (project1/go-mode) politely to terminate
[jsonrpc] Server exited with status 9
down-list: jsonrpc-error: "request id=4 failed:", (jsonrpc-error-message . "Timed out")

After: both servers are stopped, the timeout errors are converted to messages by with-demoted-errors. Messages:

[eglot] Asking EGLOT (project1/go-mode) politely to terminate
[jsonrpc] Server exited with status 9
[eglot] shutdown all: (jsonrpc-error request id=2 failed: (jsonrpc-error-message . Timed out))
[eglot] Asking EGLOT (project2/go-mode) politely to terminate
[jsonrpc] Server exited with status 9
[eglot] shutdown all: (jsonrpc-error request id=2 failed: (jsonrpc-error-message . Timed out))

edkolev commented 1 year ago

Should I convert this PR to an emacs patch and send it to bug-gnu-emacs@gnu.org?

joaotavora commented 1 year ago

Thanks, this makes sense and the approach is reasonable. But please submit this as a patch to Emacs's lisp/progmodes/eglot.el sending email to bug-gnu-emacs@gnu.org with my email in CC.

I'd prefer if you put the with-demoted-errors outside the inner loop, seems slightly neater.

edkolev commented 1 year ago

Sure, I'll submit a patch.

Do I understand correctly your suggestion: put with-demoted-errors between the 2 loops:

(defun eglot-shutdown-all (&optional preserve-buffers)
  "Politely ask all language servers to quit, in order.
PRESERVE-BUFFERS as in `eglot-shutdown', which see."
  (interactive (list current-prefix-arg))
  (cl-loop for ss being the hash-values of eglot--servers-by-project
           do (with-demoted-errors
                  "[eglot] shutdown all: %s"
                (cl-loop for s in ss do (eglot-shutdown s nil preserve-buffers)))))

joaotavora commented 1 year ago

Yes, that's it, and you can kill one of the newlines. Do you have an FSF copyright assignment? Your patch can go in regardless, but it's good to get one of you plan on working more on Eglot and Emacs.

edkolev commented 1 year ago

Patch is now submitted https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-11/msg00658.html

I've signed the FSF copyright assignment some time ago.

edkolev commented 1 year ago

The patch has been merged, thank you!

Commit: https://github.com/emacs-mirror/emacs/commit/c833b291f5

joaotavora / eglot

Ignore errors when shutting down all servers #1098