Skip to content

Commit

Permalink
fix: extract panic-utils crate and fix occasional incorrect downcasting
Browse files Browse the repository at this point in the history
Extract `panic-utils` crate and consistently use it for downcasting
panic payloads to strings across the codebase.

The implementation is a bit more efficient than what was used previously
and avoids unnecessary allocations, and also fixes a few bugs related to
only one of the two possible cases handled when downcasting panic
payloads. The most egregious was the case in `sql-query-connector` where
we tried downcasting a panic from the database driver to `String` and
then unwrapped the result, leading to possible double panics if the
original panic was of the form `panic!("message")` and not
`panic!("message: {details}")` and thus the panic payload was a `&static
str` and not a `String`.
  • Loading branch information
aqrln committed Feb 7, 2025
1 parent a728551 commit 20a5343
Show file tree
Hide file tree
Showing 14 changed files with 55 additions and 32 deletions.
9 changes: 9 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions libs/driver-adapters/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ workspace = true
async-trait.workspace = true
futures.workspace = true
once_cell = "1.15"
panic-utils.path = "../../libs/panic-utils"
prisma-metrics.path = "../../libs/metrics"
serde.workspace = true
serde_json.workspace = true
Expand Down
5 changes: 1 addition & 4 deletions libs/driver-adapters/src/napi/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,7 @@ where
}

fn panic_to_napi_err<R>(panic_payload: Box<dyn Any + Send>) -> napi::Result<R> {
panic_payload
.downcast_ref::<&str>()
.map(|s| -> String { (*s).to_owned() })
.or_else(|| panic_payload.downcast_ref::<String>().map(|s| s.to_owned()))
panic_utils::downcast_box_to_string(panic_payload)
.map(|message| Err(napi::Error::from_reason(format!("PANIC: {message}"))))
.ok_or(napi::Error::from_reason("PANIC: unknown panic".to_string()))
.unwrap()
Expand Down
9 changes: 9 additions & 0 deletions libs/panic-utils/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
[package]
name = "panic-utils"
version = "0.1.0"
edition = "2021"

[dependencies]

[lints]
workspace = true
18 changes: 18 additions & 0 deletions libs/panic-utils/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
use std::{any::Any, borrow::Cow};

/// Downcasts a boxed [`Any`] from a panic payload to a string.
pub fn downcast_box_to_string(object: Box<dyn Any>) -> Option<Cow<'static, str>> {
object
.downcast::<&'static str>()
.map(|s| Cow::Borrowed(*s))
.or_else(|object| object.downcast::<String>().map(|s| Cow::Owned(*s)))
.ok()
}

/// Downcasts a reference to [`Any`] from panic info hook to a string.
pub fn downcast_ref_to_string(object: &dyn Any) -> Option<&str> {
object
.downcast_ref::<&str>()
.copied()
.or_else(|| object.downcast_ref::<String>().map(|s| s.as_str()))
}
1 change: 1 addition & 0 deletions libs/user-facing-errors/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ version = "0.1.0"
edition = "2021"

