Try very hard to guard against all possible misuses of Thread

Prior to this commit, it was still possible to trigger panics in
`Executor` methods by misusing `Thread` and `Executor` in clever ways.
For example, by setting a single `Thread` as the main thread of *two*
`Executor`s, you could end up in a situation where you could
artificially make the main `Thread` of one executor have any arbitrary
mode, and this could trigger `unreachable!()` panics.

Now, we try much harder to guard against all of these possible
situations. We add a *new* error condition to `Executor::step` which is
purposefully *outside* of the normal Lua error handling, since it
indicates a probable bug in the Rust-side use of an `Executor`. Normal
`piccolo` code which uses `Executor` directly will probably always want
to *unwrap* this error, since it shouldn't be possible to cause this
error with correct `piccolo` use in normal cases.

src/error.rs

@@ -109,11 +109,15 @@ impl fmt::Display for RuntimeError {
 
 impl<E: Into<anyhow::Error>> From<E> for RuntimeError {
     fn from(err: E) -> Self {
-        Self(Arc::new(err.into()))
+        Self::new(err)
     }
 }
 
 impl RuntimeError {
+    pub fn new(err: impl Into<anyhow::Error>) -> Self {
+        Self(Arc::new(err.into()))
+    }
+
     pub fn root_cause(&self) -> &(dyn StdError + 'static) {
         self.0.root_cause()
     }
@@ -179,7 +183,7 @@ impl<'gc> From<RuntimeError> for Error<'gc> {
 
 impl<'gc, E: Into<anyhow::Error>> From<E> for Error<'gc> {
     fn from(error: E) -> Self {
-        Self::Runtime(RuntimeError::from(error))
+        Self::Runtime(RuntimeError::new(error))
     }
 }
 

src/lua.rs

@@ -11,8 +11,9 @@ use crate::{
     stash::{Fetchable, Stashable},
     stdlib::{load_base, load_coroutine, load_io, load_math, load_string, load_table},
     string::InternedStringSet,
-    Error, ExternError, FromMultiValue, FromValue, Fuel, IntoValue, Registry, Singleton,
-    StashedExecutor, String, Table, TypeError, Value,
+    thread::BadThreadMode,
+    Error, ExternError, FromMultiValue, FromValue, Fuel, IntoValue, Registry, RuntimeError,
+    Singleton, StashedExecutor, String, Table, TypeError, Value,
 };
 
 /// A value representing the main "execution context" of a Lua state.
@@ -263,16 +264,18 @@ impl Lua {
     ///
     /// This will periodically exit the arena in order to collect garbage concurrently with running
     /// Lua code.
-    pub fn finish(&mut self, executor: &StashedExecutor) {
+    pub fn finish(&mut self, executor: &StashedExecutor) -> Result<(), BadThreadMode> {
         const FUEL_PER_GC: i32 = 4096;
 
         loop {
             let mut fuel = Fuel::with(FUEL_PER_GC);
 
-            if self.enter(|ctx| ctx.fetch(executor).step(ctx, &mut fuel)) {
+            if self.enter(|ctx| ctx.fetch(executor).step(ctx, &mut fuel))? {
                 break;
             }
         }
+
+        Ok(())
     }
 
     /// Run the given executor to completion and then take return values from the returning thread.
@@ -283,7 +286,7 @@ impl Lua {
         &mut self,
         executor: &StashedExecutor,
     ) -> Result<R, ExternError> {
-        self.finish(executor);
+        self.finish(executor).map_err(RuntimeError::new)?;
         self.try_enter(|ctx| ctx.fetch(executor).take_result::<R>(ctx)?)
     }
 }

src/thread/executor.rs

@@ -53,12 +53,6 @@ pub type ExecutorInner<'gc> = RefLock<ExecutorState<'gc>>;
 /// control back and forth. All Lua code that is run is done so directly or indirectly by calling
 /// [`Executor::step`].
 ///
-/// If a `Thread` is currently in the network of threads being run by the `Executor`, then the
-/// `Executor` expects that the state of these threads will not be externally changed (by, for
-/// example, calling [`Thread::take_result`] or [`Thread::reset`]). This should not result in
-/// panics, but may result in `Executor::step` erroring in a way that means that no further progress
-/// can be amde.
-///
 /// # Panics
 ///
 /// `Executor` is dangerous to use from within any kind of Lua callback. It it not meant to be used
@@ -144,7 +138,18 @@ impl<'gc> Executor<'gc> {
                     ThreadMode::Result => ExecutorMode::Result,
                     ThreadMode::Normal => ExecutorMode::Normal,
                     ThreadMode::Suspended => ExecutorMode::Suspended,
-                    ThreadMode::Waiting => unreachable!(),
+                    ThreadMode::Waiting => {
+                        // This should never happen from correct `Executor` / `Thread` use. In
+                        // order for the main thread to be in the `Waiting` state with no thread
+                        // being waited on, that thread must have been used by two `Executor`s at
+                        // one time, and the *other* `Executor` must have moved it to the `Waiting`
+                        // state.
+                        //
+                        // We call this `ExecutorMode::Normal` since the main thread is still not in
+                        // some completed state, but calling `Executor::step` will never exit this
+                        // mode (only forever return a `BadThreadMode` error).
+                        ExecutorMode::Normal
+                    }
                     ThreadMode::Running => ExecutorMode::Running,
                 }
             }
@@ -161,71 +166,80 @@ impl<'gc> Executor<'gc> {
     /// Returns `false` if the method has exhausted its fuel, but there is more work to
     /// do, and returns `true` if no more progress can be made. If `true` is returned, then
     /// `Executor::mode()` will no longer be `ExecutorMode::Normal`.
-    pub fn step(self, ctx: Context<'gc>, fuel: &mut Fuel) -> bool {
+    ///
+    /// # Errors
+    ///
+    /// If a `Thread` being run by this `Executor` in an unexpected state, then this method will
+    /// return a `BadThreadMode` error.
+    ///
+    /// If a `Thread` is currently in the stack of threads being run by an `Executor`, then that
+    /// `Executor` expects to be the sole instance driving those threads to completion and expects
+    /// that the state of these threads will not be externally changed. This rule cannot be violated
+    /// from Lua or by normal `Rust` callbacks, only by purposefully misusing an `Executor` from
+    /// Rust by, for example, setting a single `Thread` as the main thread of two `Executor`s
+    /// at once or by manually calling [`Thread::take_result`] or [`Thread::reset`] on a thread
+    /// currently being run by an `Executor`.
+    ///
+    /// This is considered "outside" of a normal Lua or Rust callback error since it cannot be
+    /// triggered solely by Lua and likely indicates a bug in some Rust code, so this error is
+    /// delivered through a separate channel than normal results and cannot be caught by Lua.
+    pub fn step(self, ctx: Context<'gc>, fuel: &mut Fuel) -> Result<bool, BadThreadMode> {
         let mut state = self.0.borrow_mut(&ctx);
-
-        loop {
+        Ok(loop {
             let mut top_thread = state.thread_stack.last().copied().unwrap();
             let mut res_thread = None;
             match top_thread.mode() {
                 ThreadMode::Normal => {}
                 ThreadMode::Running => {
                     panic!("`Executor` thread already running")
                 }
-                _ => {
-                    if state.thread_stack.len() == 1 {
-                        break true;
-                    } else {
-                        state.thread_stack.pop();
-                        res_thread = Some(top_thread);
-                        top_thread = state.thread_stack.last().copied().unwrap();
-                    }
+                ThreadMode::Stopped | ThreadMode::Suspended | ThreadMode::Result
+                    if state.thread_stack.len() == 1 =>
+                {
+                    break true;
+                }
+                ThreadMode::Result => {
+                    state.thread_stack.pop();
+                    res_thread = Some(top_thread);
+                    top_thread = state.thread_stack.last().copied().unwrap();
+                }
+                mode => {
+                    return Err(BadThreadMode {
+                        found: mode,
+                        expected: None,
+                    })
                 }
             }
 
             let mut top_state = top_thread.into_inner().borrow_mut(&ctx);
             let top_state = &mut *top_state;
             if let Some(res_thread) = res_thread {
                 let mode = top_state.mode();
-                if mode == ThreadMode::Waiting {
-                    assert!(matches!(top_state.frames.pop(), Some(Frame::WaitThread)));
-                    match res_thread.mode() {
-                        ThreadMode::Result => {
-                            // Take the results from the res_thread and return them to our top
-                            // thread.
-                            let mut res_state = res_thread.into_inner().borrow_mut(&ctx);
-                            match res_state.take_result() {
-                                Ok(vals) => {
-                                    let bottom = top_state.stack.len();
-                                    top_state.stack.extend(vals);
-                                    top_state.return_to(bottom);
-                                }
-                                Err(err) => {
-                                    top_state.frames.push(Frame::Error(err.into()));
-                                }
-                            }
-                            drop(res_state);
-                        }
-                        ThreadMode::Normal => unreachable!(),
-                        res_mode => top_state.frames.push(Frame::Error(
-                            BadThreadMode {
-                                found: res_mode,
-                                expected: None,
-                            }
-                            .into(),
-                        )),
-                    }
-                } else {
+                if mode != ThreadMode::Waiting {
                     // Shenanigans have happened and the top thread has had its state externally
                     // changed.
-                    top_state.frames.push(Frame::Error(
-                        BadThreadMode {
-                            found: mode,
-                            expected: None,
-                        }
-                        .into(),
-                    ));
+                    return Err(BadThreadMode {
+                        found: mode,
+                        expected: Some(ThreadMode::Waiting),
+                    });
+                }
+
+                assert!(matches!(top_state.frames.pop(), Some(Frame::WaitThread)));
+                assert_eq!(res_thread.mode(), ThreadMode::Result);
+                // Take the results from the res_thread and return them to our top
+                // thread.
+                let mut res_state = res_thread.into_inner().borrow_mut(&ctx);
+                match res_state.take_result() {
+                    Ok(vals) => {
+                        let bottom = top_state.stack.len();
+                        top_state.stack.extend(vals);
+                        top_state.return_to(bottom);
+                    }
+                    Err(err) => {
+                        top_state.frames.push(Frame::Error(err.into()));
+                    }
                 }
+                drop(res_state);
             }
 
             if top_state.mode() == ThreadMode::Normal {
@@ -486,7 +500,7 @@ impl<'gc> Executor<'gc> {
             if !fuel.should_continue() {
                 break false;
             }
-        }
+        })
     }
 
     pub fn take_result<T: FromMultiValue<'gc>>(

src/thread/thread.rs

@@ -496,6 +496,13 @@ impl<'gc> ThreadState<'gc> {
         self.open_upvalues.truncate(start);
     }
 
+    pub(super) fn reset(&mut self, mc: &Mutation<'gc>) {
+        self.close_upvalues(mc, 0);
+        assert!(self.open_upvalues.is_empty());
+        self.stack.clear();
+        self.frames.clear();
+    }
+
     fn resurrect_live_upvalues(&self, fc: &Finalization<'gc>) {
         for &upval in &self.open_upvalues {
             if !Gc::is_dead(fc, UpValue::into_inner(upval)) {
@@ -520,13 +527,6 @@ impl<'gc> ThreadState<'gc> {
             }
         }
     }
-
-    fn reset(&mut self, mc: &Mutation<'gc>) {
-        self.close_upvalues(mc, 0);
-        assert!(self.open_upvalues.is_empty());
-        self.stack.clear();
-        self.frames.clear();
-    }
 }
 
 pub(super) struct LuaFrame<'gc, 'a> {

tests/callback.rs

@@ -285,15 +285,15 @@ fn resume_with_err() {
         ctx.stash(Executor::run(&ctx, thread).unwrap())
     });
 
-    lua.finish(&executor);
+    lua.finish(&executor).unwrap();
 
     lua.enter(|ctx| {
         let executor = ctx.fetch(&executor);
         assert!(executor.take_result::<String>(ctx).unwrap().unwrap() == "return");
         executor.resume(ctx, ()).unwrap();
     });
 
-    lua.finish(&executor);
+    lua.finish(&executor).unwrap();
 
     lua.enter(
         |ctx| match ctx.fetch(&executor).take_result::<()>(ctx).unwrap() {

tests/error.rs

@@ -22,7 +22,7 @@ fn error_unwind() -> Result<(), ExternError> {
         Ok(ctx.stash(Executor::start(ctx, closure.into(), ())))
     })?;
 
-    lua.finish(&executor);
+    lua.finish(&executor).unwrap();
     lua.try_enter(|ctx| {
         match ctx.fetch(&executor).take_result::<()>(ctx)? {
             Err(Error::Lua(LuaError(Value::String(s)))) => assert!(s == "test error"),

tests/fuel.rs

@@ -21,7 +21,7 @@ fn test_interrupt() -> Result<(), ExternError> {
     lua.enter(|ctx| {
         let executor = ctx.fetch(&executor);
         let mut fuel = Fuel::with(i32::MAX);
-        assert!(!executor.step(ctx, &mut fuel));
+        assert!(!executor.step(ctx, &mut fuel).unwrap());
         assert!(fuel.is_interrupted());
         assert!(executor.mode() == ExecutorMode::Normal)
     });

tests/userdata.rs

@@ -1,12 +1,12 @@
 use gc_arena::{lock::Lock, Collect, Gc, Rootable};
-use piccolo::{Callback, CallbackReturn, Closure, Executor, ExternError, Lua, UserData, Value};
+use piccolo::{Callback, CallbackReturn, Closure, Executor, Lua, UserData, Value};
 
 #[derive(Collect)]
 #[collect(no_drop)]
 struct MyUserData<'gc>(Gc<'gc, Lock<i32>>);
 
 #[test]
-fn userdata() -> Result<(), ExternError> {
+fn userdata() -> Result<(), anyhow::Error> {
     let mut lua = Lua::core();
 
     lua.try_enter(|ctx| {
@@ -43,7 +43,7 @@ fn userdata() -> Result<(), ExternError> {
         Ok(ctx.stash(Executor::start(ctx, closure.into(), ())))
     })?;
 
-    lua.finish(&executor);
+    lua.finish(&executor)?;
 
     lua.try_enter(|ctx| {
         let (ud, res) = ctx
@@ -58,5 +58,7 @@ fn userdata() -> Result<(), ExternError> {
 
         assert!(ud.downcast::<Rootable![MyUserData2]>().is_err());
         Ok(())
-    })
+    })?;
+
+    Ok(())
 }