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.

Sign up for GitHub

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

Jump to bottom

Make server panic hook more error resilient #12610

Merged
merged 2 commits into from
Aug 2, 2024
Merged

Make server panic hook more error resilient #12610

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ff2f9b
Make ruff-server panic hook more error resilient
MichaReiser Aug 1, 2024
a5457ff
Move panic hook out of messenger
MichaReiser Aug 2, 2024
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
View file
Open in desktop
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
14 changes: 8 additions & 6 deletions crates/ruff_server/src/message.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ pub(crate) fn init_messenger(client_sender: ClientSender) {

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

if let Some(messenger) = MESSENGER.get() {
let _ = messenger.send(lsp_server::Message::Notification(
lsp_server::Notification {
Expand All @@ -33,12 +35,12 @@ pub(crate) fn init_messenger(client_sender: ClientSender) {

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

// we also need to print to stderr directly in case tracing hasn't
// been initialized.
// But use `writeln` instead of `eprintln` to avoid panicking when the stderr pipe is broken.
let mut stderr = std::io::stderr().lock();
writeln!(stderr, "{panic_info}\n{backtrace}").ok();
}));
}

Expand Down
Loading