Add a panic unwind handler during plugin calls (#12526)

# Description
If a panic happens during a plugin call, because it always happens
outside of the main thread, it currently just hangs Nushell because the
plugin stays running without ever producing a response to the call.

This adds a panic handler that calls `exit(1)` after the unwind finishes
to the plugin runner. The panic error is still printed to stderr as
always, and waiting for the unwind to finish helps to ensure that
anything on the stack with `Drop` behavior that needed to run still
runs, at least on that thread.

# User-Facing Changes
Panics now look like this, which is what they looked like before the
plugin behavior was moved to a separate thread:

```
thread 'plugin runner (primary)' panicked at crates/nu_plugin_example/src/commands/main.rs:45:9:
Test panic
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Error: nu:🐚:plugin_failed_to_decode

  × Plugin failed to decode: Failed to receive response to plugin call

```

# Tests + Formatting
- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`
This commit is contained in:
Devyn Cairns 2024-04-16 08:00:32 -07:00 committed by GitHub
parent e6fbf7d01d
commit 48e4448e55
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

View File

@ -11,6 +11,7 @@ use std::{
ffi::OsString,
io::{BufReader, BufWriter},
ops::Deref,
panic::AssertUnwindSafe,
path::Path,
process::{Child, Command as CommandSys},
sync::{
@ -557,6 +558,9 @@ pub enum ServePluginError {
/// A thread spawning error occurred.
#[error("{0}")]
ThreadSpawnError(#[source] std::io::Error),
/// A panic occurred.
#[error("a panic occurred in a plugin thread")]
Panicked,
}
impl From<ShellError> for ServePluginError {
@ -655,23 +659,32 @@ where
// Handle each Run plugin call on a thread
thread::scope(|scope| {
let run = |engine, call_info| {
let CallInfo { name, call, input } = call_info;
let result = if let Some(command) = commands.get(&name) {
command.run(plugin, &engine, &call, input)
} else {
Err(
LabeledError::new(format!("Plugin command not found: `{name}`")).with_label(
format!("plugin `{plugin_name}` doesn't have this command"),
call.head,
),
)
};
let write_result = engine
.write_response(result)
.and_then(|writer| writer.write())
.try_to_report(&engine);
if let Err(err) = write_result {
let _ = error_tx.send(err);
// SAFETY: It should be okay to use `AssertUnwindSafe` here, because we don't use any
// of the references after we catch the unwind, and immediately exit.
let unwind_result = std::panic::catch_unwind(AssertUnwindSafe(|| {
let CallInfo { name, call, input } = call_info;
let result = if let Some(command) = commands.get(&name) {
command.run(plugin, &engine, &call, input)
} else {
Err(
LabeledError::new(format!("Plugin command not found: `{name}`"))
.with_label(
format!("plugin `{plugin_name}` doesn't have this command"),
call.head,
),
)
};
let write_result = engine
.write_response(result)
.and_then(|writer| writer.write())
.try_to_report(&engine);
if let Err(err) = write_result {
let _ = error_tx.send(err);
}
}));
if unwind_result.is_err() {
// Exit after unwind if a panic occurred
std::process::exit(1);
}
};