[dependencies]
panic-utils.path = "../panic-utils"
user-facing-error-macros = { path = "../user-facing-error-macros" }
serde_json.workspace = true
serde.workspace = true
Expand Down
7 changes: 1 addition & 6 deletions libs/user-facing-errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,12 +122,7 @@ impl Error {
/// Construct a new UnknownError from a [`PanicHookInfo`] in a panic hook. [`UnknownError`]s
/// created with this constructor will have a proper, useful backtrace.
pub fn new_in_panic_hook(panic_info: &std::panic::PanicHookInfo<'_>) -> Self {
let message = panic_info
.payload()
.downcast_ref::<&str>()
.map(|s| -> String { (*s).to_owned() })
.or_else(|| panic_info.payload().downcast_ref::<String>().map(|s| s.to_owned()))
.unwrap_or_else(|| "<unknown panic>".to_owned());
let message = panic_utils::downcast_ref_to_string(panic_info.payload()).unwrap_or("<unknown panic>");

let backtrace = Some(format!("{:?}", backtrace::Backtrace::new()));
let location = panic_info
Expand Down
3 changes: 3 additions & 0 deletions query-engine/connectors/sql-query-connector/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,6 @@ version = "1.0"
[dependencies.user-facing-errors]
features = ["sql"]
path = "../../../libs/user-facing-errors"

[dependencies.panic-utils]
path = "../../../libs/panic-utils"
4 changes: 2 additions & 2 deletions query-engine/connectors/sql-query-connector/src/error.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use connector_interface::error::*;
use quaint::error::ErrorKind as QuaintKind;
use query_structure::{prelude::DomainError, Filter};
use std::{any::Any, string::FromUtf8Error};
use std::{any::Any, borrow::Cow, string::FromUtf8Error};
use thiserror::Error;
use user_facing_errors::query_engine::DatabaseConstraint;

Expand Down Expand Up @@ -100,7 +100,7 @@ impl From<Box<dyn Any + Send>> for RawError {
fn from(e: Box<dyn Any + Send>) -> Self {
Self::Database {
code: None,
message: Some(*e.downcast::<String>().unwrap()),
message: panic_utils::downcast_box_to_string(e).map(Cow::into_owned),
}
}
}
Expand Down
1 change: 1 addition & 0 deletions query-engine/query-engine/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ hyper = { version = "0.14", features = ["server", "http1", "http2", "runtime"] }
tracing.workspace = true
tracing-subscriber = { version = "0.3", features = ["json", "env-filter"] }
prisma-metrics.path = "../../libs/metrics"
panic-utils.path = "../../libs/panic-utils"

user-facing-errors = { path = "../../libs/user-facing-errors" }
telemetry = { path = "../../libs/telemetry" }
Expand Down
10 changes: 3 additions & 7 deletions query-engine/query-engine/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,25 +38,21 @@ async fn main() -> Result<(), AnyError> {
fn set_panic_hook(log_format: LogFormat) {
if let LogFormat::Json = log_format {
std::panic::set_hook(Box::new(|info| {
let payload = info
.payload()
.downcast_ref::<String>()
.cloned()
.unwrap_or_else(|| info.payload().downcast_ref::<&str>().unwrap().to_string());
let payload = panic_utils::downcast_ref_to_string(info.payload()).unwrap_or_default();

match info.location() {
Some(location) => {
tracing::event!(
tracing::Level::ERROR,
message = "PANIC",
reason = payload.as_str(),
reason = payload,
file = location.file(),
line = location.line(),
column = location.column(),
);
}
None => {
tracing::event!(tracing::Level::ERROR, message = "PANIC", reason = payload.as_str());
tracing::event!(tracing::Level::ERROR, message = "PANIC", reason = payload);
}
}

Expand Down
1 change: 1 addition & 0 deletions schema-engine/cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ schema-core = { path = "../core" }
user-facing-errors = { path = "../../libs/user-facing-errors", features = [
"all-native",
] }
panic-utils = { path = "../../libs/panic-utils" }

backtrace = "0.3.59"
base64 = "0.13"
Expand Down
7 changes: 1 addition & 6 deletions schema-engine/cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,7 @@ async fn main() {

fn set_panic_hook() {
std::panic::set_hook(Box::new(move |panic_info| {
let message = panic_info
.payload()
.downcast_ref::<&str>()
.copied()
.or_else(|| panic_info.payload().downcast_ref::<String>().map(|s| s.as_str()))
.unwrap_or("<unknown panic>");
let message = panic_utils::downcast_ref_to_string(panic_info.payload()).unwrap_or("<unknown panic>");

let location = panic_info
.location()
Expand Down
11 changes: 4 additions & 7 deletions schema-engine/cli/tests/cli_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,10 @@ where
match res {
Ok(_) => (),
Err(panic_payload) => {
let res = panic_payload
.downcast_ref::<&str>()
.map(|s| -> String { (*s).to_owned() })
.or_else(|| panic_payload.downcast_ref::<String>().map(|s| s.to_owned()))
.unwrap_or_default();

panic!("Error: '{}'", res);
panic!(
"Error: '{}'",
panic_utils::downcast_box_to_string(panic_payload).unwrap_or_default()
);
}
}
}
Expand Down

0 comments on commit 20a5343

Please sign in to comment.