kickstart.nvim icon indicating copy to clipboard operation
kickstart.nvim copied to clipboard

Fix highlight errors when lsp crash or stop

Open sudo-tee opened this issue 1 year ago • 4 comments

It adds a check wether the client is still available before highlighting.

If the client is not there anymore it returns true to unregister the autocommand

This fix the method textDocument/documentHighlight is not supported by any of the servers registered for the current buffer errors when doing a LspRestart or the server crashes

sudo-tee avatar Apr 19 '24 12:04 sudo-tee

Please forgive my ignorance here but will this fix negatively affect people with different LSP configurations etc?

feoh avatar Apr 20 '24 17:04 feoh

How do you test or reproduce this error? I tested :LspRestart and :LspStop and :LspStart and have not seen any errors and the behaviour worked as expected. My observed behaviour:

  • on LspRestart the highlights blinked for a moment and came back
  • on LspStop the highlights disappear on cursor move
  • on LspStart the highlights came back

These cases seem to work normally. But I only tested with lua, perhaps some other LSP behaves differently. I don't know what happens in case of LSP crash or how to reproduce it, but I suspect in such a case the highlights will not be the only issue? Perhaps if LSP crashes the best is to just restart nvim, since it will not behave as expected? In any case I think we need to understand more details about this.

dam9000 avatar Apr 20 '24 23:04 dam9000

I have a pretty unstable LSP (tsserver) on a couple projects. That requires me to frequently restart it. When the server crash I receive a load of errors method textDocument/documentHighlight is not supported by any of the servers registered for the current buffer

Theses errors are probably not an issue by themselves because they are silent, but I'm using Noice and nvim-notify so they are pretty noisy.

The issue is that the more I restart the LSP the more errors notifications I get, tsserver sometimes require 3 restart in a row. They only go away if a use the :e command or close nvim.

This led me to believe that there might be a small memory leak as the more you restart the lsp the more autocmds are created.

This might be an edge case but doing this trick completely resolved my issues and I can restart the LSP and continue working. I thought I'd share through this pull request.

sudo-tee avatar Apr 21 '24 13:04 sudo-tee

Hmm I wonder if a more proper approach would be to listen to the LspDetach event and then call vim.lsp.buf.clear_references and unregister all Cursor* autocmds? Does the LspDetach event get triggered when the LSP crashes? Also, have you submitted a bug report on tsserver crash or perhaps it's already a known issue?

dam9000 avatar Apr 21 '24 13:04 dam9000

It's is a good idea and it seems to work properly by removing the command in the LspDetatch. As per the crash I have to wait until it happens to see if the error is still there, but with this fix LspRestart will clear the commands the error will automatically be cleared as well. I would say it is not really an issue anymore.

I updated the PR to remove the the commands in the LspDetatch event like you suggested.

sudo-tee avatar Apr 22 '24 12:04 sudo-tee

@sudo-tee It's looking much better now. I think it can be simplified further by using nvim_clear_autocmds like this:

diff --git a/init.lua b/init.lua
index f587aec..50e56d5 100644
--- a/init.lua
+++ b/init.lua
@@ -544,10 +544,7 @@ require('lazy').setup({
         group = vim.api.nvim_create_augroup('kickstart-lsp-detach', { clear = true }),
         callback = function(event)
           vim.lsp.buf.clear_references()
-          local cmds = vim.api.nvim_get_autocmds { group = 'kickstart-lsp-highlight', buffer = event.buf }
-          for _, cmd in ipairs(cmds) do
-            vim.api.nvim_del_autocmd(cmd.id)
-          end
+          vim.api.nvim_clear_autocmds { group = 'kickstart-lsp-highlight', buffer = event.buf }
         end,
       })

If you agree that this works and is cleaner go ahead and integrate it and I think we have a final solution.

dam9000 avatar Apr 22 '24 14:04 dam9000

@dam9000

I confirm that it works with the vim.api.nvim_clear_autocmds. Thanks a lot

I updated the PR to use this method instead.

Thanks lot for your help on the issue :)

sudo-tee avatar Apr 22 '24 14:04 sudo-tee

Thanks for working towards the best solution on this everyone!

feoh avatar Apr 22 '24 15:04 feoh