From 1ff12a771986ca1e0c1572efcce5c3ee9bd74946 Mon Sep 17 00:00:00 2001 From: Ben Dennis Date: Sat, 13 Jan 2024 08:29:09 -0600 Subject: [PATCH 01/11] Keep lsp event listener thread alive when malformed json is encountered from the lsp server --- helix-lsp/src/transport.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helix-lsp/src/transport.rs b/helix-lsp/src/transport.rs index 9fdd30aa01cf..89ab28261318 100644 --- a/helix-lsp/src/transport.rs +++ b/helix-lsp/src/transport.rs @@ -302,7 +302,7 @@ impl Transport { } Err(err) => { error!("{} err: <- {err:?}", transport.name); - break; + continue; } } } From aafbd511b316cbb421d1484d492d6259617572b1 Mon Sep 17 00:00:00 2001 From: Ben Dennis Date: Mon, 15 Jan 2024 20:37:59 -0600 Subject: [PATCH 02/11] Update unexpected error flow in recv() to close outstanding requests and close the language server --- helix-lsp/src/transport.rs | 65 +++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 29 deletions(-) diff --git a/helix-lsp/src/transport.rs b/helix-lsp/src/transport.rs index 89ab28261318..7a088c1620a4 100644 --- a/helix-lsp/src/transport.rs +++ b/helix-lsp/src/transport.rs @@ -271,38 +271,13 @@ impl Transport { }; } Err(Error::StreamClosed) => { - // Close any outstanding requests. - for (id, tx) in transport.pending_requests.lock().await.drain() { - match tx.send(Err(Error::StreamClosed)).await { - Ok(_) => (), - Err(_) => { - error!("Could not close request on a closed channel (id={:?})", id) - } - } - } - - // Hack: inject a terminated notification so we trigger code that needs to happen after exit - use lsp_types::notification::Notification as _; - let notification = - ServerMessage::Call(jsonrpc::Call::Notification(jsonrpc::Notification { - jsonrpc: None, - method: lsp_types::notification::Exit::METHOD.to_string(), - params: jsonrpc::Params::None, - })); - match transport - .process_server_message(&client_tx, notification, &transport.name) - .await - { - Ok(_) => {} - Err(err) => { - error!("err: <- {:?}", err); - } - } + close_language_server(&transport, &client_tx).await; break; } Err(err) => { - error!("{} err: <- {err:?}", transport.name); - continue; + error!("Unexpected error when processing message from {}. Closing language server. err: <- {err:?}", &transport.name); + close_language_server(&transport, &client_tx).await; + break; } } } @@ -424,3 +399,35 @@ impl Transport { } } } + +async fn close_language_server( + transport: &Arc, + client_tx: &UnboundedSender<(usize, crate::Call)>, +) { + // Close any outstanding requests. + for (id, tx) in transport.pending_requests.lock().await.drain() { + match tx.send(Err(Error::StreamClosed)).await { + Ok(_) => (), + Err(_) => { + error!("Could not close request on a closed channel (id={:?})", id) + } + } + } + + // Hack: inject a terminated notification so we trigger code that needs to happen after exit + use lsp_types::notification::Notification as _; + let notification = ServerMessage::Call(jsonrpc::Call::Notification(jsonrpc::Notification { + jsonrpc: None, + method: lsp_types::notification::Exit::METHOD.to_string(), + params: jsonrpc::Params::None, + })); + match transport + .process_server_message(client_tx, notification, &transport.name) + .await + { + Ok(_) => {} + Err(err) => { + error!("err: <- {:?}", err); + } + } +} From 383fe4312a80fe040db1adb7fc80737b02a80be8 Mon Sep 17 00:00:00 2001 From: Ben Dennis Date: Mon, 15 Jan 2024 20:40:01 -0600 Subject: [PATCH 03/11] Log malformed notifications as info instead of error --- helix-term/src/application.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index 3abe9cae54ac..fa931177e4d5 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -676,7 +676,7 @@ impl Application { let notification = match Notification::parse(&method, params) { Ok(notification) => notification, Err(err) => { - log::error!( + log::info!( "received malformed notification from Language Server: {}", err ); From bef57cd01d506fd6eb80ef375d7e3b2b8bc91700 Mon Sep 17 00:00:00 2001 From: Ben Dennis Date: Mon, 15 Jan 2024 21:29:04 -0600 Subject: [PATCH 04/11] Make close_language_server a nested function inside recv, similar to what's done in send --- helix-lsp/src/transport.rs | 66 ++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 32 deletions(-) diff --git a/helix-lsp/src/transport.rs b/helix-lsp/src/transport.rs index 7a088c1620a4..fa15c5da913e 100644 --- a/helix-lsp/src/transport.rs +++ b/helix-lsp/src/transport.rs @@ -254,6 +254,40 @@ impl Transport { client_tx: UnboundedSender<(usize, jsonrpc::Call)>, ) { let mut recv_buffer = String::new(); + + async fn close_language_server( + transport: &Arc, + client_tx: &UnboundedSender<(usize, crate::Call)>, + ) { + // Close any outstanding requests. + for (id, tx) in transport.pending_requests.lock().await.drain() { + match tx.send(Err(Error::StreamClosed)).await { + Ok(_) => (), + Err(_) => { + error!("Could not close request on a closed channel (id={:?})", id) + } + } + } + + // Hack: inject a terminated notification so we trigger code that needs to happen after exit + use lsp_types::notification::Notification as _; + let notification = + ServerMessage::Call(jsonrpc::Call::Notification(jsonrpc::Notification { + jsonrpc: None, + method: lsp_types::notification::Exit::METHOD.to_string(), + params: jsonrpc::Params::None, + })); + match transport + .process_server_message(client_tx, notification, &transport.name) + .await + { + Ok(_) => {} + Err(err) => { + error!("err: <- {:?}", err); + } + } + } + loop { match Self::recv_server_message(&mut server_stdout, &mut recv_buffer, &transport.name) .await @@ -399,35 +433,3 @@ impl Transport { } } } - -async fn close_language_server( - transport: &Arc, - client_tx: &UnboundedSender<(usize, crate::Call)>, -) { - // Close any outstanding requests. - for (id, tx) in transport.pending_requests.lock().await.drain() { - match tx.send(Err(Error::StreamClosed)).await { - Ok(_) => (), - Err(_) => { - error!("Could not close request on a closed channel (id={:?})", id) - } - } - } - - // Hack: inject a terminated notification so we trigger code that needs to happen after exit - use lsp_types::notification::Notification as _; - let notification = ServerMessage::Call(jsonrpc::Call::Notification(jsonrpc::Notification { - jsonrpc: None, - method: lsp_types::notification::Exit::METHOD.to_string(), - params: jsonrpc::Params::None, - })); - match transport - .process_server_message(client_tx, notification, &transport.name) - .await - { - Ok(_) => {} - Err(err) => { - error!("err: <- {:?}", err); - } - } -} From e18329ca33bc238f56415ac7ffc6d2e0aa17e8e7 Mon Sep 17 00:00:00 2001 From: Ben Dennis Date: Mon, 15 Jan 2024 21:33:35 -0600 Subject: [PATCH 05/11] Update malformed notification log text --- helix-term/src/application.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index fa931177e4d5..c797e58d7efc 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -677,7 +677,7 @@ impl Application { Ok(notification) => notification, Err(err) => { log::info!( - "received malformed notification from Language Server: {}", + "Ignoring unknown notification from Language Server: {}", err ); return; From 1c14f21cf37ccbc298146b1eee4fc56dae7a082f Mon Sep 17 00:00:00 2001 From: Ben Dennis Date: Mon, 15 Jan 2024 22:05:12 -0600 Subject: [PATCH 06/11] Clean up new log text a bit --- helix-lsp/src/transport.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/helix-lsp/src/transport.rs b/helix-lsp/src/transport.rs index fa15c5da913e..a83cd83bc17d 100644 --- a/helix-lsp/src/transport.rs +++ b/helix-lsp/src/transport.rs @@ -309,7 +309,10 @@ impl Transport { break; } Err(err) => { - error!("Unexpected error when processing message from {}. Closing language server. err: <- {err:?}", &transport.name); + error!( + "Closing {} after unexpected error: {err:?}", + &transport.name + ); close_language_server(&transport, &client_tx).await; break; } From efdd577e42475bc547086a64dd9f7b98d8e83419 Mon Sep 17 00:00:00 2001 From: Ben Dennis Date: Mon, 15 Jan 2024 22:08:50 -0600 Subject: [PATCH 07/11] Initialize recv_buffer closer to where it's used --- helix-lsp/src/transport.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/helix-lsp/src/transport.rs b/helix-lsp/src/transport.rs index a83cd83bc17d..4da01b815539 100644 --- a/helix-lsp/src/transport.rs +++ b/helix-lsp/src/transport.rs @@ -253,8 +253,6 @@ impl Transport { mut server_stdout: BufReader, client_tx: UnboundedSender<(usize, jsonrpc::Call)>, ) { - let mut recv_buffer = String::new(); - async fn close_language_server( transport: &Arc, client_tx: &UnboundedSender<(usize, crate::Call)>, @@ -288,6 +286,8 @@ impl Transport { } } + let mut recv_buffer = String::new(); + loop { match Self::recv_server_message(&mut server_stdout, &mut recv_buffer, &transport.name) .await From 68929b2d4983acf4f10a80cac6f76a3fce19d872 Mon Sep 17 00:00:00 2001 From: Ben Dennis Date: Mon, 15 Jan 2024 22:16:01 -0600 Subject: [PATCH 08/11] Use "exit" instead of "close" --- helix-lsp/src/transport.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/helix-lsp/src/transport.rs b/helix-lsp/src/transport.rs index 4da01b815539..1c74082db24b 100644 --- a/helix-lsp/src/transport.rs +++ b/helix-lsp/src/transport.rs @@ -253,7 +253,7 @@ impl Transport { mut server_stdout: BufReader, client_tx: UnboundedSender<(usize, jsonrpc::Call)>, ) { - async fn close_language_server( + async fn exit_language_server( transport: &Arc, client_tx: &UnboundedSender<(usize, crate::Call)>, ) { @@ -305,15 +305,15 @@ impl Transport { }; } Err(Error::StreamClosed) => { - close_language_server(&transport, &client_tx).await; + exit_language_server(&transport, &client_tx).await; break; } Err(err) => { error!( - "Closing {} after unexpected error: {err:?}", + "Exiting {} after unexpected error: {err:?}", &transport.name ); - close_language_server(&transport, &client_tx).await; + exit_language_server(&transport, &client_tx).await; break; } } From 7366321aec81e832abab10a5e89d91db4009b275 Mon Sep 17 00:00:00 2001 From: Ben Dennis Date: Mon, 15 Jan 2024 22:58:20 -0600 Subject: [PATCH 09/11] Remove whitespace --- helix-lsp/src/transport.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/helix-lsp/src/transport.rs b/helix-lsp/src/transport.rs index 1c74082db24b..a10b02452e60 100644 --- a/helix-lsp/src/transport.rs +++ b/helix-lsp/src/transport.rs @@ -287,7 +287,6 @@ impl Transport { } let mut recv_buffer = String::new(); - loop { match Self::recv_server_message(&mut server_stdout, &mut recv_buffer, &transport.name) .await From 00ed2e1c3e504d8e11a13a252eec08eaccf4b9d8 Mon Sep 17 00:00:00 2001 From: Ben Dennis Date: Tue, 16 Jan 2024 23:16:38 -0600 Subject: [PATCH 10/11] Remove the need for a helper method to exit the language server --- helix-lsp/src/transport.rs | 76 +++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 42 deletions(-) diff --git a/helix-lsp/src/transport.rs b/helix-lsp/src/transport.rs index a10b02452e60..f2f35d6abf4b 100644 --- a/helix-lsp/src/transport.rs +++ b/helix-lsp/src/transport.rs @@ -253,39 +253,6 @@ impl Transport { mut server_stdout: BufReader, client_tx: UnboundedSender<(usize, jsonrpc::Call)>, ) { - async fn exit_language_server( - transport: &Arc, - client_tx: &UnboundedSender<(usize, crate::Call)>, - ) { - // Close any outstanding requests. - for (id, tx) in transport.pending_requests.lock().await.drain() { - match tx.send(Err(Error::StreamClosed)).await { - Ok(_) => (), - Err(_) => { - error!("Could not close request on a closed channel (id={:?})", id) - } - } - } - - // Hack: inject a terminated notification so we trigger code that needs to happen after exit - use lsp_types::notification::Notification as _; - let notification = - ServerMessage::Call(jsonrpc::Call::Notification(jsonrpc::Notification { - jsonrpc: None, - method: lsp_types::notification::Exit::METHOD.to_string(), - params: jsonrpc::Params::None, - })); - match transport - .process_server_message(client_tx, notification, &transport.name) - .await - { - Ok(_) => {} - Err(err) => { - error!("err: <- {:?}", err); - } - } - } - let mut recv_buffer = String::new(); loop { match Self::recv_server_message(&mut server_stdout, &mut recv_buffer, &transport.name) @@ -303,16 +270,41 @@ impl Transport { } }; } - Err(Error::StreamClosed) => { - exit_language_server(&transport, &client_tx).await; - break; - } Err(err) => { - error!( - "Exiting {} after unexpected error: {err:?}", - &transport.name - ); - exit_language_server(&transport, &client_tx).await; + if !matches!(err, Error::StreamClosed) { + error!( + "Exiting {} after unexpected error: {err:?}", + &transport.name + ); + } + + // Close any outstanding requests. + for (id, tx) in transport.pending_requests.lock().await.drain() { + match tx.send(Err(Error::StreamClosed)).await { + Ok(_) => (), + Err(_) => { + error!("Could not close request on a closed channel (id={:?})", id) + } + } + } + + // Hack: inject a terminated notification so we trigger code that needs to happen after exit + use lsp_types::notification::Notification as _; + let notification = + ServerMessage::Call(jsonrpc::Call::Notification(jsonrpc::Notification { + jsonrpc: None, + method: lsp_types::notification::Exit::METHOD.to_string(), + params: jsonrpc::Params::None, + })); + match transport + .process_server_message(&client_tx, notification, &transport.name) + .await + { + Ok(_) => {} + Err(err) => { + error!("err: <- {:?}", err); + } + } break; } } From a008f0b1ad96e105a36d0aca9b9ba5ddadd1ca8c Mon Sep 17 00:00:00 2001 From: Ben Dennis Date: Tue, 16 Jan 2024 23:18:30 -0600 Subject: [PATCH 11/11] Match on Unhandled error explicitly and keep catch-all error case around --- helix-term/src/application.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index c797e58d7efc..c43235aaebbc 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -33,7 +33,7 @@ use crate::{ ui::{self, overlay::overlaid}, }; -use log::{debug, error, warn}; +use log::{debug, error, info, warn}; #[cfg(not(feature = "integration"))] use std::io::stdout; use std::{collections::btree_map::Entry, io::stdin, path::Path, sync::Arc}; @@ -675,8 +675,12 @@ impl Application { Call::Notification(helix_lsp::jsonrpc::Notification { method, params, .. }) => { let notification = match Notification::parse(&method, params) { Ok(notification) => notification, + Err(helix_lsp::Error::Unhandled) => { + info!("Ignoring Unhandled notification from Language Server"); + return; + } Err(err) => { - log::info!( + error!( "Ignoring unknown notification from Language Server: {}", err );