From 49c2510d85a084aa3c1ea33346fb42f06066cf1f Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 8 Jan 2025 18:20:08 +0100 Subject: [PATCH 01/10] Disable LSP on panic --- crates/ark/src/lsp/backend.rs | 8 +++++ crates/ark/src/lsp/main_loop.rs | 54 +++++++++++++++++++++------------ 2 files changed, 42 insertions(+), 20 deletions(-) diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index 8433ad380..9366e1214 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -7,6 +7,7 @@ #![allow(deprecated)] +use std::sync::atomic::Ordering; use std::sync::Arc; use crossbeam::channel::Sender; @@ -26,6 +27,7 @@ use tower_lsp::LanguageServer; use tower_lsp::LspService; use tower_lsp::Server; +use super::main_loop::LSP_HAS_CRASHED; use crate::interface::RMain; use crate::lsp::handlers::VirtualDocumentParams; use crate::lsp::handlers::VirtualDocumentResponse; @@ -132,6 +134,12 @@ struct Backend { impl Backend { async fn request(&self, request: LspRequest) -> anyhow::Result { + if LSP_HAS_CRASHED.load(Ordering::Acquire) { + return Err(anyhow::anyhow!( + "The LSP server has crashed and is now shut down!" + )); + } + let (response_tx, mut response_rx) = tokio_unbounded_channel::>(); diff --git a/crates/ark/src/lsp/main_loop.rs b/crates/ark/src/lsp/main_loop.rs index 242fd6979..77e24846e 100644 --- a/crates/ark/src/lsp/main_loop.rs +++ b/crates/ark/src/lsp/main_loop.rs @@ -8,6 +8,8 @@ use std::collections::HashMap; use std::future; use std::pin::Pin; +use std::sync::atomic::AtomicBool; +use std::sync::atomic::Ordering; use std::sync::RwLock; use anyhow::anyhow; @@ -50,6 +52,8 @@ pub(crate) type TokioUnboundedReceiver = tokio::sync::mpsc::UnboundedReceiver /// LSPs to send log messages and tasks to the newer LSPs. static AUXILIARY_EVENT_TX: RwLock>> = RwLock::new(None); +pub static LSP_HAS_CRASHED: AtomicBool = AtomicBool::new(false); + // This is the syntax for trait aliases until an official one is stabilised. // This alias is for the future of a `JoinHandle>` trait AnyhowJoinHandleFut: @@ -272,61 +276,62 @@ impl GlobalState { match request { LspRequest::Initialize(params) => { - respond(tx, state_handlers::initialize(params, &mut self.lsp_state, &mut self.world), LspResponse::Initialize)?; + respond(tx, || state_handlers::initialize(params, &mut self.lsp_state, &mut self.world), LspResponse::Initialize)?; }, LspRequest::Shutdown() => { // TODO - respond(tx, Ok(()), LspResponse::Shutdown)?; + respond(tx, || Ok(()), LspResponse::Shutdown)?; }, LspRequest::WorkspaceSymbol(params) => { - respond(tx, handlers::handle_symbol(params), LspResponse::WorkspaceSymbol)?; + respond(tx, || handlers::handle_symbol(params), LspResponse::WorkspaceSymbol)?; }, LspRequest::DocumentSymbol(params) => { - respond(tx, handlers::handle_document_symbol(params, &self.world), LspResponse::DocumentSymbol)?; + respond(tx, || handlers::handle_document_symbol(params, &self.world), LspResponse::DocumentSymbol)?; }, LspRequest::ExecuteCommand(_params) => { - respond(tx, handlers::handle_execute_command(&self.client).await, LspResponse::ExecuteCommand)?; + let response = handlers::handle_execute_command(&self.client).await; + respond(tx, || response, LspResponse::ExecuteCommand)?; }, LspRequest::Completion(params) => { - respond(tx, handlers::handle_completion(params, &self.world), LspResponse::Completion)?; + respond(tx, || handlers::handle_completion(params, &self.world), LspResponse::Completion)?; }, LspRequest::CompletionResolve(params) => { - respond(tx, handlers::handle_completion_resolve(params), LspResponse::CompletionResolve)?; + respond(tx, || handlers::handle_completion_resolve(params), LspResponse::CompletionResolve)?; }, LspRequest::Hover(params) => { - respond(tx, handlers::handle_hover(params, &self.world), LspResponse::Hover)?; + respond(tx, || handlers::handle_hover(params, &self.world), LspResponse::Hover)?; }, LspRequest::SignatureHelp(params) => { - respond(tx, handlers::handle_signature_help(params, &self.world), LspResponse::SignatureHelp)?; + respond(tx, || handlers::handle_signature_help(params, &self.world), LspResponse::SignatureHelp)?; }, LspRequest::GotoDefinition(params) => { - respond(tx, handlers::handle_goto_definition(params, &self.world), LspResponse::GotoDefinition)?; + respond(tx, || handlers::handle_goto_definition(params, &self.world), LspResponse::GotoDefinition)?; }, LspRequest::GotoImplementation(_params) => { // TODO - respond(tx, Ok(None), LspResponse::GotoImplementation)?; + respond(tx, || Ok(None), LspResponse::GotoImplementation)?; }, LspRequest::SelectionRange(params) => { - respond(tx, handlers::handle_selection_range(params, &self.world), LspResponse::SelectionRange)?; + respond(tx, || handlers::handle_selection_range(params, &self.world), LspResponse::SelectionRange)?; }, LspRequest::References(params) => { - respond(tx, handlers::handle_references(params, &self.world), LspResponse::References)?; + respond(tx, || handlers::handle_references(params, &self.world), LspResponse::References)?; }, LspRequest::StatementRange(params) => { - respond(tx, handlers::handle_statement_range(params, &self.world), LspResponse::StatementRange)?; + respond(tx, || handlers::handle_statement_range(params, &self.world), LspResponse::StatementRange)?; }, LspRequest::HelpTopic(params) => { - respond(tx, handlers::handle_help_topic(params, &self.world), LspResponse::HelpTopic)?; + respond(tx, || handlers::handle_help_topic(params, &self.world), LspResponse::HelpTopic)?; }, LspRequest::OnTypeFormatting(params) => { state_handlers::did_change_formatting_options(¶ms.text_document_position.text_document.uri, ¶ms.options, &mut self.world); - respond(tx, handlers::handle_indent(params, &self.world), LspResponse::OnTypeFormatting)?; + respond(tx, || handlers::handle_indent(params, &self.world), LspResponse::OnTypeFormatting)?; }, LspRequest::VirtualDocument(params) => { - respond(tx, handlers::handle_virtual_document(params, &self.world), LspResponse::VirtualDocument)?; + respond(tx, || handlers::handle_virtual_document(params, &self.world), LspResponse::VirtualDocument)?; }, LspRequest::InputBoundaries(params) => { - respond(tx, handlers::handle_input_boundaries(params), LspResponse::InputBoundaries)?; + respond(tx, || handlers::handle_input_boundaries(params), LspResponse::InputBoundaries)?; }, }; }, @@ -373,7 +378,7 @@ impl GlobalState { Handler: Send + 'static, { lsp::spawn_blocking(move || { - respond(response_tx, handler(), into_lsp_response).and(Ok(None)) + respond(response_tx, || handler(), into_lsp_response).and(Ok(None)) }) } } @@ -398,9 +403,18 @@ impl GlobalState { /// * - `into_lsp_response`: A constructor for the relevant `LspResponse` variant. fn respond( response_tx: TokioUnboundedSender>, - response: anyhow::Result, + response: impl FnOnce() -> anyhow::Result, into_lsp_response: impl FnOnce(T) -> LspResponse, ) -> anyhow::Result<()> { + let response = std::panic::catch_unwind(std::panic::AssertUnwindSafe(response)) + .map_err(|err| { + // Set global crash flag to disable the LSP + LSP_HAS_CRASHED.store(true, Ordering::Release); + anyhow!("Panic occurred while handling request: {err:?}") + }) + // Unwrap nested Result + .and_then(|resp| resp); + let out = match response { Ok(_) => Ok(()), Err(ref err) => Err(anyhow!("Error while handling request:\n{err:?}")), From cac80398beab1e27f67d3a6b341358ef05a5f660 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 27 Jan 2025 17:14:53 +0100 Subject: [PATCH 02/10] Don't create backtraces when LSP has crashed --- crates/ark/src/lsp/backend.rs | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index 9366e1214..b7463183f 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -46,12 +46,23 @@ use crate::lsp::statement_range::StatementRangeParams; use crate::lsp::statement_range::StatementRangeResponse; use crate::r_task; +// This enum mainly allows us to create a JSON RPC error when the LSP has +// previously crashed without going through `anyhow` which always creates a +// backtrace. Backtraces would flood the logs with irrelevant information. +pub(crate) enum RequestResponse { + Crashed, + Result(anyhow::Result), +} + // Based on https://stackoverflow.com/a/69324393/1725177 macro_rules! cast_response { ($target:expr, $pat:path) => {{ match $target { - Ok($pat(resp)) => Ok(resp), - Err(err) => Err(new_jsonrpc_error(format!("{err:?}"))), + RequestResponse::Result(Ok($pat(resp))) => Ok(resp), + RequestResponse::Result(Err(err)) => Err(new_jsonrpc_error(format!("{err:?}"))), + RequestResponse::Crashed => Err(new_jsonrpc_error(String::from( + "The LSP server has crashed and is now shut down!", + ))), _ => panic!("Unexpected variant while casting to {}", stringify!($pat)), } }}; @@ -133,11 +144,9 @@ struct Backend { } impl Backend { - async fn request(&self, request: LspRequest) -> anyhow::Result { + async fn request(&self, request: LspRequest) -> RequestResponse { if LSP_HAS_CRASHED.load(Ordering::Acquire) { - return Err(anyhow::anyhow!( - "The LSP server has crashed and is now shut down!" - )); + return RequestResponse::Crashed; } let (response_tx, mut response_rx) = @@ -149,7 +158,7 @@ impl Backend { .unwrap(); // Wait for response from main loop - response_rx.recv().await.unwrap() + RequestResponse::Result(response_rx.recv().await.unwrap()) } fn notify(&self, notif: LspNotification) { From 24236998c01202dc5ebe3fd3079b802ec754c05a Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 27 Jan 2025 17:25:10 +0100 Subject: [PATCH 03/10] Disable panic hook for LSP threads --- crates/ark/src/lsp/backend.rs | 6 +++--- crates/ark/src/main.rs | 11 +++++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index b7463183f..8a4119d6d 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -50,7 +50,7 @@ use crate::r_task; // previously crashed without going through `anyhow` which always creates a // backtrace. Backtraces would flood the logs with irrelevant information. pub(crate) enum RequestResponse { - Crashed, + Disabled, Result(anyhow::Result), } @@ -60,7 +60,7 @@ macro_rules! cast_response { match $target { RequestResponse::Result(Ok($pat(resp))) => Ok(resp), RequestResponse::Result(Err(err)) => Err(new_jsonrpc_error(format!("{err:?}"))), - RequestResponse::Crashed => Err(new_jsonrpc_error(String::from( + RequestResponse::Disabled => Err(new_jsonrpc_error(String::from( "The LSP server has crashed and is now shut down!", ))), _ => panic!("Unexpected variant while casting to {}", stringify!($pat)), @@ -146,7 +146,7 @@ struct Backend { impl Backend { async fn request(&self, request: LspRequest) -> RequestResponse { if LSP_HAS_CRASHED.load(Ordering::Acquire) { - return RequestResponse::Crashed; + return RequestResponse::Disabled; } let (response_tx, mut response_rx) = diff --git a/crates/ark/src/main.rs b/crates/ark/src/main.rs index 308e94bb6..da146e95f 100644 --- a/crates/ark/src/main.rs +++ b/crates/ark/src/main.rs @@ -300,8 +300,19 @@ fn main() -> anyhow::Result<()> { // keeps running in an unstable state as all communications with this // thread will error out or panic. // https://stackoverflow.com/questions/35988775/how-can-i-cause-a-panic-on-a-thread-to-immediately-end-the-main-thread + // + // A better way to manage panics on background threads would be to ensure + // that we join all spawned threads up to the main thread. let old_hook = std::panic::take_hook(); std::panic::set_hook(Box::new(move |panic_info| { + // We don't want the threads managed by a Tokio runtime to `abort()` the + // process since their panics are caught and handled in other ways. + // This escape hatch is a hack that will also be activated by other + // Tokio contexts than just the LSP. + if tokio::runtime::Handle::try_current().is_ok() { + return; + } + let info = panic_info.payload(); let loc = if let Some(location) = panic_info.location() { From 130d9b6d119b80543f5ab13bfca704efd2c4ad99 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 28 Jan 2025 10:00:00 +0100 Subject: [PATCH 04/10] Notify users of crashes --- crates/ark/src/lsp/backend.rs | 67 +++++++++++++++++++++++++++------ crates/ark/src/lsp/main_loop.rs | 14 ++++++- 2 files changed, 67 insertions(+), 14 deletions(-) diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index 8a4119d6d..86190398c 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -46,20 +46,45 @@ use crate::lsp::statement_range::StatementRangeParams; use crate::lsp::statement_range::StatementRangeResponse; use crate::r_task; -// This enum mainly allows us to create a JSON RPC error when the LSP has -// previously crashed without going through `anyhow` which always creates a -// backtrace. Backtraces would flood the logs with irrelevant information. +// This enum is useful for two things. First it allows us to distinguish a +// normal request failure from a crash. In the latter case we send a +// notification to the client so the user knows the LSP has crashed. +// +// Once the LSP has crashed all requests respond with an error. This is a +// workaround, ideally we'd properly shut down the LSP from the server. The +// `Disabled` enum variant is an indicator of this state. We could have just +// created an anyhow error passed through the `Resul` variant but that would +// flood the LSP logs with irrelevant backtraces. pub(crate) enum RequestResponse { Disabled, + Crashed(anyhow::Result), Result(anyhow::Result), } // Based on https://stackoverflow.com/a/69324393/1725177 macro_rules! cast_response { - ($target:expr, $pat:path) => {{ + ($self:expr, $target:expr, $pat:path) => {{ match $target { RequestResponse::Result(Ok($pat(resp))) => Ok(resp), RequestResponse::Result(Err(err)) => Err(new_jsonrpc_error(format!("{err:?}"))), + RequestResponse::Crashed(err) => { + // Notify user that the LSP has crashed and is no longer active + $self + .client + .show_message( + MessageType::ERROR, + concat!( + "The R language server has crashed and has been disabled. ", + "Smart features such as completions will no longer work in this session. ", + "Please report this crash to https://github.com/posit-dev/positron/issues ", + "with full logs (see https://positron.posit.co/troubleshooting.html#python-and-r-logs)." + ), + ) + .await; + // The backtrace is reported via `err` and eventually shows up + // in the LSP logs on the client side + Err(new_jsonrpc_error(format!("{err:?}"))) + }, RequestResponse::Disabled => Err(new_jsonrpc_error(String::from( "The LSP server has crashed and is now shut down!", ))), @@ -71,10 +96,7 @@ macro_rules! cast_response { #[derive(Debug)] pub(crate) enum LspMessage { Notification(LspNotification), - Request( - LspRequest, - TokioUnboundedSender>, - ), + Request(LspRequest, TokioUnboundedSender), } #[derive(Debug)] @@ -138,6 +160,9 @@ struct Backend { /// Channel for communication with the main loop. events_tx: TokioUnboundedSender, + /// Tower-LSP's client + client: Client, + /// Handle to main loop. Drop it to cancel the loop, all associated tasks, /// and drop all owned state. _main_loop: tokio::task::JoinSet<()>, @@ -149,8 +174,7 @@ impl Backend { return RequestResponse::Disabled; } - let (response_tx, mut response_rx) = - tokio_unbounded_channel::>(); + let (response_tx, mut response_rx) = tokio_unbounded_channel::(); // Relay request to main loop self.events_tx @@ -158,7 +182,7 @@ impl Backend { .unwrap(); // Wait for response from main loop - RequestResponse::Result(response_rx.recv().await.unwrap()) + response_rx.recv().await.unwrap() } fn notify(&self, notif: LspNotification) { @@ -173,6 +197,7 @@ impl Backend { impl LanguageServer for Backend { async fn initialize(&self, params: InitializeParams) -> Result { cast_response!( + self, self.request(LspRequest::Initialize(params)).await, LspResponse::Initialize ) @@ -184,6 +209,7 @@ impl LanguageServer for Backend { async fn shutdown(&self) -> Result<()> { cast_response!( + self, self.request(LspRequest::Shutdown()).await, LspResponse::Shutdown ) @@ -206,6 +232,7 @@ impl LanguageServer for Backend { params: WorkspaceSymbolParams, ) -> Result>> { cast_response!( + self, self.request(LspRequest::WorkspaceSymbol(params)).await, LspResponse::WorkspaceSymbol ) @@ -216,6 +243,7 @@ impl LanguageServer for Backend { params: DocumentSymbolParams, ) -> Result> { cast_response!( + self, self.request(LspRequest::DocumentSymbol(params)).await, LspResponse::DocumentSymbol ) @@ -226,6 +254,7 @@ impl LanguageServer for Backend { params: ExecuteCommandParams, ) -> jsonrpc::Result> { cast_response!( + self, self.request(LspRequest::ExecuteCommand(params)).await, LspResponse::ExecuteCommand ) @@ -249,6 +278,7 @@ impl LanguageServer for Backend { async fn completion(&self, params: CompletionParams) -> Result> { cast_response!( + self, self.request(LspRequest::Completion(params)).await, LspResponse::Completion ) @@ -256,6 +286,7 @@ impl LanguageServer for Backend { async fn completion_resolve(&self, item: CompletionItem) -> Result { cast_response!( + self, self.request(LspRequest::CompletionResolve(item)).await, LspResponse::CompletionResolve ) @@ -263,6 +294,7 @@ impl LanguageServer for Backend { async fn hover(&self, params: HoverParams) -> Result> { cast_response!( + self, self.request(LspRequest::Hover(params)).await, LspResponse::Hover ) @@ -270,6 +302,7 @@ impl LanguageServer for Backend { async fn signature_help(&self, params: SignatureHelpParams) -> Result> { cast_response!( + self, self.request(LspRequest::SignatureHelp(params)).await, LspResponse::SignatureHelp ) @@ -280,6 +313,7 @@ impl LanguageServer for Backend { params: GotoDefinitionParams, ) -> Result> { cast_response!( + self, self.request(LspRequest::GotoDefinition(params)).await, LspResponse::GotoDefinition ) @@ -290,6 +324,7 @@ impl LanguageServer for Backend { params: GotoImplementationParams, ) -> Result> { cast_response!( + self, self.request(LspRequest::GotoImplementation(params)).await, LspResponse::GotoImplementation ) @@ -300,6 +335,7 @@ impl LanguageServer for Backend { params: SelectionRangeParams, ) -> Result>> { cast_response!( + self, self.request(LspRequest::SelectionRange(params)).await, LspResponse::SelectionRange ) @@ -307,6 +343,7 @@ impl LanguageServer for Backend { async fn references(&self, params: ReferenceParams) -> Result>> { cast_response!( + self, self.request(LspRequest::References(params)).await, LspResponse::References ) @@ -317,6 +354,7 @@ impl LanguageServer for Backend { params: DocumentOnTypeFormattingParams, ) -> Result>> { cast_response!( + self, self.request(LspRequest::OnTypeFormatting(params)).await, LspResponse::OnTypeFormatting ) @@ -344,6 +382,7 @@ impl Backend { params: StatementRangeParams, ) -> jsonrpc::Result> { cast_response!( + self, self.request(LspRequest::StatementRange(params)).await, LspResponse::StatementRange ) @@ -354,6 +393,7 @@ impl Backend { params: HelpTopicParams, ) -> jsonrpc::Result> { cast_response!( + self, self.request(LspRequest::HelpTopic(params)).await, LspResponse::HelpTopic ) @@ -364,6 +404,7 @@ impl Backend { params: VirtualDocumentParams, ) -> tower_lsp::jsonrpc::Result { cast_response!( + self, self.request(LspRequest::VirtualDocument(params)).await, LspResponse::VirtualDocument ) @@ -374,6 +415,7 @@ impl Backend { params: InputBoundariesParams, ) -> tower_lsp::jsonrpc::Result { cast_response!( + self, self.request(LspRequest::InputBoundaries(params)).await, LspResponse::InputBoundaries ) @@ -399,7 +441,7 @@ pub fn start_lsp(runtime: Arc, address: String, conn_init_tx: Sender, address: String, conn_init_tx: Sender( - response_tx: TokioUnboundedSender>, + response_tx: TokioUnboundedSender, handler: Handler, into_lsp_response: impl FnOnce(T) -> LspResponse + Send + 'static, ) where @@ -402,14 +403,17 @@ impl GlobalState { /// * - `response`: The response wrapped in a `anyhow::Result`. Errors are logged. /// * - `into_lsp_response`: A constructor for the relevant `LspResponse` variant. fn respond( - response_tx: TokioUnboundedSender>, + response_tx: TokioUnboundedSender, response: impl FnOnce() -> anyhow::Result, into_lsp_response: impl FnOnce(T) -> LspResponse, ) -> anyhow::Result<()> { + let mut crashed = false; + let response = std::panic::catch_unwind(std::panic::AssertUnwindSafe(response)) .map_err(|err| { // Set global crash flag to disable the LSP LSP_HAS_CRASHED.store(true, Ordering::Release); + crashed = true; anyhow!("Panic occurred while handling request: {err:?}") }) // Unwrap nested Result @@ -422,6 +426,12 @@ fn respond( let response = response.map(into_lsp_response); + let response = if crashed { + RequestResponse::Crashed(response) + } else { + RequestResponse::Result(response) + }; + // Ignore errors from a closed channel. This indicates the request has // been cancelled on the tower-lsp side. let _ = response_tx.send(response); From 56070a7a5e6b948c2395217a762e370dd3e37f36 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 28 Jan 2025 10:33:46 +0100 Subject: [PATCH 05/10] Unwrap message from panic info --- crates/ark/src/lsp/main_loop.rs | 14 +++++++++++++- crates/ark/src/main.rs | 16 ++++++++-------- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/crates/ark/src/lsp/main_loop.rs b/crates/ark/src/lsp/main_loop.rs index 33ff45883..5f779449f 100644 --- a/crates/ark/src/lsp/main_loop.rs +++ b/crates/ark/src/lsp/main_loop.rs @@ -414,7 +414,19 @@ fn respond( // Set global crash flag to disable the LSP LSP_HAS_CRASHED.store(true, Ordering::Release); crashed = true; - anyhow!("Panic occurred while handling request: {err:?}") + + let msg: String = if let Some(msg) = err.downcast_ref::<&str>() { + msg.to_string() + } else if let Some(msg) = err.downcast_ref::() { + msg.clone() + } else { + String::from("Couldn't retrieve the message.") + }; + + // This creates an uninformative backtrace that is reported in the + // LSP logs. Note that the relevant backtrace is the one created by + // our panic hook and reported via the _kernel_ logs. + anyhow!("Panic occurred while handling request: {msg}") }) // Unwrap nested Result .and_then(|resp| resp); diff --git a/crates/ark/src/main.rs b/crates/ark/src/main.rs index da146e95f..785abb1f0 100644 --- a/crates/ark/src/main.rs +++ b/crates/ark/src/main.rs @@ -305,14 +305,6 @@ fn main() -> anyhow::Result<()> { // that we join all spawned threads up to the main thread. let old_hook = std::panic::take_hook(); std::panic::set_hook(Box::new(move |panic_info| { - // We don't want the threads managed by a Tokio runtime to `abort()` the - // process since their panics are caught and handled in other ways. - // This escape hatch is a hack that will also be activated by other - // Tokio contexts than just the LSP. - if tokio::runtime::Handle::try_current().is_ok() { - return; - } - let info = panic_info.payload(); let loc = if let Some(location) = panic_info.location() { @@ -348,6 +340,14 @@ fn main() -> anyhow::Result<()> { log::error!("Panic! {loc} No contextual information.\n{trace}"); } + // We don't want the threads managed by a Tokio runtime to `abort()` the + // process since their panics are caught and handled in other ways. + // This escape hatch is a hack that will also be activated by other + // Tokio contexts than just the LSP. + if tokio::runtime::Handle::try_current().is_ok() { + return; + } + // Give some time to flush log log::logger().flush(); std::thread::sleep(std::time::Duration::from_millis(250)); From f8ed164d11a449742a0589f9eac428174449d8cc Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 28 Jan 2025 10:41:13 +0100 Subject: [PATCH 06/10] Allow `Shutdown` requests to go through --- crates/ark/src/lsp/backend.rs | 10 +++------- crates/ark/src/lsp/main_loop.rs | 4 ---- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index 86190398c..bbdd71eea 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -114,7 +114,6 @@ pub(crate) enum LspNotification { #[derive(Debug)] pub(crate) enum LspRequest { Initialize(InitializeParams), - Shutdown(), WorkspaceSymbol(WorkspaceSymbolParams), DocumentSymbol(DocumentSymbolParams), ExecuteCommand(ExecuteCommandParams), @@ -136,7 +135,6 @@ pub(crate) enum LspRequest { #[derive(Debug)] pub(crate) enum LspResponse { Initialize(InitializeResult), - Shutdown(()), WorkspaceSymbol(Option>), DocumentSymbol(Option), ExecuteCommand(Option), @@ -208,11 +206,9 @@ impl LanguageServer for Backend { } async fn shutdown(&self) -> Result<()> { - cast_response!( - self, - self.request(LspRequest::Shutdown()).await, - LspResponse::Shutdown - ) + // Don't go through the main loop because we want this request to + // succeed even when the LSP has crashed and has been disabled. + Ok(()) } async fn did_change_workspace_folders(&self, params: DidChangeWorkspaceFoldersParams) { diff --git a/crates/ark/src/lsp/main_loop.rs b/crates/ark/src/lsp/main_loop.rs index 5f779449f..c717fec19 100644 --- a/crates/ark/src/lsp/main_loop.rs +++ b/crates/ark/src/lsp/main_loop.rs @@ -279,10 +279,6 @@ impl GlobalState { LspRequest::Initialize(params) => { respond(tx, || state_handlers::initialize(params, &mut self.lsp_state, &mut self.world), LspResponse::Initialize)?; }, - LspRequest::Shutdown() => { - // TODO - respond(tx, || Ok(()), LspResponse::Shutdown)?; - }, LspRequest::WorkspaceSymbol(params) => { respond(tx, || handlers::handle_symbol(params), LspResponse::WorkspaceSymbol)?; }, From 7d96540272e828a86528796b525f803ba6c4a015 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Tue, 28 Jan 2025 16:35:44 -0500 Subject: [PATCH 07/10] `select!` on `shutdown_tx` and `server.serve()` to really shutdown --- crates/ark/src/lsp/backend.rs | 25 ++++++++++++++++++++----- crates/ark/src/lsp/handlers.rs | 1 + 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index bbdd71eea..b0374e20f 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -83,6 +83,7 @@ macro_rules! cast_response { .await; // The backtrace is reported via `err` and eventually shows up // in the LSP logs on the client side + let _ = $self.shutdown_tx.send(()).await; Err(new_jsonrpc_error(format!("{err:?}"))) }, RequestResponse::Disabled => Err(new_jsonrpc_error(String::from( @@ -155,6 +156,8 @@ pub(crate) enum LspResponse { #[derive(Debug)] struct Backend { + shutdown_tx: tokio::sync::mpsc::Sender<()>, + /// Channel for communication with the main loop. events_tx: TokioUnboundedSender, @@ -436,6 +439,8 @@ pub fn start_lsp(runtime: Arc, address: String, conn_init_tx: Sender(1); + let init = |client: Client| { let state = GlobalState::new(client.clone()); let events_tx = state.events_tx(); @@ -458,6 +463,7 @@ pub fn start_lsp(runtime: Arc, address: String, conn_init_tx: Sender, address: String, conn_init_tx: Sender { + log::trace!( + "LSP thread exiting gracefully after connection closed ({:?}).", + address + ); + }, + _ = shutdown_rx.recv() => { + log::trace!( + "LSP thread exiting after receiving a shutdown request ({:?}).", + address + ); + } + } }) } diff --git a/crates/ark/src/lsp/handlers.rs b/crates/ark/src/lsp/handlers.rs index f09604632..d3fc3e321 100644 --- a/crates/ark/src/lsp/handlers.rs +++ b/crates/ark/src/lsp/handlers.rs @@ -334,6 +334,7 @@ pub(crate) fn handle_statement_range( params: StatementRangeParams, state: &WorldState, ) -> anyhow::Result> { + panic!("oh no"); let uri = ¶ms.text_document.uri; let document = state.get_document(uri)?; From bcd181047b2315191f456a5d331cd2346c7a95f7 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 29 Jan 2025 08:57:47 +0100 Subject: [PATCH 08/10] Sleep to ensure notification shows up --- crates/ark/src/lsp/backend.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index b0374e20f..b6c4099f6 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -81,6 +81,9 @@ macro_rules! cast_response { ), ) .await; + + tokio::time::sleep(tokio::time::Duration::from_millis(500)).await; + // The backtrace is reported via `err` and eventually shows up // in the LSP logs on the client side let _ = $self.shutdown_tx.send(()).await; From 1f499e2bf1e04ee36bb908ad69e47a634efbc0ce Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 29 Jan 2025 09:37:33 +0100 Subject: [PATCH 09/10] Use Jupyter to send crash notification --- crates/ark/src/lsp/backend.rs | 37 ++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index b6c4099f6..e35c62417 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -10,6 +10,8 @@ use std::sync::atomic::Ordering; use std::sync::Arc; +use amalthea::comm::ui_comm::ShowMessageParams as UiShowMessageParams; +use amalthea::comm::ui_comm::UiFrontendEvent; use crossbeam::channel::Sender; use serde_json::Value; use stdext::result::ResultOrLog; @@ -69,20 +71,7 @@ macro_rules! cast_response { RequestResponse::Result(Err(err)) => Err(new_jsonrpc_error(format!("{err:?}"))), RequestResponse::Crashed(err) => { // Notify user that the LSP has crashed and is no longer active - $self - .client - .show_message( - MessageType::ERROR, - concat!( - "The R language server has crashed and has been disabled. ", - "Smart features such as completions will no longer work in this session. ", - "Please report this crash to https://github.com/posit-dev/positron/issues ", - "with full logs (see https://positron.posit.co/troubleshooting.html#python-and-r-logs)." - ), - ) - .await; - - tokio::time::sleep(tokio::time::Duration::from_millis(500)).await; + report_crash(); // The backtrace is reported via `err` and eventually shows up // in the LSP logs on the client side @@ -97,6 +86,26 @@ macro_rules! cast_response { }}; } +fn report_crash() { + let user_message = concat!( + "The R language server has crashed and has been disabled. ", + "Smart features such as completions will no longer work in this session. ", + "Please report this crash to https://github.com/posit-dev/positron/issues ", + "with full logs (see https://positron.posit.co/troubleshooting.html#python-and-r-logs)." + ); + + r_task(|| { + let event = UiFrontendEvent::ShowMessage(UiShowMessageParams { + message: String::from(user_message), + }); + + let main = RMain::get(); + if let Some(ui_comm_tx) = main.get_ui_comm_tx() { + ui_comm_tx.send_event(event); + } + }); +} + #[derive(Debug)] pub(crate) enum LspMessage { Notification(LspNotification), From 37deee3181525d29c0ef8b57803b72c5207c6c87 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 31 Jan 2025 13:54:56 +0100 Subject: [PATCH 10/10] Address code review --- crates/ark/src/lsp/backend.rs | 18 ++++++++---------- crates/ark/src/lsp/handlers.rs | 1 - crates/ark/src/lsp/main_loop.rs | 2 +- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index e35c62417..8f222337a 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -52,11 +52,11 @@ use crate::r_task; // normal request failure from a crash. In the latter case we send a // notification to the client so the user knows the LSP has crashed. // -// Once the LSP has crashed all requests respond with an error. This is a -// workaround, ideally we'd properly shut down the LSP from the server. The -// `Disabled` enum variant is an indicator of this state. We could have just -// created an anyhow error passed through the `Resul` variant but that would -// flood the LSP logs with irrelevant backtraces. +// Once the LSP has crashed all requests respond with an error. This prevents +// any handler from running while we process the message to shut down the +// server. The `Disabled` enum variant is an indicator of this state. We could +// have just created an anyhow error passed through the `Result` variant but that +// would flood the LSP logs with irrelevant backtraces. pub(crate) enum RequestResponse { Disabled, Crashed(anyhow::Result), @@ -168,14 +168,13 @@ pub(crate) enum LspResponse { #[derive(Debug)] struct Backend { + /// Shutdown notifier used to unwind tower-lsp and disconnect from the + /// client when an LSP handler panics. shutdown_tx: tokio::sync::mpsc::Sender<()>, /// Channel for communication with the main loop. events_tx: TokioUnboundedSender, - /// Tower-LSP's client - client: Client, - /// Handle to main loop. Drop it to cancel the loop, all associated tasks, /// and drop all owned state. _main_loop: tokio::task::JoinSet<()>, @@ -454,7 +453,7 @@ pub fn start_lsp(runtime: Arc, address: String, conn_init_tx: Sender(1); let init = |client: Client| { - let state = GlobalState::new(client.clone()); + let state = GlobalState::new(client); let events_tx = state.events_tx(); // Start main loop and hold onto the handle that keeps it alive @@ -477,7 +476,6 @@ pub fn start_lsp(runtime: Arc, address: String, conn_init_tx: Sender anyhow::Result> { - panic!("oh no"); let uri = ¶ms.text_document.uri; let document = state.get_document(uri)?; diff --git a/crates/ark/src/lsp/main_loop.rs b/crates/ark/src/lsp/main_loop.rs index c717fec19..6bbb83da0 100644 --- a/crates/ark/src/lsp/main_loop.rs +++ b/crates/ark/src/lsp/main_loop.rs @@ -396,7 +396,7 @@ impl GlobalState { /// # Arguments /// /// * - `response_tx`: A response channel for the tower-lsp request handler. -/// * - `response`: The response wrapped in a `anyhow::Result`. Errors are logged. +/// * - `response`: A closure producing a response wrapped in a `anyhow::Result`. Errors are logged. /// * - `into_lsp_response`: A constructor for the relevant `LspResponse` variant. fn respond( response_tx: TokioUnboundedSender,