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

Panics on failed to send a message: SendError #18055

Closed
ConradIrwin opened this issue Sep 5, 2024 · 9 comments · Fixed by #18105
Closed

Panics on failed to send a message: SendError #18055

ConradIrwin opened this issue Sep 5, 2024 · 9 comments · Fixed by #18105
Labels
Broken Window Bugs / technical debt to be addressed immediately

Comments

@ConradIrwin
Copy link

We've noticed when using rust analyzer on the zed codebase that it crashes every 10-30 minutes:

stderr: thread '<unnamed>' panicked at /Users/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/lsp-server-0.7.6/src/stdio.rs:28:37:
stderr: receiver was dropped, failed to send a message: "SendError(..)"
stderr: stack backtrace:
stderr: 0: _rust_begin_unwind
stderr: 1: core::panicking::panic_fmt
stderr: 2: core::result::unwrap_failed
stderr: note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stderr: thread 'LspServer' panicked at /Users/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/lsp-server-0.7.6/src/stdio.rs:59:17:
stderr: Box<dyn Any>
stderr: stack backtrace:
stderr: 0: std::panicking::begin_panic
stderr: 1: lsp_server::stdio::IoThreads::join
stderr: 2: rust_analyzer::run_server
stderr: note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stderr: thread 'main' panicked at /Users/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/jod-thread-0.1.2/src/lib.rs:33:22:
stderr: called `Result::unwrap()` on an `Err` value: Any { .. }
stderr: stack backtrace:
stderr: 0: _rust_begin_unwind
stderr: 1: core::panicking::panic_fmt
stderr: 2: core::result::unwrap_failed
stderr: 3: stdx::thread::JoinHandle<T>::join
stderr: 4: rust_analyzer::with_extra_thread
stderr: 5: rust_analyzer::main
stderr: note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

We have also seen a few users reporting this (presumably on their own projects) at zed-industries/zed#17376.

@ConradIrwin ConradIrwin added the Broken Window Bugs / technical debt to be addressed immediately label Sep 5, 2024
@Veykril
Copy link
Member

Veykril commented Sep 6, 2024

All of those are follow up errors, the original cause isn't in that log (something panicked outside a catch_unwind)

@Veykril
Copy link
Member

Veykril commented Sep 6, 2024

#18055 should remove some of the noise

bors added a commit that referenced this issue Sep 6, 2024
fix: Don't panic lsp writer thread on dropped receiver

Should reduce the noise a bit (#18055). This removes the panic (and a follow up panic) when the server incorrectly shuts down, turning it into a proper late exit error.
@Oppen
Copy link

Oppen commented Sep 11, 2024

What I see in the code is that IoThreads::join always panics on error.
From the backtrace I believe the crash happens either here or here. Both are error recovery paths, so I believe they could safely ignore panics with catch_unwind.
Of course, I may be missing context due to inlining or be plainly wrong in my assessment, but unless that's the case, would it be reasonable to add catch_unwind there?

@Veykril
Copy link
Member

Veykril commented Sep 11, 2024

IoThreads::join panicking is just extra noise only occuring when something else panicked (we have changed the behavior for IoThreads::join last week though because its so noisy). catch_unwind won't do anything, as stated above, all the panic messages in the posted backtrace are follow up panics, the actual cause for the issue is missing in the backtrace.

@ConradIrwin
Copy link
Author

This is the error I get now:

Interestingly the file does seem to exist, it's just not a rust file?

stderr: Error: file not found: /Users/conrad/0/zed/zed/a.txt
stderr: sending on a disconnected channel
stderr:
stderr: Stack backtrace:
stderr: 0: std::backtrace::Backtrace::create
stderr: 1: anyhow::error::<impl anyhow::Error>::msg
stderr: 2: anyhow::__private::format_err
stderr: 3: rust_analyzer::run_server
stderr: 4: std::sys::backtrace::__rust_begin_short_backtrace
stderr: 5: core::ops::function::FnOnce::call_once{{vtable.shim}}
stderr: 6: std::sys::pal::unix::thread::Thread::new::thread_start
stderr: 7: __pthread_joiner_wake

@Oppen
Copy link

Oppen commented Sep 11, 2024

It doesn't look for the file but whether it has been intended from what I've seen, so I think you're onto something there.

@Veykril
Copy link
Member

Veykril commented Sep 12, 2024

Ah okay, we are returning an error in a notification handler (which exits the main loop) hence all the other panics due to dropping channels unexpectedly

@Veykril
Copy link
Member

Veykril commented Sep 12, 2024

The only handler exerting this behavior is for textDocument/didSave. Zed incorrectly tells r-a that a file was saved that r-a did not register the event for

let mut selectors = vec![
lsp_types::DocumentFilter {
language: None,
scheme: None,
pattern: Some("**/*.rs".into()),
},
lsp_types::DocumentFilter {
language: None,
scheme: None,
pattern: Some("**/Cargo.toml".into()),
},
lsp_types::DocumentFilter {
language: None,
scheme: None,
pattern: Some("**/Cargo.lock".into()),
},
];
selectors.extend(additional_filters);
let save_registration_options = lsp_types::TextDocumentSaveRegistrationOptions {
include_text: Some(false),
text_document_registration_options: lsp_types::TextDocumentRegistrationOptions {
document_selector: Some(selectors),
},
};
let registration = lsp_types::Registration {
id: "textDocument/didSave".to_owned(),
method: "textDocument/didSave".to_owned(),
register_options: Some(serde_json::to_value(save_registration_options).unwrap()),
};
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_didSave

(now we still shouldnt exit here because of this either)

@ConradIrwin
Copy link
Author

Sorry about that, and thanks for the fix! Let me see if we can figure out why that regressed on our end.

lnicola pushed a commit to lnicola/rust that referenced this issue Sep 25, 2024
fix: Don't panic lsp writer thread on dropped receiver

Should reduce the noise a bit (rust-lang/rust-analyzer#18055). This removes the panic (and a follow up panic) when the server incorrectly shuts down, turning it into a proper late exit error.
lnicola pushed a commit to lnicola/rust that referenced this issue Sep 25, 2024
fix: Faulty notifications should not bring down the server

Fixes rust-lang/rust-analyzer#18055, if a client sends us an unregistered document path in a did save notification it would force us to exit the thread. That is obviously not great behavior, we should be more fallible here
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Broken Window Bugs / technical debt to be addressed immediately
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants