Cc: app-emacs-dev@janestreet.com
Date: Sat, 4 May 2024 04:09:48 +0300
From: Dmitry Gutov <dmitry@gutov.dev>
Hi Spencer,
On 02/05/2024 22:37, Spencer Baugh wrote:
>
> In some project /home/foo/proj, with pretty much any LSP server:
>
> 1. In /home/foo/proj, M-x eglot, starting some LSP server
>
> 2. Delete the directory /home/foo/proj
>
> 3. The LSP server will crash/exit
>
> 4. The process sentinel for the server will run, running
> eglot--on-shutdown which by default will call eglot-reconnect
>
> 5. eglot-reconnect extracts the saved project instance out of the
> server, which has a root directory which no longer exists, and calls
> eglot--connect with it
>
> 6. eglot--connect calls project-name on a nonexistent project instance,
> which may fail with an error depending on the project implementation
> (I have a custom project implementation, but I think this can happen
> with project-vc too)
>
> 7. This causes the process sentinel to error.
>
> I think the right fix is probably for eglot--on-shutdown (or maybe
> eglot-reconnect) to call (project-current nil (project-root pr)) to find
> the new project instance. If that returns nil, the project has
> disappeared, and eglot should just not try to reconnect. This also
> would make eglot behave better if the project layout changes (e.g. if
> there are nested projects).
I think I like this solution (as long as the nil value returned by
project-current on this step is appropriately handled).
Something like:
diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 6896baf30ce..7b2461c3ce6 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1426,11 +1426,15 @@ eglot-reconnect
(interactive (list (eglot--current-server-or-lose) t))
(when (jsonrpc-running-p server)
(ignore-errors (eglot-shutdown server interactive nil
'preserve-buffers)))
- (eglot--connect (eglot--major-modes server)
- (eglot--project server)
- (eieio-object-class-name server)
- (eglot--saved-initargs server)
- (eglot--language-ids server))
+ (let* ((root (project-root (eglot--project server)))
+ (project (project-current nil root)))
+ (if (not project)
+ (eglot--error "Project in `%s' is gone!" root)
+ (eglot--connect (eglot--major-modes server)
+ project
+ (eieio-object-class-name server)
+ (eglot--saved-initargs server)
+ (eglot--language-ids server))))
(eglot--message "Reconnected!"))
(defvar eglot--managed-mode) ; forward decl
Though it also raises a question about the caching strategy for VC-Aware
project backend. At the moment is associates a project with a directory
more or less indefinitely, and this is a case to watch out for.
> Alternatively, maybe eglot--on-shutdown shouldn't automatically
> reconnect in the first place? Maybe reconnection should happen
> automatically only when some specific buffer tries to interact with the
> LSP - then it can run project-current in the context of that specific
> buffer, and see there's no project, and fail. Plus, if the user kills
> all the buffers in the project (possibly with project-kill-buffers)
> before deleting it, this approach would entirely avoid the unnecessary
> eglot reconnection attempt.
This also sounds good, though it'd probably require more changes
overall. Additionally, perhaps I'd change the association from (server
-> project) to (server -> project-root), relying on the project
backends' internal caches to fetch the project value whenever it's
needed. That might be the most reliable approach. Perhaps the slowest in
theory, but hopefully not noticeably so.