Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LSP: Allow configuration to control processId #14504

Closed
adrianord opened this issue May 6, 2021 · 7 comments
Closed

LSP: Allow configuration to control processId #14504

adrianord opened this issue May 6, 2021 · 7 comments
Labels
enhancement feature request lsp

Comments

@adrianord
Copy link

When running LSPs in a container that were made with vscode-languageserver-node, the processId is passed to the server and is periodically checked. If the PID passed to the language server no longer exists, the language server kills itself. This causes the language server to exit 3 seconds after neovim initializes the connection due to neovim's process not existing in the container.

The fix for this is to pass processId as null to the language server. This turns off checks that the client process is still running.

This is the current work around in neovim

vim.loop.getpid = function() return nil end

This will then pass nil to processId when it reaches the initialize_params here (uv being vim.loop)

local initialize_params = {
-- The process Id of the parent process that started the server. Is null if
-- the process has not been started by another process. If the parent
-- process is not alive then the server should exit (see exit notification)
-- its process.
processId = uv.getpid();
-- Information about the client
-- since 3.15.0
clientInfo = {
name = "Neovim",
version = string.format("%s.%s.%s", version.major, version.minor, version.patch)
};
-- The rootPath of the workspace. Is null if no folder is open.
--
-- @deprecated in favour of rootUri.
rootPath = config.root_dir;
-- The rootUri of the workspace. Is null if no folder is open. If both
-- `rootPath` and `rootUri` are set `rootUri` wins.
rootUri = vim.uri_from_fname(config.root_dir);
-- User provided initialization options.
initializationOptions = config.init_options;
-- The capabilities provided by the client (editor or tool)
capabilities = config.capabilities or protocol.make_client_capabilities();
-- The initial trace setting. If omitted trace is disabled ("off").
-- trace = "off" | "messages" | "verbose";
trace = valid_traces[config.trace] or 'off';
-- The workspace folders configured in the client when the server starts.
-- This property is only available if the client supports workspace folders.
-- It can be `null` if the client supports workspace folders but none are
-- configured.
--
-- Since 3.6.0
-- workspaceFolders?: WorkspaceFolder[] | null;
-- export interface WorkspaceFolder {
-- -- The associated URI for this workspace folder.
-- uri
-- -- The name of the workspace folder. Used to refer to this
-- -- workspace folder in the user interface.
-- name
-- }
workspaceFolders = {{
uri = vim.uri_from_fname(config.root_dir);
name = string.format("%s", config.root_dir);
}};
}

This seems like a rather dangerous workaround depending on how getpid is used in other parts of neovim.

I would rather be able to set the processId initialization_param like I would capabilities or rootUri.

@adrianord adrianord added the enhancement feature request label May 6, 2021
@vigoux vigoux added the lsp label May 6, 2021
@mjlbach
Copy link
Contributor

mjlbach commented May 6, 2021

I'd recommend running the container with --pid=host, I think this is much simpler than adding a configurable workaround.

@adrianord
Copy link
Author

adrianord commented May 6, 2021

Thank you for the suggestion, while this might have worked for a regular linux distro running docker(I can try this too if you'd like, I have pop_os on my laptop), this does not work for WSL2 since Docker Desktop runs docker as a separate distro in WSL2, which gives it it's own pid namespace.

Edit: this is after testing on a Windows desktop running WSL2, forgot to mention that.

@adrianord
Copy link
Author

I'd also like to point out that setting processId to null is the recommended way to avoid client process checks by the team that wrote vscode-languageserver-node

microsoft/vscode-languageserver-node#364 (comment)

@mjlbach
Copy link
Contributor

mjlbach commented May 6, 2021

Right, my suggestion was to enable the client process check, but just share the host process namespace with the container which should work on linux.

For your use case, you can use before_init to override initialize_params.process_id to vim.NIL. So this feature request is already implemented :)

if config.before_init then
-- TODO(ashkan) handle errors here.
pcall(config.before_init, initialize_params, config)
end

@adrianord
Copy link
Author

Works perfect, thank you very much for your help! 😊

@neovim-discourse
Copy link

This issue has been mentioned on Neovim Discourse. There might be relevant details there:

https://neovim.discourse.group/t/please-help-trying-to-use-dockerls-as-a-podman-container/1265/1

@neovim-discourse
Copy link

This issue has been mentioned on Neovim Discourse. There might be relevant details there:

https://neovim.discourse.group/t/solved-please-help-trying-to-use-dockerls-as-a-podman-container/1265/3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature request lsp
Projects
None yet
Development

No branches or pull requests

4 participants