From 047ef00e949002cdd7d7ef30e3e3c33ea9e8e562 Mon Sep 17 00:00:00 2001 From: max ulidtko Date: Tue, 4 May 2021 03:23:26 +0300 Subject: [PATCH 1/4] Fix terminated LS processes turning into zombies [DRAFT] --- src/language_server_protocol.rs | 54 +++++++++++++++++++++++++-------- src/rpcclient.rs | 8 +++-- 2 files changed, 47 insertions(+), 15 deletions(-) diff --git a/src/language_server_protocol.rs b/src/language_server_protocol.rs index e3a319556..e2f033b2d 100644 --- a/src/language_server_protocol.rs +++ b/src/language_server_protocol.rs @@ -3610,7 +3610,7 @@ impl LanguageClient { Ok(()) })?; - let (child_id, reader, writer): (_, Box, Box) = + let (child, reader, writer): (_, Box, Box) = if command.get(0).map(|c| c.starts_with("tcp://")) == Some(true) { let addr = command .get(0) @@ -3642,7 +3642,7 @@ impl LanguageClient { None => Stdio::null(), }; - let process = std::process::Command::new( + let mut process = std::process::Command::new( command.get(0).ok_or_else(|| anyhow!("Empty command!"))?, ) .args(&command[1..]) @@ -3653,7 +3653,6 @@ impl LanguageClient { .spawn() .with_context(|| format!("Failed to start language server ({:?})", command))?; - let child_id = Some(process.id()); let reader = Box::new(BufReader::new( process .stdout @@ -3664,7 +3663,9 @@ impl LanguageClient { .stdin .ok_or_else(|| anyhow!("Failed to get subprocess stdin"))?, )); - (child_id, reader, writer) + process.stdout = None; + process.stdin = None; + (Some(process), reader, writer) }; let lcn = self.clone(); @@ -3678,7 +3679,7 @@ impl LanguageClient { Some(language_id.clone()), reader, writer, - child_id, + child, self.get_state(|state| state.tx.clone())?, on_server_crash, )?; @@ -3722,10 +3723,40 @@ impl LanguageClient { if language_id.is_none() { return Ok(()); } + let lang_id_real = language_id.clone().unwrap(); - // we don't want to restart if the server was shut down by the user, so check - // VIM_IS_SERVER_RUNNING as that should be true at this point only if the server exited - // unexpectedly. + // avoid any racing of this cleanup with new client creation + let client_update_mutex = self.get_client_update_mutex(language_id.clone())?; + let _per_langid_client_lock = client_update_mutex.lock().map_err(|err| + anyhow!("Failed to lock cleanup of client for languageId {:?}: {:?}", + lang_id_real, err))?; + + // must read out the child process' exit code -- lest it'll turn into a zombie! + self.update_state(|state| { + let client_ref = match state.clients.remove(language_id) { + Some(arc) => Ok(arc), + None => Err(anyhow!("Expected to have an RpcClient for {}, found None", lang_id_real)), + }?; + let mut client = Arc::try_unwrap(client_ref) + .map_err(|_| anyhow!("Arc::try_unwrap() unsuccessful return")) + .context("More than 1 reference to RpcClient")?; + if let Some(child) = client.child_process.as_mut() { + match child.try_wait() { + Ok(Some(exitcode)) => info!("Server process exited with {}", exitcode), + Ok(None) => warn!("No exitcode available, this should never happen."), + Err(e) => error!("Process wait failed: {}", e), + } + } + client.child_process = None; + state.clients.insert(language_id.clone(), Arc::new(client)); + Ok(()) + }) + .map_err(|err| anyhow!( + "Could not cleanup leftover process for langId {:?}, leaving a zombie. {:?}", + language_id.clone().unwrap(), err))?; + + // next we handle restarting the server -- unless of course it was intentionally + // shut down by the user, in which case VIM_IS_SERVER_RUNNING will be unset. let filename = self.vim()?.get_filename(&Value::Null)?; let is_running: u8 = self .vim()? @@ -3772,9 +3803,9 @@ impl LanguageClient { self.vim()?.echoerr("Server crashed, restarting client")?; std::thread::sleep(Duration::from_millis(300 * (restarts as u64).pow(2))); - self.start_server(&json!({"languageId": language_id.clone().unwrap()}))?; + self.start_server(&json!({"languageId": lang_id_real}))?; self.text_document_did_open(&json!({ - "languageId": language_id.clone().unwrap(), + "languageId": lang_id_real, "filename": filename, }))?; @@ -3909,8 +3940,7 @@ impl LanguageClient { state .clients .get(&Some(language_id.clone())) - .map(|c| c.process_id) - .unwrap_or_default(), + .and_then(|c| c.child_process.as_ref().map(|p| p.id())) ); msg += &format!("Language server stderr: {}\n", server_stderr,); msg += &format!("Log level: {}\n", state.logger.level); diff --git a/src/rpcclient.rs b/src/rpcclient.rs index dac81af60..70d382048 100644 --- a/src/rpcclient.rs +++ b/src/rpcclient.rs @@ -9,6 +9,7 @@ use std::str::FromStr; use std::{ collections::HashMap, io::BufRead, + process::Child, sync::atomic::{AtomicU64, Ordering}, thread, time::Duration, @@ -32,7 +33,8 @@ pub struct RpcClient { writer_tx: Sender, #[serde(skip_serializing)] reader_tx: Sender<(Id, Sender)>, - pub process_id: Option, + #[serde(skip_serializing)] // FIXME + pub child_process: Option, } impl RpcClient { @@ -41,7 +43,7 @@ impl RpcClient { language_id: LanguageId, reader: impl BufRead + Send + 'static, writer: impl Write + Send + 'static, - process_id: Option, + child_process: Option, sink: Sender, on_crash: impl Fn(&LanguageId) + Clone + Send + 'static, ) -> Result { @@ -86,7 +88,7 @@ impl RpcClient { Ok(Self { language_id, id: AtomicU64::default(), - process_id, + child_process, reader_tx, writer_tx, }) From 6d2223d93369f2fb51afe218f9a8b51c17832b6e Mon Sep 17 00:00:00 2001 From: max ulidtko Date: Tue, 4 May 2021 03:59:48 +0300 Subject: [PATCH 2/4] Keep the RpcClient on deliberate server stop Forgetting it here is premature, and will leak the server process. Instead, let on_server_crash() handle the cleanup & deal with zombies. --- src/language_server_protocol.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/language_server_protocol.rs b/src/language_server_protocol.rs index e2f033b2d..355a04bc7 100644 --- a/src/language_server_protocol.rs +++ b/src/language_server_protocol.rs @@ -792,7 +792,6 @@ impl LanguageClient { self.handle_cursor_moved(&Value::Null, true)?; self.update_state(|state| { - state.clients.remove(&Some(language_id.into())); state.last_cursor_line = 0; state.text_documents.retain(|f, _| !f.starts_with(&root)); state.roots.remove(language_id); From 15a593402cdc48466bbbfc6b05c20b7ce22da824 Mon Sep 17 00:00:00 2001 From: max ulidtko Date: Tue, 4 May 2021 04:04:19 +0300 Subject: [PATCH 3/4] Don't reinsert reaped RpcClient, no need (fixup) --- src/language_server_protocol.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/language_server_protocol.rs b/src/language_server_protocol.rs index 355a04bc7..479137e27 100644 --- a/src/language_server_protocol.rs +++ b/src/language_server_protocol.rs @@ -3746,8 +3746,6 @@ impl LanguageClient { Err(e) => error!("Process wait failed: {}", e), } } - client.child_process = None; - state.clients.insert(language_id.clone(), Arc::new(client)); Ok(()) }) .map_err(|err| anyhow!( @@ -3787,7 +3785,6 @@ impl LanguageClient { restarts = 0; }; - state.clients.remove(language_id); state.restarts.insert(language_id.clone(), restarts); Ok(()) })?; From d7905a3aec9d24adde841299e01a1b75324805fa Mon Sep 17 00:00:00 2001 From: max ulidtko Date: Tue, 4 May 2021 04:05:20 +0300 Subject: [PATCH 4/4] Downgrade too chatty tracers to debug loglevel The extra-verbose dumps of pages and pages of user-generated data *per message* simply don't belong at INFO loglevel, killing it's usability. --- src/language_server_protocol.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/language_server_protocol.rs b/src/language_server_protocol.rs index 479137e27..c43de2d89 100644 --- a/src/language_server_protocol.rs +++ b/src/language_server_protocol.rs @@ -2091,7 +2091,7 @@ impl LanguageClient { Ok(()) } - #[tracing::instrument(level = "info", skip(self))] + #[tracing::instrument(level = "debug", skip(self))] pub fn text_document_publish_diagnostics(&self, params: &Value) -> Result<()> { let params = PublishDiagnosticsParams::deserialize(params)?; if !self.get_config(|c| c.diagnostics_enable)? { @@ -3873,7 +3873,7 @@ impl LanguageClient { Ok(()) } - #[tracing::instrument(level = "info", skip(self))] + #[tracing::instrument(level = "debug", skip(self))] pub fn workspace_did_change_watched_files(&self, params: &Value) -> Result<()> { let filename = self.vim()?.get_filename(params)?; let language_id = self.vim()?.get_language_id(&filename, params)?;