Correct LSP behaviour when changing sessions?

josh-nz commented 3 weeks ago

I've investigated the errors I'm seeing when I change sessions via :PossessionLoad as per https://github.com/jedrzejboczar/possession.nvim/pull/61#issue-2345938433.

I have tried a minimal config repoduction with just my lsp config and Possession and I can still trigger the issue, even with 0.11-nightly. The key to triggering this is to have delete_buffers = true in the Possession config. More on this later.

One issue occurs in Neovim Lua file nvim/share/nvim/runtime/lua/vim/lsp.lua in the reset_defaults function. It tries to call vim.keymap.del on the K mapping. Even though it has done a check for this keymap existing right before it deletes it, the del function errors saying it cannot find it. I still had K mapped to this function manually (from before 0.10 natively supported it). Removing this manual mapping seems to solve this issue, but there are others. I wonder if the real source of this issue is not my manual mapping, but some form of race condition, as outlined below with the other issues I'm seeing.

There are a few other issues that can occur. While the keymap issue seems to occur every time you load a session and there is already an existing session loaded, these other issues only start to occur after loading different sessions a few times. All the errors complain about an invalid buffer id.

I don't have a very detailed knowledge of how the LSP client in Neovim works, but it looks like on starting, the client registers an exit callback which is what will eventually call the K map reset and other things. It looks like to me that this might happen async, as when I log the buffers being deleted by Possession during a load, it looks like the internal Neovim code is trying to access a buffer that no longer exists. Quite why or how it's getting a reference to a non-existing buffer is beyond me, except if it was async and this is a race condition.

The last line in utils.lua delete_all_buffers (which is what is called during :PossessionLoad when the delete_buffers = true config is set), is vim.lsp.stop_client(lsp_get_clients()) and this seems to be the source of the above mentioned issues. Removing this line makes my issues go away.

