From 48e4448e55b9d1ad3e53c5b0d87387d346ae288e Mon Sep 17 00:00:00 2001
From: Devyn Cairns <devyn.cairns@gmail.com>
Date: Tue, 16 Apr 2024 08:00:32 -0700
Subject: [PATCH] Add a panic unwind handler during plugin calls (#12526)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

# 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::shell::plugin_failed_to_decode

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

```

# Tests + Formatting
- :green_circle: `toolkit fmt`
- :green_circle: `toolkit clippy`
- :green_circle: `toolkit test`
- :green_circle: `toolkit test stdlib`
---
 crates/nu-plugin/src/plugin/mod.rs | 47 +++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 17 deletions(-)

diff --git a/crates/nu-plugin/src/plugin/mod.rs b/crates/nu-plugin/src/plugin/mod.rs
index 6ab62eb079..3366688a11 100644
--- a/crates/nu-plugin/src/plugin/mod.rs
+++ b/crates/nu-plugin/src/plugin/mod.rs
@@ -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);
             }
         };