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

DAP server: Improved handling of Rust panic in the API. #1668

Open
noppej opened this issue Jul 13, 2023 · 5 comments
Open

DAP server: Improved handling of Rust panic in the API. #1668

noppej opened this issue Jul 13, 2023 · 5 comments
Assignees
Labels
hygiene Tasks related to improving code efficiency, readability, etc.

Comments

@noppej
Copy link
Contributor

noppej commented Jul 13, 2023

The DAP server runs as a background task when the VSCode extension is used. To ensure the process doesn't "disappear" when it encounters a Rust panic, we deliberately avoided all panic situations, by adding specific errors.

This practice adds complexity to the code, and @Yatekii suggested we look at using https://doc.rust-lang.org/std/panic/fn.catch_unwind.html to allow the DAP server to maintain good usability, while also allowing the ergonomic use of panic.

@noppej noppej added the hygiene Tasks related to improving code efficiency, readability, etc. label Jul 13, 2023
@noppej noppej self-assigned this Jul 13, 2023
@noppej
Copy link
Contributor Author

noppej commented Jul 26, 2023

As an experiment I implemented first std::panic::catch_unwind and then std::thread::spawn in probe_rs::cmd::dap_server, and they both behave pretty much the same. It protects the debugger from exiting on a panic, but the information available to the end user isn't very useful. See the following code snippet, as well as the documented TODO for why this was abandoned.

pub fn run(cmd: Cmd, time_offset: UtcOffset) -> Result<()> {
    let log_info_message = setup_logging(time_offset)?;

    // The debug server runs as a background process that the VSCode extension depends on.
    // If the server panics, the VSCode extension will not be able to communicate with it,
    // and the user will have no idea what happened.
    // The debug server and associated code relies on standard Rust error handling, and will
    // report `Err` values to the user, in appropriately formatted messages.
    // However, the various crates used by the debug server may panic, so we catch those panics
    // here, and report them to the user via the logging mechanism, which is configured in `setup_logging`,
    // to ensure the VSCode extension can pick up the error and report it to the user.
    match thread::Builder::new()
        .name("probe-rs DAP server".to_string())
        .spawn(move || debug(cmd.port, cmd.vscode, &log_info_message, time_offset))
    {
        Ok(dap_server_handle) => {
            if let Err(err) = dap_server_handle.join() {
                tracing::error!(
                    "Debug server thread panicked. Please report this as a bug: {err:?}"
                );
                // TODO: The value reported for `err` is not very helpful, because it is a `Box<dyn Any + Send + 'static>`.
                // [This RFC](https://github.com/rust-lang/rust/issues/96024) when it has run its course,
                // we can something like the proposed `err.request_value()` on the backtrace.
            }
        }
        Err(e) => {
            tracing::error!("Failed to spawn probe-rs DAP server thread: {e:?}");
        }
    };

    Ok(())
}

Coincidentally, the work that was done in probe-rs/vscode#63 and #1699, effectively means that Rust's panic message and stack trace, will be displayed to the user. In other words, this opens up the possibility to allow the use of panic!, expect!, todo!etc. in the API, and still provides an acceptable user experience.

Based on that, I will close this issue as completed.

@noppej noppej closed this as completed Jul 26, 2023
@Yatekii
Copy link
Member

Yatekii commented Jul 27, 2023

I think I would still implement this because it lets you shutdown gracefully instead of crashing the DAP server :)

@Yatekii Yatekii reopened this Jul 27, 2023
@noppej
Copy link
Contributor Author

noppej commented Jul 27, 2023

The code posted above is done and tested and can be implemented. However, the current version of Rust obscures the original panic message and stack trace. Until this RFC completes, the current experience is still better (more informative for the user). We can keep this issue open until then :)

@Yatekii
Copy link
Member

Yatekii commented Jul 27, 2023

Uhm, I am pretty sure it should be possible to log the panic :)

@noppej
Copy link
Contributor Author

noppej commented Jul 27, 2023

It is currently being logged, but as soon as you hide it behind a spawn or a catch_unwind, it is lost. I imagine that is why the RFC is there. If I'm mistaken, I'd love you to show me the way :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hygiene Tasks related to improving code efficiency, readability, etc.
Projects
None yet
Development

No branches or pull requests

2 participants