R: Crashes in LSP main loop

jmcphers commented 1 week ago

No repro yet, but multiple folks have observed R crashes in the LSP main loop, especially on Workbench. Here's what the UI looks like when this happens:

And here's a stack trace:

[R]   2024-11-08T22:26:04.996195Z ERROR  Panic! In file 'crates/ark/src/lsp/main_loop.rs' at line 450: called `Option::unwrap()` on a `None` value
[R] 
[R] Backtrace:
[R]    0: std::backtrace::Backtrace::create
[R]    1: ark::main::{{closure}}::{{closure}}
[R]    2: ark::main::{{closure}}
[R]    3: std::panicking::rust_panic_with_hook
[R]    4: std::panicking::begin_panic_handler::{{closure}}
[R]    5: std::sys::backtrace::__rust_end_short_backtrace
[R]    6: _rust_begin_unwind
[R]    7: core::panicking::panic_fmt
[R]    8: core::panicking::panic
[R]    9: core::option::unwrap_failed
[R]   10: ark::lsp::main_loop::GlobalState::start::{{closure}}
[R]   11: tokio::runtime::task::harness::Harness<T,S>::poll
[R]   12: tokio::runtime::scheduler::multi_thread::worker::Context::run_task
[R]   13: tokio::runtime::scheduler::multi_thread::worker::Context::run
[R]   14: tokio::macros::scoped_tls::ScopedKey<T>::set
[R]   15: tokio::runtime::scheduler::multi_thread::worker::run
[R]   16: <tokio::runtime::blocking::task::BlockingTask<T> as core::future::future::Future>::poll
[R]   17: tokio::runtime::task::core::Core<T,S>::poll
[R]   18: tokio::runtime::task::harness::Harness<T,S>::poll
[R]   19: tokio::runtime::blocking::pool::Inner::run
[R]   20: std::sys::backtrace::__rust_begin_short_backtrace
[R]   21: core::ops::function::FnOnce::call_once{{vtable.shim}}
[R]   22: std::sys::pal::unix::thread::Thread::new::thread_start
[R]   23: __pthread_joiner_wake
[R] 
[R]     at crates/ark/src/main.rs:278

A Slack thread in which @Trevor-Reid mentioned this crash: https://positpbc.slack.com/archives/C07V12KUZLG/p1731000667301089

There was a proposed fix for this in https://github.com/posit-dev/ark/pull/617 (though it was ultimately not committed), and some discussion there about the problem. The right fix here may require work in Ark to better manage LSP state (e.g. to ensure only one session is active at a time, and gracefully shut down an existing LSP session with its event loops when a new one is requested)

lionel- commented 2 days ago

The right fix here may require work in Ark to better manage LSP state (e.g. to ensure only one session is active at a time, and gracefully shut down an existing LSP session with its event loops when a new one is requested)

Unfortunately the only way to do that is to send a Shutdown request followed by an Exit notification from the client (also depends on https://github.com/ebkalderon/tower-lsp/issues/399 being fixed, we can use tower-lsp = { git = "https://github.com/lionel-/tower-lsp", branch = "bugfix/patches" } until then).

The correct fix from Ark's viewpoint would be to refuse to start an LSP until the existing one has been disposed of, but we can't do that while the frontend creates multiple LSP sessions (only because of those zombie extension hosts IIUC?).

So let's just add the guardrails around global state that your proposed in the linked PR.

lionel- commented 2 days ago

So let's just add the guardrails around global state that your proposed in the linked PR.

But then it's possible for older LSPs to send log messages and notifications (publish diagnostics) via the currently active LSP, which could be quite confusing.

So I guess we should make that offending global state local. It was mostly made global for the convenience of using free functions/macros for log messages.

posit-dev / positron

R: Crashes in LSP main loop #5321