Skip to content

Commit

Permalink
fix(core/config): catch panics during deserialization
Browse files Browse the repository at this point in the history
  • Loading branch information
loyd committed Jul 11, 2024
1 parent aa2d483 commit 259fe1b
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 18 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 9 additions & 1 deletion elfo-core/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<C> Config for C where C: for<'de> Deserialize<'de> + Send + Sync + fmt::Debug + 'static {}
Expand Down Expand Up @@ -57,6 +57,14 @@ impl AnyConfig {
}

pub(crate) fn decode<C: Config>(&self) -> Result<AnyConfig, String> {
match panic::sync_catch(|| self.do_decode::<C>()) {
Ok(Ok(config)) => Ok(config),
Ok(Err(err)) => Err(err),
Err(panic) => Err(panic),
}
}

fn do_decode<C: Config>(&self) -> Result<AnyConfig, String> {
let mut raw = (*self.raw).clone();

let system_decoded = if let Value::Map(map) = &mut raw {
Expand Down
1 change: 1 addition & 0 deletions elfo-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
28 changes: 28 additions & 0 deletions elfo-core/src/panic.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
use std::{
any::Any,
future::Future,
panic::{self, AssertUnwindSafe},
};

use futures::FutureExt;

pub(crate) fn sync_catch<R>(f: impl FnOnce() -> R) -> Result<R, String> {
panic::catch_unwind(AssertUnwindSafe(f)).map_err(panic_to_string)
}

pub(crate) async fn catch<R>(f: impl Future<Output = R>) -> Result<R, String> {
AssertUnwindSafe(f)
.catch_unwind()
.await
.map_err(panic_to_string)
}

fn panic_to_string(payload: Box<dyn Any + Send>) -> String {
if let Some(message) = payload.downcast_ref::<&str>() {
format!("panic: {message}")
} else if let Some(message) = payload.downcast_ref::<String>() {
format!("panic: {message}")
} else {
"panic: <unsupported payload>".to_string()
}
}
23 changes: 6 additions & 17 deletions elfo-core/src/supervisor.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -22,6 +20,7 @@ use crate::{
message::Request,
messages, msg,
object::{GroupVisitor, Object, OwnedObject},
panic,
restarting::{RestartBackoff, RestartPolicy},
routers::{Outcome, Router},
runtime::RuntimeManager,
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -508,13 +507,3 @@ fn extract_response_token<R: Request>(envelope: Envelope) -> ResponseToken<R> {
_ => unreachable!(),
})
}

fn panic_to_string(payload: Box<dyn Any>) -> String {
if let Some(message) = payload.downcast_ref::<&str>() {
format!("panic: {message}")
} else if let Some(message) = payload.downcast_ref::<String>() {
format!("panic: {message}")
} else {
"panic: <unsupported payload>".to_string()
}
}
22 changes: 22 additions & 0 deletions elfo/tests/update_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<D>(_deserializer: D) -> Result<Self, D::Error>
where
D: serde::de::Deserializer<'de>,
{
panic!("intentional panic");
}
}

let blueprint = ActorGroup::new()
.config::<BadConfig>()
.exec(|_| async { unreachable!() });

let _proxy = elfo::test::proxy(blueprint, AnyConfig::default()).await;
}

0 comments on commit 259fe1b

Please sign in to comment.