Move LSP's `tokio::Runtime` and `Client` to `RMain` #193

DavisVaughan · 2024-01-03T20:30:51Z

Joint work with @lionel-

Addresses posit-dev/positron#1885
Addresses posit-dev/positron#1956

We now use 1 tokio::Runtime for LSP tasks. The tasks could either come from the main LSP thread or from an R callback, like file.edit() (which calls the LSP show_document() method).
We now call spawn() rather than block_on() in ps_editor(). This allows R to return immediately from ps_editor(), preventing the deadlock that is described in R exists unexpectedly when calling file.edit() many times positron#1885 (comment). We aren't worried about the fact that file.edit() could technically return before the file has been opened. RStudio similarly just sends off an event to open the file and then immediately returns, and it hasn't proven to be an issue there. We also weren't waiting on the file to be open before, the future we were blocking on was just confirmation that the show document request actually gets sent, not that it had been received and acted upon.
I've finally been able to remove all reliance on the ugly global variables we had floating around.
- The LSP tokio Runtime is created in ark's start_kernel() and wrapped in an Arc so it can be sent to the LSP thread (the runtime can't be cloned). It is also sent start_r() to be a part of RMain. Runtime methods are all immutable, so it should not require a Mutex.
- The LSP Client was initially trickier, but I figured out that you can build the LspService at any time, and that is what gives us access to the Client, so I extracted that build call out into build_lsp_service() and we call that from start_kernel() now too. The returned service and socket are passed along to the LSP handler to actually start the LSP, but the client is instead routed to start_r() to be a part of RMain. One thing to note is that we need to be somewhat careful that the LSP actually gets initialized before sending requests through the Client. i.e. the LSP initialize() call needs to get called first. I think this is unlikely to be problematic in practice?

Proof of some things working on the windows vm that didn't work before

Screen.Recording.2024-01-03.at.3.46.09.PM.mov

Screen.Recording.2024-01-03.at.3.46.49.PM.mov

DavisVaughan · 2024-01-03T20:33:21Z

crates/ark/src/lsp/backend.rs

@@ -496,35 +497,51 @@ impl Backend {
    }
 }

-#[tokio::main]


No more attribute because we manage this manually now by creating the Runtime elsewhere and calling block_on() ourselves

Cool, this attribute seems a bit too magic/mysterious when you're not familiar with tokio

DavisVaughan · 2024-01-03T20:35:00Z

crates/ark/src/lsp/backend.rs


-    #[cfg(feature = "runtime-agnostic")]
-    let (read, write) = (read.compat(), write.compat_write());
+pub fn build_lsp_service() -> (LspService<Backend>, ClientSocket, Client) {


This encapsulates the "trick" for getting access to the Client in a way that can be sent along to start_r()

DavisVaughan · 2024-01-03T20:35:24Z

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) {


Happened to realize along the way that this doesn't need to be async

I was also thinking we could remove the unnecessary block_on() at some point since they complicate call stacks with tokio stuff.

DavisVaughan · 2024-01-03T20:35:50Z

crates/ark/src/lsp/editor.rs

-    let rt = Runtime::new().unwrap();
-    let globals = R_CALLBACK_GLOBALS.as_ref().unwrap();
+    let main = RMain::get();
+    let runtime = main.get_lsp_runtime();


Now reusing existing runtime!

DavisVaughan · 2024-01-03T20:36:54Z

crates/ark/src/lsp/editor.rs

+            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:?}");
+            }


This seems unlikely, but better than an unwrap() IMO

DavisVaughan · 2024-01-03T20:37:47Z

crates/ark/src/lsp/handler.rs

+        // 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)


I like this Option + take() trick for one-time ownership transfer to threads. Useful for things that can't be cloned

lionel- · 2024-01-08T09:05:23Z

crates/ark/src/interface.rs

+    // 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<Runtime>,


Should this be called tokio_runtime? I assume we'll share with entities other than the LSP if tokio becomes needed in other parts of ark?

Happy to rename this in the future, but right now I'm not 100% sure if we will use the same tokio runtime everywhere or not. For example, in help_proxy.rs we have another separate runtime already

// The help proxy main entry point. #[tokio::main] async fn task(target_port: u16) -> anyhow::Result<()> { // Create the help proxy. let help_proxy = HelpProxy::new(target_port)?; // Set the help proxy port. unsafe { browser::PORT = help_proxy.source_port }; // Run the help proxy. Ok(help_proxy.run().await?) }

crates/ark/src/lsp/backend.rs

crates/ark/src/lsp/editor.rs

DavisVaughan commented Jan 3, 2024

View reviewed changes

DavisVaughan requested a review from lionel- January 3, 2024 20:38

lionel- approved these changes Jan 8, 2024

View reviewed changes

DavisVaughan added 8 commits January 8, 2024 08:45

Hack away enough to get a runtime global

eb510bc

Recognize that enqueue_diagnostics() isn't async

20210cc

Use non-blocking spawn() to avoid deadlocking R

1e92bf7

Let start_kernel() own the runtime

20c9529

Let start_kernel() build the LSP service

0307a7a

Another bit of rationale

f57aa38

Switch to trace!() and use log:: everywhere

f467711

Separate constructor and runner

cf346b5

DavisVaughan force-pushed the test/lsp-one-runtime branch from b7c26f7 to cf346b5 Compare January 8, 2024 13:45

DavisVaughan merged commit e3c7b45 into main Jan 8, 2024
1 check passed

DavisVaughan deleted the test/lsp-one-runtime branch January 8, 2024 13:55

DavisVaughan mentioned this pull request Jan 10, 2024

Rebuild LSP service, socket, and client on reconnect #201

Merged

lionel- mentioned this pull request May 17, 2024

Open files via Jupyter instead of LSP #357

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move LSP's `tokio::Runtime` and `Client` to `RMain` #193

Move LSP's `tokio::Runtime` and `Client` to `RMain` #193

DavisVaughan commented Jan 3, 2024 •

edited

Loading

DavisVaughan Jan 3, 2024

lionel- Jan 8, 2024

DavisVaughan Jan 3, 2024

DavisVaughan Jan 3, 2024

lionel- Jan 8, 2024

DavisVaughan Jan 3, 2024

DavisVaughan Jan 3, 2024

DavisVaughan Jan 3, 2024

lionel- Jan 8, 2024

DavisVaughan Jan 8, 2024

Move LSP's tokio::Runtime and Client to RMain #193

Move LSP's tokio::Runtime and Client to RMain #193

Conversation

DavisVaughan commented Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Move LSP's `tokio::Runtime` and `Client` to `RMain` #193

Move LSP's `tokio::Runtime` and `Client` to `RMain` #193

DavisVaughan commented Jan 3, 2024 •

edited

Loading