Workspaces not added to the server again when initially removed

[asmodeus812]

Description

When removing a workspace from a running client using the built vim.buf.remove_workspace_folder, then when force exploring a buffer from the removed workspace again, the workspace is not added back, the server is not notified. This is relevant only for servers with multi workspace support.

function M.remove_workspace_folder(workspace_folder)
  workspace_folder = workspace_folder
    or npcall(vim.fn.input, 'Workspace Folder: ', vim.fn.expand('%:p:h'))
  api.nvim_command('redraw')
  if not (workspace_folder and #workspace_folder > 0) then
    return
  end
  local params = util.make_workspace_params(
    { {} },
    { { uri = vim.uri_from_fname(workspace_folder), name = workspace_folder } }
  )
  for _, client in pairs(vim.lsp.get_active_clients({ bufnr = 0 })) do
    for idx, folder in pairs(client.workspace_folders or {}) do
      if folder.name == workspace_folder then
        vim.lsp.buf_notify(0, 'workspace/didChangeWorkspaceFolders', params)
        client.workspace_folders[idx] = nil
        return
      end
    end
  end
  print(workspace_folder, 'is not currently part of the workspace')
end

Implementation of the remove function as of neovim 0.9.4 above. As you can see on top of sending notify to the server it clears the workspace folders, however, when force exploring a buffer belonging to the removed workspace (as resolved by root_dir), the server is neither notified nor is the workspace_folder added back in.

[justinmk]

Thanks for your report. The configs in this repo are unsupported and provided only as a starting point. We depend on users (like you) to troubleshoot issues with their specific LSP setups and send improvements.

If you found a bug in the core Nvim vim.lsp module (not part of this repo), the best way to get it fixed is to report to Nvim (not nvim-lspconfig) with:

• steps to reproduce it without the particular LSP server and other factors particular to your environment
• (optional, but very helpful): by adding a failing test case to lsp_spec.lua, which has code to setup a fake LSP server to simulate various scenarios

[asmodeus812]

@justinmk i do not think this issue is part of the core, the core has these specific utility functions, but the core does not handle workspace configurations. This specific plugin does, in the implementation, there is a specific case where the server is checked if it supports multiple workspaces, and instead of spawning additional servers it re-uses existing ones. Please re-open this issue as it affects this specific plugin and not core.

[asmodeus812]

@justinmk -

nvim-lspconfig/lua/lspconfig/manager.lua

Line 92 in 41f40dc

function M:_register_workspace_folders(bufnr, root_dir, client)

[glepnir]

provide reproduce step i will take a look at it.

[asmodeus812]

1. Make sure workspace support is on for lua ls
2. Open a file from a plugin A
3. Open another file from another plugin B
4. Both roots of the plugins are registered as workspace folders in the lua ls client.
5. Delete / wipe out all buffers from one of the workspaces / plugins - A or B
6. Manually remove the workspace from the client.workspace _folders and send notify ro the server for the removed workspace. (I Used as an example part of the impl from the core's utility remove_workspace function)
7. Explore / edit one of the deleted buffers again, from the removed workspace
8. Workspace belonging to that file/buffer is not added again

Furthermore, it might be a good idea to have step 6 be handled internally by lspconfig. As it is already responsible for understanding how to add new workspaces, it can intercept if all buffers of a given workspace for a client are deleted and detach the workspace folder and do a notify to the client for workspace removed ( the inverse of what it already does )

[glepnir]

I will check it later.

[asmodeus812]

@glepnir, thanks, there is some work being done here by @arenevier (but it is incomplete) - #2837. But This is very much related to my issue and in general to the clean up of clients.

In general lspconfig does not correctly consider how to correctly clean up clients, that it starts and manages. We have two main use cases.

• when a client is started without workspace support, when the last ever buffer is closed for that client, the client has to be terminated.
• when a client is started with workspace support, then upon closing the last buffer for a workspace, the workspace is de-registered, any state held for it removed from the client etc. (my issue), upon closing the last buffer for the sole workspace currently registered, then we clean up the client too - this is the base case from above

[glepnir]

when a client is started without workspace support, when the last ever buffer is closed for that client, the client has to be terminated.

If the problem is due to some auto-shutdown server plugin maybe nothing todo here? We do not handle automatically shutting down the server when no buffer is attached. there has an issue track in neovim core.

[asmodeus812]

@glepnir i do not understand what auto-shutdown plugin means, the argument here is that whatever resources lspconfig manages, in this case running language server clients, it should be capable of cleaning them up too.

Leaving any of this to core, is not a good idea, core does not manage servers, neither does it decide when to start them etc, it just provides an api to work with clients/servers. However the workspace issue is still a real issue solely affecting lspconfig, since lspconfig is managing them i.e sending addWorkspace, therefore it should removeWorkspace, otherwise it leaves the client in a inconsistent state in comparison with the editor.

Lspconfig has the most information and context on what was started, for which root directories, how it was started, and so much more, and therefore it has a much better idea on how/when to cleanup the started client resources, this can't be done from a separate plugin, which has none of this context or information, let alone core.

[glepnir]

when a client is started without workspace support, when the last ever buffer is closed for that client, the client has to be terminated.

again in this regard, lspconfig can do is provide api or command i think . lspconfig will not shut down the server. Because some servers cache large amounts of content. You may use it again in tens of seconds or minutes. We cannot decide when to shut down the server. relate neovim/neovim#14430

[asmodeus812]

@glepnir this is a false assumption, probably even wrong. When server is started without workspace support the client is initialized for specific workspace, if the user closes all buffers for a workspace, it is very much expected that all resources relating to that workspace are correctly freed up, if i open a new file from another workspace it will not attach to the same client instance anyway, it will create a new one for the new workspace.

You may use it again in tens of seconds or minutes - there is no reason to close ALL buffers for the workspace if we want to use it again, closing one or more buffers provides no benefit at all, that was the entire argument for this issue in the first place (having all resources for a workspace freed up in the editor, including the buffers, by the user, implies something, imo).

Not to mention the auto-clean up can be accompanied by a configuration the user might opt into, further more an additional user input might be taken in to accept cleaning the client (again a config opt-in). Not to mention again, the pr above, even adds a timeout before it kills the client, as a response to your argument: You may use it again in tens of seconds or minutes

The only possible reason when an auto cleanup is not desired is if you have set nohidden, but by default nvim has set hidden.

In any case, i did take a look at the issue and as i said people there also are reluctant to put this into core, which is correct, we do not want this in core at all. neovim/neovim#14430 (comment).

In My use case for example, where i often open 10s of projects/workspaces in a single nvim instance, and want to kill buffers for a specific workspace, i do not want rogue clients taking up resources, staying on. I am sure you are familiar that several language servers like for example TS, can easily eat up a ton of ram, and often people resort to restarting them frequently.

Freeing up any amounts of buffers for a workspace, resource wise, and leaving the client running, will never outweigh having the client taking up who knows how much system resources (given it often ranges in the GBs of memory)

As a final thought, as i already said, my most important concern is to not leave the editor and any externally managed resources out of sync.

We are talking about having the auto-shutdown on BufUnload, bufwipeout, there are nuances on what buffer delete means, what i refer to in the original issue, is complete free up of the buffers.

[justinmk]

i do not think this issue is part of the core, the core has these specific utility functions, but the core does not handle workspace configurations.

That may be the case temporarily, but given all the discussion in this issue, this is clearly something that needs to be solved in core, not in nvim-lspconfig. nvim-lspconfig is not supposed to have complex features, it's supposed to be only a collection of configurations.

[asmodeus812]

@justinmk, if these specific features like multi workspace support and server management are going to be moved into core, then yes, i am okay with that. But i did not expect that was the intention in the first place. To leave only the config collections in lspconfig. I am okay as long as these lsp features do not end up split between the core and a plugin. It should be either - or

[mfussenegger]

, if the user closes all buffers for a workspace, it is very much expected that all resources relating to that workspace are correctly freed up,

This is something you might expect. You'll find just as many people who do not want that, because initializing language servers can be expensive, and you may not want to pay that price just because you closed the last buffer and re-opened another one. IDEs typically also don't do that, they let you explicitly close a project instead.

The only possible reason when an auto cleanup is not desired is if you have set nohidden, but by default nvim has set hidden.

This is again your view. Your preferences != preferences of everybody.

That may be the case temporarily, but given all the discussion in this issue, this is clearly something that needs to be solved in core

If neovim ever gets projects primitives (neovim/neovim#8610) we should look into how to align the LSP features with it, but until that's in - I'd rather not have any heuristics or automatism regarding starting/stopping of language servers built-in in core but keep that delegating that to users via the lsp.start/start_client functions.
It just opens up a can of worms that we've seen happening in this repo (like needing an option to not start clients on certain conditions, to not attach them to certain buffers and so on)

What I'd already see in scope of core without that:

• A LspStop command, to have that available without nvim-lspconfig
• Some replacement for the existing workspace folder methods to make them work better with multiple clients and extending the workspace folders could even be integrated into lsp.start in combination with the reuse_client predicate it already has (if it returns true and the root_dir is different, automatically extend/merge workspace folders)

Given that lspconfig already contains some workspace logic attachment logic (in addition to configuration), I'd agree with this issues initial point that the re-attachment should currently also be handled in nvim-lspconfig.

[asmodeus812]

@mfussenegger cherry picking quotes does not magically make your argument somehow true, or mean the current / original behavior is somehow the one true way to go. As i have suggested above, this can be opt-in just as autostart is optin. The fact that the spawning of clients is managed by this plugin automatically by deafult, but the equally important management & clean-up does not even exist is unacceptable. Above, i suggested a heuristics to be the exact opposite of how autostart works, atm. But it can be a number of other things. Since lspconfig can auto start, it should know how to autokill - when opted in

What i suggest is to provide the user with an ability to choose. Mike Acton had a great quote once, taken a bit out of context, from a presentation, but the essence of it went something along the lines of "... people like you are why i wait 30 minutes for Word to start. "

The software world has completely forgone any considerations about managing resources, performance, and general cleanliness.

I have implemented the autoclose in my config, yet it is just not even close to what needs to be done to correctly ensure proper resource auto cleanup. As i mentioned above, users outside lspconfig have very little context of the state, which causes some things to be either impossible to implement outside lspconfig or downright ugly workarounds - leading to decoupled inconsistent and incoherent state of the editor and the resources it manages.

[mfussenegger]

cherry picking quotes does not magically make your argument somehow true,

And what argument is that? I merely pointed out that these matter of fact statements are subjective preferences.
I actually agreed with you that something about this should be done in lspconfig:

Given that lspconfig already contains some workspace logic attachment logic (in addition to configuration), I'd agree with this issues initial point that the re-attachment should currently also be handled in nvim-lspconfig.