Merge pull request #1534 from mxdamien/feat/ShowStatusInUIAfterPanicI…

…nModel Return message string from model panic handler to be displayed in UI …
hannobraun · Feb 7, 2023 · ed08c4c · ed08c4c
2 parents 5da91e2 + c89b948
commit ed08c4c
Show file tree

Hide file tree

Showing 6 changed files with 65 additions and 27 deletions.
diff --git a/crates/fj-proc/src/expand.rs b/crates/fj-proc/src/expand.rs
@@ -64,9 +64,9 @@ impl ToTokens for Metadata {
         let Self { name, arguments } = self;
 
         tokens.extend(quote! {
-            fn metadata(&self) -> fj::models::ModelMetadata {
-                fj::models::ModelMetadata::new(#name)
-                #( .with_argument(#arguments) )*
+            fn metadata(&self) -> std::result::Result<fj::models::ModelMetadata, Box<dyn std::error::Error + Send + Sync +'static>> {
+                Ok(fj::models::ModelMetadata::new(#name)
+                #( .with_argument(#arguments) )*)
             }
         });
     }

diff --git a/crates/fj/src/abi/context.rs b/crates/fj/src/abi/context.rs
@@ -5,8 +5,13 @@ use crate::abi::ffi_safe::StringSlice;
 #[repr(C)]
 pub struct Context<'a> {
     user_data: *const c_void,
-    get_argument:
-        unsafe extern "C" fn(*const c_void, StringSlice) -> StringSlice,
+    get_argument: unsafe extern "C" fn(
+        *const c_void,
+        StringSlice,
+    ) -> crate::abi::ffi_safe::Result<
+        StringSlice,
+        crate::abi::ffi_safe::String,
+    >,
     _lifetime: PhantomData<&'a ()>,
 }
 
@@ -15,15 +20,24 @@ impl<'a> From<&'a &dyn crate::models::Context> for Context<'a> {
         unsafe extern "C" fn get_argument(
             user_data: *const c_void,
             name: StringSlice,
-        ) -> StringSlice {
+        ) -> crate::abi::ffi_safe::Result<
+            StringSlice,
+            crate::abi::ffi_safe::String,
+        > {
             let ctx = &*(user_data as *const &dyn crate::models::Context);
 
             match std::panic::catch_unwind(AssertUnwindSafe(|| {
                 ctx.get_argument(&name)
             })) {
-                Ok(Some(arg)) => StringSlice::from_str(arg),
-                Ok(None) => StringSlice::from_str(""),
-                Err(payload) => crate::abi::on_panic(payload),
+                Ok(Some(arg)) => {
+                    crate::abi::ffi_safe::Result::Ok(StringSlice::from_str(arg))
+                }
+                Ok(None) => {
+                    crate::abi::ffi_safe::Result::Ok(StringSlice::from_str(""))
+                }
+                Err(payload) => crate::abi::ffi_safe::Result::Err(
+                    crate::abi::on_panic(payload),
+                ),
             }
         }
 
@@ -47,9 +61,17 @@ impl crate::models::Context for Context<'_> {
 
             let name = StringSlice::from_str(name);
 
-            match get_argument(user_data, name).into_str() {
-                "" => None,
-                other => Some(other),
+            match name.trim().is_empty() {
+                true => None,
+                false => match get_argument(user_data, name) {
+                    super::ffi_safe::Result::Ok(other) => {
+                        match other.is_empty() {
+                            true => None,
+                            false => Some(other.into_str()),
+                        }
+                    }
+                    super::ffi_safe::Result::Err(_) => None,
+                },
             }
         }
     }

diff --git a/crates/fj/src/abi/ffi_safe.rs b/crates/fj/src/abi/ffi_safe.rs
@@ -338,7 +338,7 @@ impl Deref for StringSlice {
 #[derive(Debug)]
 #[repr(C)]
 pub struct BoxedError {
-    msg: String,
+    pub(crate) msg: String,
 }
 
 impl Display for BoxedError {

diff --git a/crates/fj/src/abi/mod.rs b/crates/fj/src/abi/mod.rs
@@ -72,7 +72,7 @@ pub use self::{
 /// struct MyModel { }
 ///
 /// impl Model for MyModel {
-///     fn metadata(&self) -> ModelMetadata { todo!() }
+///     fn metadata(&self) -> std::result::Result<fj::models::ModelMetadata, Box<(dyn std::error::Error + Send + Sync + 'static)>> { todo!() }
 ///
 ///     fn shape(&self, ctx: &dyn Context) -> Result<fj::Shape, Error> {
 ///         todo!()
@@ -111,12 +111,14 @@ macro_rules! register_model {
 pub type InitFunction = unsafe extern "C" fn(*mut Host<'_>) -> InitResult;
 pub type InitResult = ffi_safe::Result<Metadata, ffi_safe::BoxedError>;
 pub type ShapeResult = ffi_safe::Result<crate::Shape, ffi_safe::BoxedError>;
+pub type ModelMetadataResult =
+    ffi_safe::Result<ModelMetadata, ffi_safe::BoxedError>;
 
 /// The name of the function generated by [`register_model`].
 ///
 pub const INIT_FUNCTION_NAME: &str = "fj_model_init";
 
-fn on_panic(payload: Box<dyn Any + Send>) -> ! {
+fn on_panic(payload: Box<dyn Any + Send>) -> crate::abi::ffi_safe::String {
     let msg: &str =
         if let Some(s) = payload.downcast_ref::<std::string::String>() {
             s.as_str()
@@ -126,7 +128,5 @@ fn on_panic(payload: Box<dyn Any + Send>) -> ! {
             "A panic occurred"
         };
 
-    eprintln!("{msg}");
-    // It's not ideal, but panicking across the FFI boundary is UB.
-    std::process::abort();
+    crate::abi::ffi_safe::String::from(msg.to_string())
 }
diff --git a/crates/fj/src/abi/model.rs b/crates/fj/src/abi/model.rs
@@ -1,14 +1,14 @@
 use std::{os::raw::c_void, panic::AssertUnwindSafe};
 
 use crate::{
-    abi::{Context, ModelMetadata, ShapeResult},
+    abi::{Context, ModelMetadataResult, ShapeResult},
     models::Error,
 };
 
 #[repr(C)]
 pub struct Model {
     ptr: *mut c_void,
-    metadata: unsafe extern "C" fn(*mut c_void) -> ModelMetadata,
+    metadata: unsafe extern "C" fn(*mut c_void) -> ModelMetadataResult,
     shape: unsafe extern "C" fn(*mut c_void, Context<'_>) -> ShapeResult,
     free: unsafe extern "C" fn(*mut c_void),
 }
@@ -30,23 +30,35 @@ impl crate::models::Model for Model {
         }
     }
 
-    fn metadata(&self) -> crate::models::ModelMetadata {
+    fn metadata(&self) -> Result<crate::models::ModelMetadata, Error> {
         let Self { ptr, metadata, .. } = *self;
 
-        unsafe { metadata(ptr).into() }
+        let result = unsafe { metadata(ptr) };
+
+        match result {
+            super::ffi_safe::Result::Ok(meta) => Ok(meta.into()),
+            super::ffi_safe::Result::Err(err) => Err(err.into()),
+        }
     }
 }
 
 impl From<Box<dyn crate::models::Model>> for Model {
     fn from(m: Box<dyn crate::models::Model>) -> Self {
-        unsafe extern "C" fn metadata(user_data: *mut c_void) -> ModelMetadata {
+        unsafe extern "C" fn metadata(
+            user_data: *mut c_void,
+        ) -> ModelMetadataResult {
             let model = &*(user_data as *mut Box<dyn crate::models::Model>);
 
             match std::panic::catch_unwind(AssertUnwindSafe(|| {
                 model.metadata()
             })) {
-                Ok(meta) => meta.into(),
-                Err(payload) => crate::abi::on_panic(payload),
+                Ok(Ok(meta)) => ModelMetadataResult::Ok(meta.into()),
+                Ok(Err(err)) => ModelMetadataResult::Err(err.into()),
+                Err(payload) => {
+                    ModelMetadataResult::Err(crate::abi::ffi_safe::BoxedError {
+                        msg: crate::abi::on_panic(payload),
+                    })
+                }
             }
         }
 
@@ -61,7 +73,11 @@ impl From<Box<dyn crate::models::Model>> for Model {
             })) {
                 Ok(Ok(shape)) => ShapeResult::Ok(shape),
                 Ok(Err(err)) => ShapeResult::Err(err.into()),
-                Err(payload) => crate::abi::on_panic(payload),
+                Err(payload) => {
+                    ShapeResult::Err(crate::abi::ffi_safe::BoxedError {
+                        msg: crate::abi::on_panic(payload),
+                    })
+                }
             }
         }
 

diff --git a/crates/fj/src/models/model.rs b/crates/fj/src/models/model.rs
@@ -9,7 +9,7 @@ pub trait Model: Send + Sync {
     fn shape(&self, ctx: &dyn Context) -> Result<Shape, Error>;
 
     /// Get metadata for the model.
-    fn metadata(&self) -> ModelMetadata;
+    fn metadata(&self) -> Result<ModelMetadata, Error>;
 }
 
 #[cfg(test)]