From 259fe1ba5857de5d799e16e03665787ccdfb3daa Mon Sep 17 00:00:00 2001 From: Paul Loyd Date: Thu, 11 Jul 2024 18:12:02 +0400 Subject: [PATCH] fix(core/config): catch panics during deserialization --- CHANGELOG.md | 1 + elfo-core/src/config.rs | 10 +++++++++- elfo-core/src/lib.rs | 1 + elfo-core/src/panic.rs | 28 ++++++++++++++++++++++++++++ elfo-core/src/supervisor.rs | 23 ++++++----------------- elfo/tests/update_config.rs | 22 ++++++++++++++++++++++ 6 files changed, 67 insertions(+), 18 deletions(-) create mode 100644 elfo-core/src/panic.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 3437a91d..eed4cf91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - core/request: avoid all-request to multiple groups hanging in some cases ([#127]). +- core/config: catch panics during deserialization of user configs. [#52]: https://github.com/elfo-rs/elfo/issues/52 [#127]: https://github.com/elfo-rs/elfo/pull/127 diff --git a/elfo-core/src/config.rs b/elfo-core/src/config.rs index 423bb52d..0612156c 100644 --- a/elfo-core/src/config.rs +++ b/elfo-core/src/config.rs @@ -9,7 +9,7 @@ use derive_more::From; use serde::{de, de::value::Error as DeError, Deserialize, Deserializer, Serialize, Serializer}; use serde_value::{Value, ValueDeserializer}; -use crate::local::Local; +use crate::{local::Local, panic}; pub trait Config: for<'de> Deserialize<'de> + Send + Sync + fmt::Debug + 'static {} impl Config for C where C: for<'de> Deserialize<'de> + Send + Sync + fmt::Debug + 'static {} @@ -57,6 +57,14 @@ impl AnyConfig { } pub(crate) fn decode(&self) -> Result { + match panic::sync_catch(|| self.do_decode::()) { + Ok(Ok(config)) => Ok(config), + Ok(Err(err)) => Err(err), + Err(panic) => Err(panic), + } + } + + fn do_decode(&self) -> Result { let mut raw = (*self.raw).clone(); let system_decoded = if let Value::Map(map) = &mut raw { diff --git a/elfo-core/src/lib.rs b/elfo-core/src/lib.rs index 119deca9..f1d1d38d 100644 --- a/elfo-core/src/lib.rs +++ b/elfo-core/src/lib.rs @@ -61,6 +61,7 @@ mod mailbox; mod memory_tracker; mod message; mod object; +mod panic; mod permissions; #[cfg(all(feature = "network", feature = "unstable"))] pub mod remote; diff --git a/elfo-core/src/panic.rs b/elfo-core/src/panic.rs new file mode 100644 index 00000000..f3c2a984 --- /dev/null +++ b/elfo-core/src/panic.rs @@ -0,0 +1,28 @@ +use std::{ + any::Any, + future::Future, + panic::{self, AssertUnwindSafe}, +}; + +use futures::FutureExt; + +pub(crate) fn sync_catch(f: impl FnOnce() -> R) -> Result { + panic::catch_unwind(AssertUnwindSafe(f)).map_err(panic_to_string) +} + +pub(crate) async fn catch(f: impl Future) -> Result { + AssertUnwindSafe(f) + .catch_unwind() + .await + .map_err(panic_to_string) +} + +fn panic_to_string(payload: Box) -> String { + if let Some(message) = payload.downcast_ref::<&str>() { + format!("panic: {message}") + } else if let Some(message) = payload.downcast_ref::() { + format!("panic: {message}") + } else { + "panic: ".to_string() + } +} diff --git a/elfo-core/src/supervisor.rs b/elfo-core/src/supervisor.rs index 775cd572..e6cdbc22 100644 --- a/elfo-core/src/supervisor.rs +++ b/elfo-core/src/supervisor.rs @@ -1,9 +1,7 @@ -use std::{ - any::Any, future::Future, mem, ops::Deref, panic::AssertUnwindSafe, sync::Arc, time::Duration, -}; +use std::{future::Future, mem, ops::Deref, sync::Arc, time::Duration}; use dashmap::DashMap; -use futures::{future::BoxFuture, FutureExt}; +use futures::future::BoxFuture; use fxhash::FxBuildHasher; use metrics::{decrement_gauge, increment_gauge}; use parking_lot::RwLock; @@ -22,6 +20,7 @@ use crate::{ message::Request, messages, msg, object::{GroupVisitor, Object, OwnedObject}, + panic, restarting::{RestartBackoff, RestartPolicy}, routers::{Outcome, Router}, runtime::RuntimeManager, @@ -323,11 +322,11 @@ where // It must be called after `entry.insert()`. let ctx = ctx.with_addr(addr).with_start_info(start_info); - let fut = AssertUnwindSafe(async { sv.exec.exec(ctx).await.unify() }).catch_unwind(); - let new_status = match fut.await { + let fut = async { sv.exec.exec(ctx).await.unify() }; + let new_status = match panic::catch(fut).await { Ok(Ok(())) => ActorStatus::TERMINATED, Ok(Err(err)) => ActorStatus::FAILED.with_details(ErrorChain(&*err)), - Err(panic) => ActorStatus::FAILED.with_details(panic_to_string(panic)), + Err(panic) => ActorStatus::FAILED.with_details(panic), }; let restart_after = { @@ -508,13 +507,3 @@ fn extract_response_token(envelope: Envelope) -> ResponseToken { _ => unreachable!(), }) } - -fn panic_to_string(payload: Box) -> String { - if let Some(message) = payload.downcast_ref::<&str>() { - format!("panic: {message}") - } else if let Some(message) = payload.downcast_ref::() { - format!("panic: {message}") - } else { - "panic: ".to_string() - } -} diff --git a/elfo/tests/update_config.rs b/elfo/tests/update_config.rs index b6442d68..522eb869 100644 --- a/elfo/tests/update_config.rs +++ b/elfo/tests/update_config.rs @@ -75,3 +75,25 @@ async fn singleton_actor_update_config() { )); assert_eq!(proxy.request(GetLimit).await, 512); } + +#[tokio::test] +#[should_panic(expected = "subject:\n- panic: intentional panic")] +async fn panic_in_deserialize() { + #[derive(Debug, Clone)] + struct BadConfig; + + impl<'de> Deserialize<'de> for BadConfig { + fn deserialize(_deserializer: D) -> Result + where + D: serde::de::Deserializer<'de>, + { + panic!("intentional panic"); + } + } + + let blueprint = ActorGroup::new() + .config::() + .exec(|_| async { unreachable!() }); + + let _proxy = elfo::test::proxy(blueprint, AnyConfig::default()).await; +}