diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index bca3d013c..85440aefb 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -1,7 +1,7 @@ // // interface.rs // -// Copyright (C) 2023 Posit Software, PBC. All rights reserved. +// Copyright (C) 2023-2024 Posit Software, PBC. All rights reserved. // // @@ -79,6 +79,8 @@ use regex::Regex; use serde_json::json; use stdext::result::ResultOrLog; use stdext::*; +use tokio::runtime::Runtime; +use tower_lsp::Client; use crate::dap::dap::DapBackendEvent; use crate::dap::Dap; @@ -125,6 +127,8 @@ pub fn start_r( input_reply_rx: Receiver, iopub_tx: Sender, kernel_init_tx: Bus, + lsp_runtime: Arc, + lsp_client: Client, dap: Arc>, ) { // Initialize global state (ensure we only do this once!) @@ -145,6 +149,8 @@ pub fn start_r( input_reply_rx, iopub_tx, kernel_init_tx, + lsp_runtime, + lsp_client, dap, )); }); @@ -236,6 +242,12 @@ pub struct RMain { pub help_tx: Option>, pub help_rx: Option>, + // LSP tokio runtime used to spawn LSP tasks on the executor and the + // corresponding client used to send LSP requests to the frontend. + // Used by R callbacks, like `ps_editor()` for `utils::file.edit()`. + lsp_runtime: Arc, + lsp_client: Client, + dap: Arc>, is_debugging: bool, @@ -317,6 +329,8 @@ impl RMain { input_reply_rx: Receiver, iopub_tx: Sender, kernel_init_tx: Bus, + lsp_runtime: Arc, + lsp_client: Client, dap: Arc>, ) -> Self { Self { @@ -338,6 +352,8 @@ impl RMain { error_traceback: Vec::new(), help_tx: None, help_rx: None, + lsp_runtime, + lsp_client, dap, is_debugging: false, is_busy: false, @@ -971,6 +987,14 @@ impl RMain { pub fn get_kernel(&self) -> &Arc> { &self.kernel } + + pub fn get_lsp_runtime(&self) -> &Arc { + &self.lsp_runtime + } + + pub fn get_lsp_client(&self) -> &Client { + &self.lsp_client + } } /// Report an incomplete request to the front end diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index 54e6076c8..33bf68ffd 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -1,7 +1,7 @@ // // backend.rs // -// Copyright (C) 2022 Posit Software, PBC. All rights reserved. +// Copyright (C) 2022-2024 Posit Software, PBC. All rights reserved. // // @@ -12,17 +12,18 @@ use std::sync::Arc; use crossbeam::channel::Sender; use dashmap::DashMap; -use log::*; use parking_lot::Mutex; use serde_json::Value; use stdext::result::ResultOrLog; use stdext::*; use tokio::net::TcpListener; +use tokio::runtime::Runtime; use tower_lsp::jsonrpc::Result; use tower_lsp::lsp_types::request::GotoImplementationParams; use tower_lsp::lsp_types::request::GotoImplementationResponse; use tower_lsp::lsp_types::*; use tower_lsp::Client; +use tower_lsp::ClientSocket; use tower_lsp::LanguageServer; use tower_lsp::LspService; use tower_lsp::Server; @@ -34,7 +35,6 @@ use crate::lsp::diagnostics; use crate::lsp::document_context::DocumentContext; use crate::lsp::documents::Document; use crate::lsp::documents::DOCUMENT_INDEX; -use crate::lsp::globals; use crate::lsp::help_topic; use crate::lsp::hover::hover; use crate::lsp::indexer; @@ -89,12 +89,12 @@ impl Backend { // then use that; otherwise, try to read the document from the provided // path and use that instead. let uri = unwrap!(Url::from_file_path(path), Err(_) => { - info!("couldn't construct uri from {}; reading from disk instead", path.display()); + log::info!("couldn't construct uri from {}; reading from disk instead", path.display()); return fallback(); }); let document = unwrap!(self.documents.get(&uri), None => { - info!("no document for uri {}; reading from disk instead", uri); + log::info!("no document for uri {}; reading from disk instead", uri); return fallback(); }); @@ -214,7 +214,7 @@ impl LanguageServer for Backend { backend_trace!(self, "symbol({:?})", params); let response = unwrap!(symbols::symbols(self, ¶ms), Err(error) => { - error!("{:?}", error); + log::error!("{:?}", error); return Ok(None); }); @@ -228,7 +228,7 @@ impl LanguageServer for Backend { backend_trace!(self, "document_symbols({})", params.text_document.uri); let response = unwrap!(symbols::document_symbols(self, ¶ms), Err(error) => { - error!("{:?}", error); + log::error!("{:?}", error); return Ok(None); }); @@ -280,7 +280,7 @@ impl LanguageServer for Backend { if let Ok(path) = uri.to_file_path() { let path = Path::new(&path); if let Err(error) = indexer::update(&doc, &path) { - error!("{:?}", error); + log::error!("{:?}", error); } } @@ -288,7 +288,7 @@ impl LanguageServer for Backend { // the document now matches the version of the change after applying // it in `on_did_change()` if params.text_document.version == version { - diagnostics::enqueue_diagnostics(self.clone(), uri.clone(), version).await; + diagnostics::enqueue_diagnostics(self.clone(), uri.clone(), version); } } @@ -382,7 +382,7 @@ impl LanguageServer for Backend { // unwrap errors let result = unwrap!(result, Err(error) => { - error!("{:?}", error); + log::error!("{:?}", error); return Ok(None); }); @@ -413,7 +413,7 @@ impl LanguageServer for Backend { // unwrap errors let result = unwrap!(result, Err(error) => { - error!("{:?}", error); + log::error!("{:?}", error); return Ok(None); }); @@ -440,7 +440,7 @@ impl LanguageServer for Backend { // build goto definition context let result = unwrap!(unsafe { goto_definition(&document, params) }, Err(error) => { - error!("{}", error); + log::error!("{}", error); return Ok(None); }); @@ -453,7 +453,7 @@ impl LanguageServer for Backend { ) -> Result> { backend_trace!(self, "goto_implementation({:?})", params); let _ = params; - error!("Got a textDocument/implementation request, but it is not implemented"); + log::error!("Got a textDocument/implementation request, but it is not implemented"); return Ok(None); } @@ -492,39 +492,57 @@ impl LanguageServer for Backend { // https://github.com/Microsoft/vscode-languageserver-node/blob/18fad46b0e8085bb72e1b76f9ea23a379569231a/client/src/common/client.ts#L701-L752 impl Backend { async fn notification(&self, params: Option) { - info!("Received Positron notification: {:?}", params); + log::info!("Received Positron notification: {:?}", params); } } -#[tokio::main] -pub async fn start_lsp(address: String, conn_init_tx: Sender) { - #[cfg(feature = "runtime-agnostic")] - use tokio_util::compat::TokioAsyncReadCompatExt; - #[cfg(feature = "runtime-agnostic")] - use tokio_util::compat::TokioAsyncWriteCompatExt; - - debug!("Connecting to LSP at '{}'", &address); - let listener = TcpListener::bind(&address).await.unwrap(); - - // Notify frontend that we are ready to accept connections - conn_init_tx - .send(true) - .or_log_warning("Couldn't send LSP server init notification"); - - let (stream, _) = listener.accept().await.unwrap(); - debug!("Connected to LSP at '{}'", address); - let (read, write) = tokio::io::split(stream); +pub fn start_lsp( + runtime: Arc, + service: LspService, + socket: ClientSocket, + address: String, + conn_init_tx: Sender, +) { + runtime.block_on(async { + #[cfg(feature = "runtime-agnostic")] + use tokio_util::compat::TokioAsyncReadCompatExt; + #[cfg(feature = "runtime-agnostic")] + use tokio_util::compat::TokioAsyncWriteCompatExt; + + log::trace!("Connecting to LSP at '{}'", &address); + let listener = TcpListener::bind(&address).await.unwrap(); + + // Notify frontend that we are ready to accept connections + conn_init_tx + .send(true) + .or_log_warning("Couldn't send LSP server init notification"); + + let (stream, _) = listener.accept().await.unwrap(); + log::trace!("Connected to LSP at '{}'", address); + let (read, write) = tokio::io::split(stream); + + #[cfg(feature = "runtime-agnostic")] + let (read, write) = (read.compat(), write.compat_write()); + + let server = Server::new(read, write, socket); + server.serve(service).await; + + log::trace!( + "LSP thread exiting gracefully after connection closed ({:?}).", + address + ); + }) +} - #[cfg(feature = "runtime-agnostic")] - let (read, write) = (read.compat(), write.compat_write()); +pub fn build_lsp_service() -> (LspService, ClientSocket, Client) { + let mut client = None; - let init = |client: Client| { - // initialize shared globals (needed for R callbacks) - globals::initialize(client.clone()); + let init = |client_callback: Client| { + client = Some(client_callback.clone()); // create backend let backend = Backend { - client, + client: client_callback, documents: DOCUMENT_INDEX.clone(), workspace: Arc::new(Mutex::new(Workspace::default())), }; @@ -541,9 +559,7 @@ pub async fn start_lsp(address: String, conn_init_tx: Sender) { .custom_method("positron/notification", Backend::notification) .finish(); - Server::new(read, write, socket).serve(service).await; - debug!( - "LSP thread exiting gracefully after connection closed ({:?}).", - address - ); + let client = client.expect("`Client` should be initialized by the `build()` callback."); + + (service, socket, client) } diff --git a/crates/ark/src/lsp/diagnostics.rs b/crates/ark/src/lsp/diagnostics.rs index f306cb4cf..77fcf5820 100644 --- a/crates/ark/src/lsp/diagnostics.rs +++ b/crates/ark/src/lsp/diagnostics.rs @@ -85,7 +85,7 @@ impl<'a> DiagnosticContext<'a> { } } -pub async fn enqueue_diagnostics(backend: Backend, uri: Url, version: i32) { +pub fn enqueue_diagnostics(backend: Backend, uri: Url, version: i32) { // log::trace!("[diagnostics({version}, {uri})] Spawning task to enqueue diagnostics."); // Spawn a task to enqueue diagnostics. diff --git a/crates/ark/src/lsp/editor.rs b/crates/ark/src/lsp/editor.rs index e83ec8379..1b71389fe 100644 --- a/crates/ark/src/lsp/editor.rs +++ b/crates/ark/src/lsp/editor.rs @@ -1,7 +1,7 @@ // // editor.rs // -// Copyright (C) 2022 Posit Software, PBC. All rights reserved. +// Copyright (C) 2022-2024 Posit Software, PBC. All rights reserved. // // @@ -9,44 +9,69 @@ use harp::vector::CharacterVector; use harp::vector::Vector; use libR_shim::*; use stdext::unwrap; -use tokio::runtime::Runtime; use tower_lsp::lsp_types::ShowDocumentParams; use tower_lsp::lsp_types::Url; -use crate::lsp::globals::R_CALLBACK_GLOBALS; +use crate::interface::RMain; #[harp::register] unsafe extern "C" fn ps_editor(file: SEXP, _title: SEXP) -> anyhow::Result { - let rt = Runtime::new().unwrap(); - let globals = R_CALLBACK_GLOBALS.as_ref().unwrap(); + let main = RMain::get(); + let runtime = main.get_lsp_runtime(); + let client = main.get_lsp_client(); + let files = CharacterVector::new_unchecked(file); + let mut uris = Vec::new(); + for file in files.iter() { - if let Some(file) = file { - rt.block_on(async move { - let uri = Url::from_file_path(&file); - - let uri = unwrap!(uri, Err(_) => { - // The R side of this handles most issues, but we don't want to panic - // if some unknown file path slips through. - // `from_file_path()` doesn't return `Display`able errors, so we - // can't necessarily give a good reason. - log::error!("Can't open file at '{}'.", file); - return; - }); - - globals - .lsp_client - .show_document(ShowDocumentParams { - uri, - external: Some(false), - take_focus: Some(true), - selection: None, - }) - .await - .unwrap(); - }); - } + let Some(file) = file else { + // `NA_character_` slipped through + continue; + }; + + let uri = Url::from_file_path(&file); + + let uri = unwrap!(uri, Err(_) => { + // The R side of this handles most issues, but we don't want to panic + // if some unknown file path slips through. + // `from_file_path()` doesn't return `Display`able errors, so we + // can't necessarily give a good reason. + log::error!("Can't open file at '{}'.", file); + continue; + }); + + uris.push(uri); + } + + // Spawn a task to open the files in the editor. We use `spawn()` rather + // than `block_on()` because we need `ps_editor()` to return immediately + // to unblock the main R thread. Otherwise, at the `await` points in + // `show_document()`, it is possible for the runtime to execute a different + // async task that requires R (like diagnostics in `enqueue_diagnostics()`), + // which would cause a deadlock if R was being blocked here. + // Using `spawn()` means that `file.edit()` and the `editor` R option hook + // don't actually wait for the file to be fully opened, but that is + // generally ok and is also what RStudio does. + // https://github.com/posit-dev/positron/issues/1885 + for uri in uris.into_iter() { + runtime.spawn(async move { + let result = client + .show_document(ShowDocumentParams { + uri: uri.clone(), + external: Some(false), + take_focus: Some(true), + selection: None, + }) + .await; + + if let Err(err) = result { + // In the unlikely event that the LSP `client` hasn't been + // initialized yet, or has shut down, we probably don't want + // to crash ark. + log::error!("Failed to open '{uri}' due to {err:?}"); + } + }); } Ok(R_NilValue) diff --git a/crates/ark/src/lsp/globals.rs b/crates/ark/src/lsp/globals.rs deleted file mode 100644 index 7cce2319f..000000000 --- a/crates/ark/src/lsp/globals.rs +++ /dev/null @@ -1,31 +0,0 @@ -// -// globals.rs -// -// Copyright (C) 2023 Posit Software, PBC. All rights reserved. -// -// - -use tower_lsp::Client; - -// The global state used by R callbacks. -// -// Doesn't need a mutex because it's only accessed by the R thread. Should -// not be used elsewhere than from an R frontend callback or an R function -// invoked by the REPL. -pub(super) static mut R_CALLBACK_GLOBALS: Option = None; - -pub(super) struct RCallbackGlobals { - pub(super) lsp_client: Client, -} - -impl RCallbackGlobals { - fn new(lsp_client: Client) -> Self { - Self { lsp_client } - } -} - -pub fn initialize(lsp_client: Client) { - unsafe { - R_CALLBACK_GLOBALS = Some(RCallbackGlobals::new(lsp_client)); - } -} diff --git a/crates/ark/src/lsp/handler.rs b/crates/ark/src/lsp/handler.rs index eef410ab6..082c5b464 100644 --- a/crates/ark/src/lsp/handler.rs +++ b/crates/ark/src/lsp/handler.rs @@ -1,27 +1,48 @@ // // handler.rs // -// Copyright (C) 2022 Posit Software, PBC. All rights reserved. +// Copyright (C) 2022-2024 Posit Software, PBC. All rights reserved. // // +use std::sync::Arc; + use amalthea::comm::comm_channel::CommMsg; use amalthea::language::server_handler::ServerHandler; use bus::BusReader; use crossbeam::channel::Sender; use stdext::spawn; +use tokio::runtime::Runtime; +use tower_lsp::ClientSocket; +use tower_lsp::LspService; use super::backend; use crate::interface::KernelInfo; +use crate::lsp::backend::Backend; pub struct Lsp { + runtime: Option>, + service: Option>, + socket: Option, kernel_init_rx: BusReader, kernel_initialized: bool, } impl Lsp { - pub fn new(kernel_init_rx: BusReader) -> Self { + pub fn new( + runtime: Arc, + service: LspService, + socket: ClientSocket, + kernel_init_rx: BusReader, + ) -> Self { + let runtime = Some(runtime); + let service = Some(service); + let socket = Some(socket); + Self { + runtime, + service, + socket, kernel_init_rx, kernel_initialized: false, } @@ -47,8 +68,13 @@ impl ServerHandler for Lsp { self.kernel_initialized = true; } + // Transfer field ownership to the thread + let runtime = self.runtime.take().unwrap(); + let service = self.service.take().unwrap(); + let socket = self.socket.take().unwrap(); + spawn!("ark-lsp", move || { - backend::start_lsp(tcp_address, conn_init_tx) + backend::start_lsp(runtime, service, socket, tcp_address, conn_init_tx) }); return Ok(()); } diff --git a/crates/ark/src/lsp/mod.rs b/crates/ark/src/lsp/mod.rs index af0a9c23a..84d453297 100644 --- a/crates/ark/src/lsp/mod.rs +++ b/crates/ark/src/lsp/mod.rs @@ -1,7 +1,7 @@ // // mod.rs // -// Copyright (C) 2022 Posit Software, PBC. All rights reserved. +// Copyright (C) 2022-2024 Posit Software, PBC. All rights reserved. // // @@ -14,7 +14,6 @@ pub mod document_context; pub mod documents; pub mod editor; pub mod events; -pub mod globals; pub mod handler; pub mod help; pub mod help_topic; diff --git a/crates/ark/src/main.rs b/crates/ark/src/main.rs index aaa358dc1..443298a9e 100644 --- a/crates/ark/src/main.rs +++ b/crates/ark/src/main.rs @@ -1,7 +1,7 @@ // // main.rs // -// Copyright (C) 2022 Posit Software, PBC. All rights reserved. +// Copyright (C) 2022-2024 Posit Software, PBC. All rights reserved. // // @@ -61,10 +61,21 @@ fn start_kernel( let (r_request_tx, r_request_rx) = bounded::(1); let (kernel_request_tx, kernel_request_rx) = bounded::(1); + // The tokio runtime used to manage LSP task execution. + // Used in the main LSP thread and from R callbacks. + // Not wrapped in a `Mutex` since none of the methods require a mutable reference. + let lsp_runtime = Arc::new(tokio::runtime::Runtime::new().unwrap()); + let (lsp_service, lsp_socket, lsp_client) = lsp::backend::build_lsp_service(); + // Create the LSP and DAP clients. // Not all Amalthea kernels provide these, but ark does. // They must be able to deliver messages to the shell channel directly. - let lsp = Arc::new(Mutex::new(lsp::handler::Lsp::new(kernel_init_tx.add_rx()))); + let lsp = Arc::new(Mutex::new(lsp::handler::Lsp::new( + Arc::clone(&lsp_runtime), + lsp_service, + lsp_socket, + kernel_init_tx.add_rx(), + ))); // DAP needs the `RRequest` channel to communicate with // `read_console()` and send commands to the debug interpreter @@ -129,6 +140,8 @@ fn start_kernel( input_reply_rx, iopub_tx, kernel_init_tx, + lsp_runtime, + lsp_client, dap, ) }