Lsp restart command spawn multiple procces for every call

[axdank]

Summary

The new lsp restart command does not shut down the current server, so one process is left hanging, and a new one is executed.
Opening the editor for the first time: 3 processes

Running lsp-restart for the first time: 6 processes

Running lsp-restart for the second time: 9 processes

Reproduction Steps

1. Open helix
2. Open the rust project and run the lsp-restart command once.
3. run lsp-restart a second time

Helix log

not relevant

Platform

Windows

Terminal Emulator

wezterm 20220908-191626-25cd05a8

Helix Version

helix 22.08.1 (e8f0886)

[dead10ck]

Interesting, this must be a Windows specific bug. This did not happen during testing on Linux.

[kirawi]

It's not clear to me where the old process is dropped in #3435.

[dead10ck]

It's not clear to me where the old process is dropped in #3435.

This was discussed in the PR: #3435 (comment)

It gets killed when the client gets dropped, which should happen here.

[dead10ck]

cc @mangas

[axdank]

For the moment I take the old client returned by entry.insert and call its shutdown method, which worked for me since it closes the old process, I think that in windows the kill_on_drop(true) function fails or has no effect.

                let NewClientResult(client, incoming) = start_client(id, language_config, config)?;
                self.incoming.push(UnboundedReceiverStream::new(incoming));

                let (_, old_client) = entry.insert((id, client.clone()));
                
                tokio::spawn(async move {
                    if let Err(e) = old_client.shutdown_and_exit().await {
                        log::error!("failed shutdown language server: {}", e);
                        return;
                    }
                });
                
                Ok(Some(client))

[kirawi]

According to the tokio documentation:

On Unix platforms processes must be “reaped” by their parent process after they have exited in order to release all OS resources. A child process which has exited, but has not yet been reaped by its parent is considered a “zombie” process. Such processes continue to count against limits imposed by the system, and having too many zombie processes present can prevent additional processes from being spawned.

Although issuing a kill signal to the child process is a synchronous operation, the resulting zombie process cannot be .awaited inside of the destructor to avoid blocking other tasks. The tokio runtime will, on a best-effort basis, attempt to reap and clean up such processes in the background, but makes no additional guarantees are made with regards how quickly or how often this procedure will take place.

If stronger guarantees are required, it is recommended to avoid dropping a Child handle where possible, and instead utilize child.wait().await or child.kill().await where possible.

[dead10ck]

Nice find. We should probably add a kill method to our LSP client then and incorporate it into the normal shutdown process. This is relevant for #3482. According to the LSP spec, the normal process is to send a shutdown request, and then a exit request, which it looks like we do:

helix/helix-lsp/src/client.rs

Lines 394 to 400 in 98dd9c4

	/// Forcefully shuts down the language server ignoring any errors.
	pub async fn force_shutdown(&self) -> Result<()> {
	if let Err(e) = self.shutdown().await {
	log::warn!("language server failed to terminate gracefully - {}", e);
	}
	self.exit().await
	}

So it looks like normally, the process should exit on its own after these two requests. However, it's conspicuously absent from the spec what should happen if the shutdown request fails. Probably what we can do is check the child process after the exit request and kill if it it's still alive for whatever reason.

[axdank]

After looking at a lot of my code and searching for the problem, I overlooked that the branch from which I use the helix executable had another branch merged into it, specifically #2653, which I believe keeps the old_client active. By testing the clean master branch, I was able to verify that the old process is definitely removed. 😕😄