diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index 8433ad380..8f222337a 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -7,8 +7,11 @@ #![allow(deprecated)] +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; @@ -26,6 +29,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; @@ -44,24 +48,68 @@ use crate::lsp::statement_range::StatementRangeParams; use crate::lsp::statement_range::StatementRangeResponse; use crate::r_task; +// 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 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), + 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 { - 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) => { + // Notify user that the LSP has crashed and is no longer active + report_crash(); + + // 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( + "The LSP server has crashed and is now shut down!", + ))), _ => panic!("Unexpected variant while casting to {}", stringify!($pat)), } }}; } +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), - Request( - LspRequest, - TokioUnboundedSender>, - ), + Request(LspRequest, TokioUnboundedSender), } #[derive(Debug)] @@ -79,7 +127,6 @@ pub(crate) enum LspNotification { #[derive(Debug)] pub(crate) enum LspRequest { Initialize(InitializeParams), - Shutdown(), WorkspaceSymbol(WorkspaceSymbolParams), DocumentSymbol(DocumentSymbolParams), ExecuteCommand(ExecuteCommandParams), @@ -101,7 +148,6 @@ pub(crate) enum LspRequest { #[derive(Debug)] pub(crate) enum LspResponse { Initialize(InitializeResult), - Shutdown(()), WorkspaceSymbol(Option>), DocumentSymbol(Option), ExecuteCommand(Option), @@ -122,6 +168,10 @@ 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, @@ -131,9 +181,12 @@ struct Backend { } impl Backend { - async fn request(&self, request: LspRequest) -> anyhow::Result { - let (response_tx, mut response_rx) = - tokio_unbounded_channel::>(); + async fn request(&self, request: LspRequest) -> RequestResponse { + if LSP_HAS_CRASHED.load(Ordering::Acquire) { + return RequestResponse::Disabled; + } + + let (response_tx, mut response_rx) = tokio_unbounded_channel::(); // Relay request to main loop self.events_tx @@ -156,6 +209,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 ) @@ -166,10 +220,9 @@ impl LanguageServer for Backend { } async fn shutdown(&self) -> Result<()> { - cast_response!( - 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) { @@ -189,6 +242,7 @@ impl LanguageServer for Backend { params: WorkspaceSymbolParams, ) -> Result>> { cast_response!( + self, self.request(LspRequest::WorkspaceSymbol(params)).await, LspResponse::WorkspaceSymbol ) @@ -199,6 +253,7 @@ impl LanguageServer for Backend { params: DocumentSymbolParams, ) -> Result> { cast_response!( + self, self.request(LspRequest::DocumentSymbol(params)).await, LspResponse::DocumentSymbol ) @@ -209,6 +264,7 @@ impl LanguageServer for Backend { params: ExecuteCommandParams, ) -> jsonrpc::Result> { cast_response!( + self, self.request(LspRequest::ExecuteCommand(params)).await, LspResponse::ExecuteCommand ) @@ -232,6 +288,7 @@ impl LanguageServer for Backend { async fn completion(&self, params: CompletionParams) -> Result> { cast_response!( + self, self.request(LspRequest::Completion(params)).await, LspResponse::Completion ) @@ -239,6 +296,7 @@ impl LanguageServer for Backend { async fn completion_resolve(&self, item: CompletionItem) -> Result { cast_response!( + self, self.request(LspRequest::CompletionResolve(item)).await, LspResponse::CompletionResolve ) @@ -246,6 +304,7 @@ impl LanguageServer for Backend { async fn hover(&self, params: HoverParams) -> Result> { cast_response!( + self, self.request(LspRequest::Hover(params)).await, LspResponse::Hover ) @@ -253,6 +312,7 @@ impl LanguageServer for Backend { async fn signature_help(&self, params: SignatureHelpParams) -> Result> { cast_response!( + self, self.request(LspRequest::SignatureHelp(params)).await, LspResponse::SignatureHelp ) @@ -263,6 +323,7 @@ impl LanguageServer for Backend { params: GotoDefinitionParams, ) -> Result> { cast_response!( + self, self.request(LspRequest::GotoDefinition(params)).await, LspResponse::GotoDefinition ) @@ -273,6 +334,7 @@ impl LanguageServer for Backend { params: GotoImplementationParams, ) -> Result> { cast_response!( + self, self.request(LspRequest::GotoImplementation(params)).await, LspResponse::GotoImplementation ) @@ -283,6 +345,7 @@ impl LanguageServer for Backend { params: SelectionRangeParams, ) -> Result>> { cast_response!( + self, self.request(LspRequest::SelectionRange(params)).await, LspResponse::SelectionRange ) @@ -290,6 +353,7 @@ impl LanguageServer for Backend { async fn references(&self, params: ReferenceParams) -> Result>> { cast_response!( + self, self.request(LspRequest::References(params)).await, LspResponse::References ) @@ -300,6 +364,7 @@ impl LanguageServer for Backend { params: DocumentOnTypeFormattingParams, ) -> Result>> { cast_response!( + self, self.request(LspRequest::OnTypeFormatting(params)).await, LspResponse::OnTypeFormatting ) @@ -327,6 +392,7 @@ impl Backend { params: StatementRangeParams, ) -> jsonrpc::Result> { cast_response!( + self, self.request(LspRequest::StatementRange(params)).await, LspResponse::StatementRange ) @@ -337,6 +403,7 @@ impl Backend { params: HelpTopicParams, ) -> jsonrpc::Result> { cast_response!( + self, self.request(LspRequest::HelpTopic(params)).await, LspResponse::HelpTopic ) @@ -347,6 +414,7 @@ impl Backend { params: VirtualDocumentParams, ) -> tower_lsp::jsonrpc::Result { cast_response!( + self, self.request(LspRequest::VirtualDocument(params)).await, LspResponse::VirtualDocument ) @@ -357,6 +425,7 @@ impl Backend { params: InputBoundariesParams, ) -> tower_lsp::jsonrpc::Result { cast_response!( + self, self.request(LspRequest::InputBoundaries(params)).await, LspResponse::InputBoundaries ) @@ -381,6 +450,8 @@ pub fn start_lsp(runtime: Arc, address: String, conn_init_tx: Sender(1); + let init = |client: Client| { let state = GlobalState::new(client); let events_tx = state.events_tx(); @@ -403,6 +474,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/main_loop.rs b/crates/ark/src/lsp/main_loop.rs index 242fd6979..6bbb83da0 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; @@ -20,6 +22,7 @@ use tower_lsp::lsp_types::MessageType; use tower_lsp::Client; use url::Url; +use super::backend::RequestResponse; use crate::lsp; use crate::lsp::backend::LspMessage; use crate::lsp::backend::LspNotification; @@ -50,6 +53,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 +277,58 @@ impl GlobalState { match request { 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)?; + respond(tx, || state_handlers::initialize(params, &mut self.lsp_state, &mut self.world), LspResponse::Initialize)?; }, 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)?; }, }; }, @@ -365,7 +367,7 @@ impl GlobalState { /// world state could be run concurrently. On the other hand, handlers that /// manipulate documents (e.g. formatting or refactoring) should not. fn spawn_handler( - response_tx: TokioUnboundedSender>, + response_tx: TokioUnboundedSender, handler: Handler, into_lsp_response: impl FnOnce(T) -> LspResponse + Send + 'static, ) where @@ -373,7 +375,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)) }) } } @@ -394,13 +396,37 @@ 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>, - response: anyhow::Result, + 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; + + 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); + let out = match response { Ok(_) => Ok(()), Err(ref err) => Err(anyhow!("Error while handling request:\n{err:?}")), @@ -408,6 +434,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); diff --git a/crates/ark/src/main.rs b/crates/ark/src/main.rs index 308e94bb6..785abb1f0 100644 --- a/crates/ark/src/main.rs +++ b/crates/ark/src/main.rs @@ -300,6 +300,9 @@ 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| { let info = panic_info.payload(); @@ -337,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));