Skip to content

Make server panic hook more error resilient #12610

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

Merged
merged 2 commits into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions crates/ruff/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,19 @@ pub fn main() -> ExitCode {
Err(err) => {
#[allow(clippy::print_stderr)]
{
use std::io::Write;

// Use `writeln` instead of `eprintln` to avoid panicking when the stderr pipe is broken.
let mut stderr = std::io::stderr().lock();

// This communicates that this isn't a linter error but ruff itself hard-errored for
// some reason (e.g. failed to resolve the configuration)
eprintln!("{}", "ruff failed".red().bold());
writeln!(stderr, "{}", "ruff failed".red().bold()).ok();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the original trace, I strongly suspect that ruff panics here... when trying to print the reason why ruff errored:

failed printing to stderr: Broken pipe (os error 32)
   0: ruff_server::message::init_messenger::{{closure}}
   1: std::panicking::rust_panic_with_hook
   2: std::panicking::begin_panic_handler::{{closure}}
   3: std::sys_common::backtrace::__rust_end_short_backtrace
   4: rust_begin_unwind
   5: core::panicking::panic_fmt
   6: std::io::stdio::_eprint
   7: ruff::main
   8: std::sys_common::backtrace::__rust_begin_short_backtrace
   9: std::rt::lang_start::{{closure}}
  10: std::rt::lang_start_internal
  11: main
  12: <unknown>
  13: __libc_start_main
  14: <unknown>

// Currently we generally only see one error, but e.g. with io errors when resolving
// the configuration it is help to chain errors ("resolving configuration failed" ->
// "failed to read file: subdir/pyproject.toml")
for cause in err.chain() {
eprintln!(" {} {cause}", "Cause:".bold());
writeln!(stderr, " {} {cause}", "Cause:".bold()).ok();
}
}
ExitStatus::Error.into()
Expand Down
50 changes: 14 additions & 36 deletions crates/ruff_server/src/message.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::sync::OnceLock;

use anyhow::Context;
use lsp_types::notification::Notification;
use std::sync::OnceLock;

use crate::server::ClientSender;

Expand All @@ -10,53 +10,31 @@ pub(crate) fn init_messenger(client_sender: ClientSender) {
MESSENGER
.set(client_sender)
.expect("messenger should only be initialized once");

// unregister any previously registered panic hook
let _ = std::panic::take_hook();

// When we panic, try to notify the client.
std::panic::set_hook(Box::new(move |panic_info| {
if let Some(messenger) = MESSENGER.get() {
let _ = messenger.send(lsp_server::Message::Notification(
lsp_server::Notification {
method: lsp_types::notification::ShowMessage::METHOD.into(),
params: serde_json::to_value(lsp_types::ShowMessageParams {
typ: lsp_types::MessageType::ERROR,
message: String::from(
"The Ruff language server exited with a panic. See the logs for more details."
),
})
.unwrap_or_default(),
},
));
}

let backtrace = std::backtrace::Backtrace::force_capture();
tracing::error!("{panic_info}\n{backtrace}");
#[allow(clippy::print_stderr)]
{
// we also need to print to stderr directly in case tracing hasn't
// been initialized.
eprintln!("{panic_info}\n{backtrace}");
}
}));
}
Comment on lines -13 to -42
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to move this out of Messenger to ensure that we unset the panic hook when the server shuts down. We may consider using catch_unwind on a per request/response model in the future. But I think that's good enough for now


pub(crate) fn show_message(message: String, message_type: lsp_types::MessageType) {
try_show_message(message, message_type).unwrap();
}

pub(super) fn try_show_message(
message: String,
message_type: lsp_types::MessageType,
) -> crate::Result<()> {
MESSENGER
.get()
.expect("messenger should be initialized")
.ok_or_else(|| anyhow::anyhow!("messenger not initialized"))?
.send(lsp_server::Message::Notification(
lsp_server::Notification {
method: lsp_types::notification::ShowMessage::METHOD.into(),
params: serde_json::to_value(lsp_types::ShowMessageParams {
typ: message_type,
message,
})
.unwrap(),
})?,
},
))
.expect("message should send");
.context("Failed to send message")?;

Ok(())
}

/// Sends an error to the client with a formatted message. The error is sent in a
Expand Down
47 changes: 44 additions & 3 deletions crates/ruff_server/src/server.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
//! Scheduling, I/O, and API endpoints.

use std::num::NonZeroUsize;
use std::str::FromStr;

use lsp_server as lsp;
use lsp_types as types;
use std::num::NonZeroUsize;
use std::panic::PanicInfo;
use std::str::FromStr;
use types::ClientCapabilities;
use types::CodeActionKind;
use types::CodeActionOptions;
Expand Down Expand Up @@ -36,6 +36,7 @@ mod client;
mod connection;
mod schedule;

use crate::message::try_show_message;
pub(crate) use connection::ClientSender;

pub(crate) type Result<T> = std::result::Result<T, api::Error>;
Expand Down Expand Up @@ -133,6 +134,46 @@ impl Server {
}

pub fn run(self) -> crate::Result<()> {
type PanicHook = Box<dyn Fn(&PanicInfo<'_>) + 'static + Sync + Send>;
struct RestorePanicHook {
hook: Option<PanicHook>,
}

impl Drop for RestorePanicHook {
fn drop(&mut self) {
if let Some(hook) = self.hook.take() {
std::panic::set_hook(hook);
}
}
}

// unregister any previously registered panic hook
// The hook will be restored when this function exits.
let _ = RestorePanicHook {
hook: Some(std::panic::take_hook()),
};

// When we panic, try to notify the client.
std::panic::set_hook(Box::new(move |panic_info| {
use std::io::Write;

let backtrace = std::backtrace::Backtrace::force_capture();
tracing::error!("{panic_info}\n{backtrace}");

// we also need to print to stderr directly for when using `$logTrace` because
// the message won't be sent to the client.
// But don't use `eprintln` because `eprintln` itself may panic if the pipe is broken.
let mut stderr = std::io::stderr().lock();
writeln!(stderr, "{panic_info}\n{backtrace}").ok();
Comment on lines +166 to +167
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main change is to use writeln here


try_show_message(
"The Ruff language server exited with a panic. See the logs for more details."
.to_string(),
lsp_types::MessageType::ERROR,
)
.ok();
}));

event_loop_thread(move || {
Self::event_loop(
&self.connection,
Expand Down
Loading