Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

R: Crashes in LSP main loop #5321

Closed
jmcphers opened this issue Nov 8, 2024 · 5 comments
Closed

R: Crashes in LSP main loop #5321

jmcphers opened this issue Nov 8, 2024 · 5 comments
Labels
area: workbench Issues related to Workbench category. lang: r

Comments

@jmcphers
Copy link
Collaborator

jmcphers commented Nov 8, 2024

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:

Image

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 posit-dev/ark#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)

Related to posit-dev/ark#622.

@jmcphers jmcphers added area: workbench Issues related to Workbench category. lang: r labels Nov 8, 2024
@jmcphers jmcphers added this to the 2025.01.0 Pre-Release milestone Nov 12, 2024
@lionel-
Copy link
Contributor

lionel- commented Nov 14, 2024

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 ebkalderon/tower-lsp#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-
Copy link
Contributor

lionel- commented Nov 14, 2024

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.

Other solutions:

If we move the LSP out of the kernel and into its own process this will all be much simpler. The lifecycle of the LSPs will be fully managed by VSCode and we can guarantee there's only one session per process.

The other way would be to replace tower-lsp by https://github.com/rust-lang/rust-analyzer/tree/master/lib/lsp-server because then we'd be in full control of the LSP event loop and we would be able to forcibly shut it down from the server, making sure there's only one session alive at a time.

@jmcphers
Copy link
Collaborator Author

I also saw this locally:

\[R]   2024-11-27T23:21:35.959166Z ERROR  Panic! In file 'crates/ark/src/lsp/document_context.rs' at line 32: Failed to find closest node to point: (0, 10) with contents 'getOptoin'
[R] 
[R] Backtrace:
[R]    0: std::backtrace_rs::backtrace::libunwind::trace
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/../../backtrace/src/backtrace/libunwind.rs:116:5
[R]    1: std::backtrace_rs::backtrace::trace_unsynchronized
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
[R]    2: std::backtrace::Backtrace::create
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/backtrace.rs:331:13
[R]    3: ark::main::{{closure}}::{{closure}}
[R]              at /Users/jmcphers/scratch/ark/crates/ark/src/main.rs:270:21
[R]    4: ark::main::{{closure}}
[R]              at /Users/jmcphers/scratch/ark/crates/ark/src/main.rs:280:25
[R]    5: <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/alloc/src/boxed.rs:2077:9
[R]    6: std::panicking::rust_panic_with_hook
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/panicking.rs:799:13
[R]    7: std::panicking::begin_panic_handler::{{closure}}
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/panicking.rs:664:13
[R]    8: std::sys_common::backtrace::__rust_end_short_backtrace
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/sys_common/backtrace.rs:171:18
[R]    9: rust_begin_unwind
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/panicking.rs:652:5
[R]   10: core::panicking::panic_fmt
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/panicking.rs:72:14
[R]   11: ark::lsp::document_context::DocumentContext::new
[R]              at /Users/jmcphers/scratch/ark/crates/ark/src/lsp/document_context.rs:32:13
[R]   12: ark::lsp::handlers::handle_signature_help
[R]              at /Users/jmcphers/scratch/ark/crates/ark/src/lsp/handlers.rs:247:19
[R]   13: ark::lsp::main_loop::GlobalState::handle_event::{{closure}}
[R]              at /Users/jmcphers/scratch/ark/crates/ark/src/lsp/main_loop.rs:284:41
[R]   14: ark::lsp::main_loop::GlobalState::main_loop::{{closure}}
[R]              at /Users/jmcphers/scratch/ark/crates/ark/src/lsp/main_loop.rs:194:56
[R]   15: ark::lsp::main_loop::GlobalState::start::{{closure}}
[R]              at /Users/jmcphers/scratch/ark/crates/ark/src/lsp/main_loop.rs:182:49
[R]   16: <core::pin::Pin<P> as core::future::future::Future>::poll
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/future/future.rs:123:9
[R]   17: tokio::runtime::task::core::Core<T,S>::poll::{{closure}}
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/core.rs:223:17
[R]   18: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/loom/std/unsafe_cell.rs:14:9
[R]   19: tokio::runtime::task::core::Core<T,S>::poll
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/core.rs:212:13
[R]   20: tokio::runtime::task::harness::poll_future::{{closure}}
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/harness.rs:476:19
[R]   21: <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/panic/unwind_safe.rs:272:9
[R]   22: std::panicking::try::do_call
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/panicking.rs:559:40
[R]   23: ___rust_try
[R]   24: std::panicking::try
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/panicking.rs:523:19
[R]   25: std::panic::catch_unwind
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/panic.rs:149:14
[R]   26: tokio::runtime::task::harness::poll_future
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/harness.rs:464:18
[R]   27: tokio::runtime::task::harness::Harness<T,S>::poll_inner
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/harness.rs:198:27
[R]   28: tokio::runtime::task::harness::Harness<T,S>::poll
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/harness.rs:152:15
[R]   29: tokio::runtime::task::raw::poll
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/raw.rs:255:5
[R]   30: tokio::runtime::task::raw::RawTask::poll
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/raw.rs:200:18
[R]   31: tokio::runtime::task::LocalNotified<S>::run
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/mod.rs:394:9
[R]   32: tokio::runtime::scheduler::multi_thread::worker::Context::run_task::{{closure}}
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/scheduler/multi_thread/worker.rs:464:13
[R]   33: tokio::runtime::coop::with_budget
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/coop.rs:107:5
[R]   34: tokio::runtime::coop::budget
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/coop.rs:73:5
[R]   35: tokio::runtime::scheduler::multi_thread::worker::Context::run_task
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/scheduler/multi_thread/worker.rs:463:9
[R]   36: tokio::runtime::scheduler::multi_thread::worker::Context::run
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/scheduler/multi_thread/worker.rs:426:24
[R]   37: tokio::runtime::scheduler::multi_thread::worker::run::{{closure}}
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/scheduler/multi_thread/worker.rs:406:17
[R]   38: tokio::macros::scoped_tls::ScopedKey<T>::set
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/macros/scoped_tls.rs:61:9
[R]   39: tokio::runtime::scheduler::multi_thread::worker::run
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/scheduler/multi_thread/worker.rs:403:5
[R]   40: tokio::runtime::scheduler::multi_thread::worker::Launch::launch::{{closure}}
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/scheduler/multi_thread/worker.rs:365:45
[R]   41: <tokio::runtime::blocking::task::BlockingTask<T> as core::future::future::Future>::poll
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/blocking/task.rs:42:21
[R]   42: tokio::runtime::task::core::Core<T,S>::poll::{{closure}}
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/core.rs:223:17
[R]   43: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/loom/std/unsafe_cell.rs:14:9
[R]   44: tokio::runtime::task::core::Core<T,S>::poll
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/core.rs:212:13
[R]   45: tokio::runtime::task::harness::poll_future::{{closure}}
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/harness.rs:476:19
[R]   46: <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/panic/unwind_safe.rs:272:9
[R]   47: std::panicking::try::do_call
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/panicking.rs:559:40
[R]   48: ___rust_try
[R]   49: std::panicking::try
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/panicking.rs:523:19
[R]   50: std::panic::catch_unwind
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/panic.rs:149:14
[R]   51: tokio::runtime::task::harness::poll_future
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/harness.rs:464:18
[R]   52: tokio::runtime::task::harness::Harness<T,S>::poll_inner
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/harness.rs:198:27
[R]   53: tokio::runtime::task::harness::Harness<T,S>::poll
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/harness.rs:152:15
[R]   54: tokio::runtime::task::raw::poll
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/raw.rs:255:5
[R]   55: tokio::runtime::task::raw::RawTask::poll
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/raw.rs:200:18
[R]   56: tokio::runtime::task::UnownedTask<S>::run
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/task/mod.rs:431:9
[R]   57: tokio::runtime::blocking::pool::Task::run
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/blocking/pool.rs:159:9
[R]   58: tokio::runtime::blocking::pool::Inner::run
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/blocking/pool.rs:513:17
[R]   59: tokio::runtime::blocking::pool::Spawner::spawn_thread::{{closure}}
[R]              at /Users/jmcphers/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.1/src/runtime/blocking/pool.rs:471:13
[R]   60: std::sys_common::backtrace::__rust_begin_short_backtrace
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/sys_common/backtrace.rs:155:18
[R]   61: std::thread::Builder::spawn_unchecked_::{{closure}}::{{closure}}
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/thread/mod.rs:542:17
[R]   62: <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/panic/unwind_safe.rs:272:9
[R]   63: std::panicking::try::do_call
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/panicking.rs:559:40
[R]   64: ___rust_try
[R]   65: std::panicking::try
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/panicking.rs:523:19
[R]   66: std::panic::catch_unwind
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/panic.rs:149:14
[R]   67: std::thread::Builder::spawn_unchecked_::{{closure}}
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/thread/mod.rs:541:30
[R]   68: core::ops::function::FnOnce::call_once{{vtable.shim}}
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/core/src/ops/function.rs:250:5
[R]   69: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/alloc/src/boxed.rs:2063:9
[R]   70: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/alloc/src/boxed.rs:2063:9
[R]   71: std::sys::pal::unix::thread::Thread::new::thread_start
[R]              at /rustc/051478957371ee0084a7c0913941d2a8c4757bb9/library/std/src/sys/pal/unix/thread.rs:108:17
[R]   72: __pthread_joiner_wake
[R] 
[R]     at crates/ark/src/main.rs:281
[R]     in handle_signature_help
[R] 

@jmcphers
Copy link
Collaborator Author

Hmm... I believe I'm on a version of Ark with the fix. This may be a separate bug.

@jonvanausdeln
Copy link
Contributor

We aren't seeing this anymore in PTD . . will re-open if we see it again.

I also ran several R restart tests and all work as expected.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: workbench Issues related to Workbench category. lang: r
Projects
None yet
Development

No branches or pull requests

3 participants