This might ultimately be some bug in Neovim (it's still present in 0.11 nightly if it is), but I'm wondering what we should be doing with the LSP clients across session loads.

Here is a basic outline of existing non-session behaviour:

Load blank Neovim
Open Lua file - Lua LSP attaches to this buffer
Open Python file - Python LSP attaches to this buffer
:LspInfo shows 1 active (Python) client attached to this buffer, and 1 active (Lua) client not attached to this buffer
Switch to the Lua buffer
:LspInfo shows the same info as above but obviously the clients are swapped.
Delete Lua buffer
:LspInfo shows that the Lua client is still active even though no Lua files are open anymore.
Delete Python buffer
:LspInfo shows that the Python (and Lua) clients are still active even though no Lua or Python files are open anymore

This suggests to me that the LSP clients remain until explicitly stopped by the user, or Neovim exists. This gives us context on how to proceed when switching between sessions within the same Neovim instance.

Given the LSP client behaviour defined above, perhaps we just remove vim.lsp.stop_client(lsp_get_clients()) and do nothing. I've had a look at several other session plugins and none of them do anything with LSP clients when loading sessions. This doesn't mean they are correct of course.

There is a question of cleaning up the LSP workspaces and how this works when buffers are deleted. Maybe Neovim takes care of it for us, but it also looks like we can something like:

                if vim.lsp.buf_is_attached(buffer, client.id) then
                    vim.lsp.buf_detach_client(buffer, client.id)
                end

which might be worthwhile, but I'm unsure what it exactly does and whether it is needed. I would hope that :bd takes care of whatever is needed automatically.

I haven't been able to find much else around what the correct thing to do is, when loading sessions and LSP clients. There is however this in the LSP FAQ:

Q: How to force-reload LSP?
A: Stop all clients, then reload the buffer.
:lua vim.lsp.stop_client(vim.lsp.get_clients())
:edit

Which makes me wonder, if instead of calling vim.lsp.stop_client(lsp_get_clients()) after deleting buffers, we call the above after loading a new session (except the autoload session, since there will be no LSP clients running at this point, and we don't want to interrupt any that are starting up and make them restart).

josh-nz commented 3 weeks ago

Moving vim.lsp.stop_client(lsp_get_clients()) from delete_all_buffers to the end of load produces the same issues.

jedrzejboczar commented 3 weeks ago

I'll try to debug it when I find some time. But regarding this part of code, it has been added in this PR https://github.com/jedrzejboczar/possession.nvim/pull/7 and it may be that the case for stopping clients is no longer valid.

One think that comes to my mind is that we might want to split the stopping of LSP clients as separate "plugin" and leave "delete_all_buffers" to just delete the buffers. As for the reason to stop clients: when you have clients for some languages A, B, C running and you switch to session that uses clients C, D, E, then A, B will still be running and using your RAM - users might prefer to have a fresh start. Using LspRestart from nvim-lspconfig removes the "empty" clients but if this could be automated then it would be best.

josh-nz commented 3 weeks ago

I agree about LSPs hanging around in RAM after a session change when the new session doesn't use these LSPs. My point was that this seems to be the standard Neovim behaviour that I've noticed, the clients still hang around when deleting buffers. It seems on the user to clean up the LSPs. So removing it doesn't break any existing Neovim behaviour. Any clean up option is a nice-to-have feature.

Splitting it out as a separate plugin might help get around these issues. I'm not sure how you'd have it run automatically though, that would seem then you're back to the current issues.

In reference to some of the comments in #7, it does seem like anything that responds to vim.schedule (which sounds async, or at least deferred to a future time) should be checking if the buffer still exists before doing anything with it. So there might be some Neovim core bugs here. I'm unsure of the expectations of this behaviour, and the LSP client.

jedrzejboczar commented 3 weeks ago

I think this is a bug in core, as you say, anything that uses vim.schedule should not assume that buffers are still valid.

I have just experienced a very similar bug, but unrelated to possession.nvim - one of my LSPs died due to some external changes to a file and then I got

 Error executing vim.schedule lua callback: /usr/share/nvim/runtime/lua/vim/lsp.lua:380: Error executing lua: vim/keymap.lua:0: E31: No such mapping 
stack traceback:
    [C]: in function 'nvim_buf_del_keymap'
    vim/keymap.lua: in function 'del'
    /usr/share/nvim/runtime/lua/vim/lsp.lua:383: in function </usr/share/nvim/runtime/lua/vim/lsp.lua:380>
    [C]: in function 'nvim_buf_call'
    /usr/share/nvim/runtime/lua/vim/lsp.lua:380: in function 'reset_defaults'
    /usr/share/nvim/runtime/lua/vim/lsp.lua:407: in function </usr/share/nvim/runtime/lua/vim/lsp.lua:394>
stack traceback:
    [C]: in function 'nvim_buf_call'
    /usr/share/nvim/runtime/lua/vim/lsp.lua:380: in function 'reset_defaults'
    /usr/share/nvim/runtime/lua/vim/lsp.lua:407: in function </usr/share/nvim/runtime/lua/vim/lsp.lua:394>

josh-nz commented 3 weeks ago

This looks like my original issue. Do you have K manually mapped to vim.lsp.buf.hover by chance?

thecontinium commented 1 week ago

I was getting these invalid buffer id type of errors until I upgraded to nightly NVIM v0.11.0-dev-3473+ge7020306a-Homebrewbuilt yesterday. It's better for me but this version doesn't fix it completely.

thecontinium commented 1 week ago

I’m in favour of stopping the LSP clients in a separate "plugin" and leave "delete_all_buffers" to just delete the buffers.

The option to turn on the stopping of the lsp clients can then be solved as a separate issue whilst having the delete_all_buffers working as well as any other session management plugin.

josh-nz commented 1 week ago

The nvim_buf_del_keymap error looks like it was a Neovim bug and is fixed here:

https://github.com/neovim/neovim/pull/29478

And I found a few that look like they address the invalid buffer id (specifically https://github.com/neovim/neovim/pull/29029 but there are a few other related fixes), so as mentioned by @thecontinium these issues regarding stopping/reseting the LSP clients look to be solved. Currently in nightly, but the issues were assigned the 0.10.1 milestone for general availability.

So I think this issue now becomes: move the stopping of the LSP clients to a separate plugin.

I'll see if I can find some time to have a try at this in the next few days.

jedrzejboczar / possession.nvim

Correct LSP behaviour when changing sessions? #66