diff --git a/Cargo.lock b/Cargo.lock index 3584d8ab..277cf9ca 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -288,9 +288,9 @@ dependencies = [ [[package]] name = "addr2line" -version = "0.15.2" +version = "0.16.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e7a2e47a1fbe209ee101dd6d61285226744c6c8d3c21c8dc878ba6cb9f467f3a" +checksum = "3e61f2b7f93d2c7d2b08263acaa4a363b3e276806c68af6134c44f523bf1aacd" dependencies = [ "gimli", ] @@ -382,9 +382,9 @@ dependencies = [ [[package]] name = "backtrace" -version = "0.3.60" +version = "0.3.61" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b7815ea54e4d821e791162e078acbebfd6d8c8939cd559c9335dceb1c8ca7282" +checksum = "e7a905d892734eea339e896738c14b9afce22b5318f64b951e70bf3844419b01" dependencies = [ "addr2line", "cc", @@ -768,9 +768,9 @@ dependencies = [ [[package]] name = "curl-sys" -version = "0.4.44+curl-7.77.0" +version = "0.4.45+curl-7.78.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4b6d85e9322b193f117c966e79c2d6929ec08c02f339f950044aba12e20bbaf1" +checksum = "de9e5a72b1c744eb5dd20b2be4d7eb84625070bb5c4ab9b347b70464ab1e62eb" dependencies = [ "cc", "libc", @@ -1193,9 +1193,9 @@ dependencies = [ [[package]] name = "gimli" -version = "0.24.0" +version = "0.25.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0e4075386626662786ddb0ec9081e7c7eeb1ba31951f447ca780ef9f5d568189" +checksum = "f0a01e0497841a3b2db4f8afa483cce65f7e96a3498bd6c541734792aeac8fe7" [[package]] name = "glob" @@ -1867,9 +1867,9 @@ dependencies = [ [[package]] name = "object" -version = "0.25.3" +version = "0.26.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a38f2be3697a57b4060074ff41b44c16870d916ad7877c17696e063257482bc7" +checksum = "c55827317fb4c08822499848a14237d2874d6f139828893017237e7ab93eb386" dependencies = [ "memchr", ] @@ -2903,6 +2903,7 @@ dependencies = [ "actix-web", "actix-web-httpauth", "async-trait", + "backtrace", "base64 0.13.0", "bb8", "bytes 1.0.1", @@ -2915,7 +2916,6 @@ dependencies = [ "diesel_migrations", "docopt", "env_logger", - "failure", "futures 0.3.15", "googleapis-raw", "grpcio", @@ -2934,6 +2934,7 @@ dependencies = [ "regex", "scheduled-thread-pool", "sentry", + "sentry-backtrace", "serde 1.0.126", "serde_derive", "serde_json", @@ -2945,6 +2946,7 @@ dependencies = [ "slog-scope", "slog-stdlog", "slog-term", + "thiserror", "time 0.2.27", "tokio", "url 2.2.2", @@ -2957,9 +2959,9 @@ dependencies = [ [[package]] name = "synstructure" -version = "0.12.4" +version = "0.12.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b834f2d66f734cb897113e34aaff2f1ab4719ca946f9a7358dba8f8064148701" +checksum = "474aaa926faa1603c40b7885a9eaea29b444d1cb2850cb7c0e37bb1a4182f4fa" dependencies = [ "proc-macro2", "quote", @@ -3009,18 +3011,18 @@ dependencies = [ [[package]] name = "thiserror" -version = "1.0.25" +version = "1.0.26" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fa6f76457f59514c7eeb4e59d891395fab0b2fd1d40723ae737d64153392e9c6" +checksum = "93119e4feac1cbe6c798c34d3a53ea0026b0b1de6a120deef895137c0529bfe2" dependencies = [ "thiserror-impl", ] [[package]] name = "thiserror-impl" -version = "1.0.25" +version = "1.0.26" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8a36768c0fbf1bb15eca10defa29526bda730a2376c2ab4393ccfa16fb1a318d" +checksum = "060d69a0afe7796bf42e9e2ff91f5ee691fb15c53d38b4b62a9a53eb23164745" dependencies = [ "proc-macro2", "quote", diff --git a/Cargo.toml b/Cargo.toml index dcd18825..77be88a5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,6 +21,7 @@ actix-web-httpauth = "0.5" actix-rt = "1" # Pin to 1.0, due to dependencies on Tokio actix-cors = "0.5" async-trait = "0.1.40" +backtrace = "0.3.61" base64 = "0.13" bb8 = "0.4.1" # pin to 0.4 due to dependencies on Tokio bytes = "1.0" @@ -36,7 +37,6 @@ diesel_logger = "0.1.1" diesel_migrations = { version = "1.4.0", features = ["mysql"] } docopt = "1.1.0" env_logger = "0.8" -failure = "0.1.8" futures = { version = "0.3", features = ["compat"] } googleapis-raw = { version = "0", path = "vendor/mozilla-rust-sdk/googleapis-raw" } # Some versions of OpenSSL 1.1.1 conflict with grpcio's built-in boringssl which can cause @@ -58,7 +58,8 @@ protobuf = "2.20.0" rand = "0.8" regex = "1.4" # pin to 0.19: https://github.com/getsentry/sentry-rust/issues/277 -sentry = { version = "0.19", features = ["with_curl_transport"] } # pin to 0.19 due to curl transport, failure +sentry = { version = "0.19", features = ["with_curl_transport"] }# pin to 0.19 due to curl transport, failure +sentry-backtrace = "0.19" serde = "1.0" serde_derive = "1.0" serde_json = { version = "1.0", features = ["arbitrary_precision"] } @@ -71,6 +72,7 @@ slog-mozlog-json = "0.1" slog-scope = "4.3" slog-stdlog = "4.1" slog-term = "2.6" +thiserror = "1.0.26" time = "^0.2.25" # pinning to 0.2.4 due to high number of dependencies (actix, bb8, deadpool, etc.) tokio = { version = "0.2.4", features = ["macros", "sync"] } diff --git a/src/db/error.rs b/src/db/error.rs index d8f90682..731c1f08 100644 --- a/src/db/error.rs +++ b/src/db/error.rs @@ -1,71 +1,68 @@ use std::fmt; use actix_web::http::StatusCode; -use failure::{Backtrace, Context, Fail}; +use thiserror::Error; -#[derive(Debug)] +#[derive(Error, Debug)] pub struct DbError { - inner: Context, + kind: DbErrorKind, pub status: StatusCode, } -#[derive(Debug, Fail)] +#[derive(Debug, Error)] pub enum DbErrorKind { - #[fail(display = "A database error occurred: {}", _0)] - DieselQuery(#[cause] diesel::result::Error), + #[error("A database error occurred: {}", _0)] + DieselQuery(#[from] diesel::result::Error), - #[fail( - display = "An error occurred while establishing a db connection: {}", - _0 - )] - DieselConnection(#[cause] diesel::result::ConnectionError), + #[error("An error occurred while establishing a db connection: {}", _0)] + DieselConnection(#[from] diesel::result::ConnectionError), - #[fail(display = "A database error occurred: {}", _0)] - SpannerGrpc(#[cause] grpcio::Error), + #[error("A database error occurred: {}", _0)] + SpannerGrpc(#[from] grpcio::Error), - #[fail(display = "Spanner data load too large: {}", _0)] + #[error("Spanner data load too large: {}", _0)] SpannerTooLarge(String), - #[fail(display = "A database pool error occurred: {}", _0)] + #[error("A database pool error occurred: {}", _0)] Pool(diesel::r2d2::PoolError), - #[fail(display = "Error migrating the database: {}", _0)] + #[error("Error migrating the database: {}", _0)] Migration(diesel_migrations::RunMigrationsError), - #[fail(display = "Specified collection does not exist")] + #[error("Specified collection does not exist")] CollectionNotFound, - #[fail(display = "Specified bso does not exist")] + #[error("Specified bso does not exist")] BsoNotFound, - #[fail(display = "Specified batch does not exist")] + #[error("Specified batch does not exist")] BatchNotFound, - #[fail(display = "Tokenserver user not found")] + #[error("Tokenserver user not found")] TokenserverUserNotFound, - #[fail(display = "An attempt at a conflicting write")] + #[error("An attempt at a conflicting write")] Conflict, - #[fail(display = "Database integrity error: {}", _0)] + #[error("Database integrity error: {}", _0)] Integrity(String), - #[fail(display = "Invalid SYNC_DATABASE_URL: {}", _0)] + #[error("Invalid SYNC_DATABASE_URL: {}", _0)] InvalidUrl(String), - #[fail(display = "Unexpected error: {}", _0)] + #[error("Unexpected error: {}", _0)] Internal(String), - #[fail(display = "User over quota")] + #[error("User over quota")] Quota, - #[fail(display = "Connection expired")] + #[error("Connection expired")] Expired, } impl DbError { pub fn kind(&self) -> &DbErrorKind { - self.inner.get_context() + &self.kind } pub fn internal(msg: &str) -> Self { @@ -73,20 +70,20 @@ impl DbError { } pub fn is_reportable(&self) -> bool { - !matches!(self.inner.get_context(), DbErrorKind::Conflict) + !matches!(&self.kind, DbErrorKind::Conflict) } pub fn metric_label(&self) -> Option { - match self.inner.get_context() { + match &self.kind { DbErrorKind::Conflict => Some("storage.conflict".to_owned()), _ => None, } } } -impl From> for DbError { - fn from(inner: Context) -> Self { - let status = match inner.get_context() { +impl From for DbError { + fn from(kind: DbErrorKind) -> Self { + let status = match kind { DbErrorKind::TokenserverUserNotFound | DbErrorKind::CollectionNotFound | DbErrorKind::BsoNotFound => StatusCode::NOT_FOUND, @@ -102,11 +99,11 @@ impl From> for DbError { _ => StatusCode::INTERNAL_SERVER_ERROR, }; - Self { inner, status } + Self { kind, status } } } -failure_boilerplate!(DbError, DbErrorKind); +impl_fmt_display!(DbError, DbErrorKind); from_error!(diesel::result::Error, DbError, DbErrorKind::DieselQuery); from_error!( diff --git a/src/db/spanner/pool.rs b/src/db/spanner/pool.rs index a27127e2..cb0e8e0c 100644 --- a/src/db/spanner/pool.rs +++ b/src/db/spanner/pool.rs @@ -192,10 +192,10 @@ impl Default for CollectionCache { #[derive(Debug, Clone, Copy)] pub struct LoggingErrorSink; -impl ErrorSink for LoggingErrorSink { +impl ErrorSink for LoggingErrorSink { fn sink(&self, e: E) { error!("bb8 Error: {}", e); - let event = sentry::integrations::failure::event_from_fail(&e); + let event = sentry::event_from_error(&e); sentry::capture_event(event); } diff --git a/src/error.rs b/src/error.rs index 60742763..66a84c80 100644 --- a/src/error.rs +++ b/src/error.rs @@ -4,7 +4,7 @@ // this cascades into Failure requiring std::error::Error being implemented // which is out of scope. #![allow(clippy::single_match, clippy::large_enum_variant)] - +use backtrace::Backtrace; use std::convert::From; use std::fmt; @@ -15,15 +15,18 @@ use actix_web::{ middleware::errhandlers::ErrorHandlerResponse, HttpResponse, Result, }; -use failure::{Backtrace, Context, Fail}; + use serde::{ ser::{SerializeMap, SerializeSeq, Serializer}, Serialize, }; +use thiserror::Error; + use crate::db::error::{DbError, DbErrorKind}; use crate::web::error::{HawkError, ValidationError, ValidationErrorKind}; use crate::web::extractors::RequestErrorLocation; +use std::error::Error; /// Legacy Sync 1.1 error codes, which Sync 1.5 also returns by replacing the descriptive JSON /// information and replacing it with one of these error codes. @@ -53,27 +56,32 @@ pub const RETRY_AFTER: u8 = 10; /// Top-level error type. #[derive(Debug)] pub struct ApiError { - inner: Context, + kind: ApiErrorKind, + pub(crate) backtrace: Backtrace, status: StatusCode, } /// Top-level ErrorKind. -#[derive(Debug, Fail)] +#[derive(Error, Debug)] pub enum ApiErrorKind { - #[fail(display = "{}", _0)] - Db(#[cause] DbError), + // Note, `#[from]` applies some derivation to the target error, but can fail + // if the target has any complexity associated with it. It's best to add + // #[derive(thiserror::Error,...)] to the target error to ensure that various + // traits are defined. + #[error("{}", _0)] + Db(DbError), - #[fail(display = "HAWK authentication error: {}", _0)] - Hawk(#[cause] HawkError), + #[error("HAWK authentication error: {}", _0)] + Hawk(HawkError), - #[fail(display = "No app_data ServerState")] + #[error("No app_data ServerState")] NoServerState, - #[fail(display = "{}", _0)] + #[error("{}", _0)] Internal(String), - #[fail(display = "{}", _0)] - Validation(#[cause] ValidationError), + #[error("{}", _0)] + Validation(ValidationError), } impl ApiErrorKind { @@ -89,7 +97,7 @@ impl ApiErrorKind { impl ApiError { pub fn kind(&self) -> &ApiErrorKind { - self.inner.get_context() + &self.kind } pub fn is_collection_not_found(&self) -> bool { @@ -199,6 +207,11 @@ impl From> for ApiError { } } } +impl Error for ApiError { + fn source(&self) -> Option<&(dyn Error + 'static)> { + self.kind.source() + } +} impl From for HttpResponse { fn from(inner: ApiError) -> Self { @@ -218,9 +231,9 @@ impl From for ApiError { } } -impl From> for ApiError { - fn from(inner: Context) -> Self { - let status = match inner.get_context() { +impl From for ApiError { + fn from(kind: ApiErrorKind) -> Self { + let status = match &kind { ApiErrorKind::Db(error) => error.status, ApiErrorKind::Hawk(_) => StatusCode::UNAUTHORIZED, ApiErrorKind::NoServerState | ApiErrorKind::Internal(_) => { @@ -229,7 +242,11 @@ impl From> for ApiError { ApiErrorKind::Validation(error) => error.status, }; - Self { inner, status } + Self { + kind, + backtrace: Backtrace::new(), + status, + } } } @@ -265,7 +282,7 @@ impl Serialize for ApiError { map.serialize_entry("reason", self.status.canonical_reason().unwrap_or(""))?; if self.status != StatusCode::UNAUTHORIZED { - map.serialize_entry("errors", &self.inner.get_context())?; + map.serialize_entry("errors", &self.kind)?; } map.end() @@ -301,33 +318,17 @@ where seq.end() } -macro_rules! failure_boilerplate { +macro_rules! impl_fmt_display { ($error:ty, $kind:ty) => { - impl Fail for $error { - fn cause(&self) -> Option<&dyn Fail> { - self.inner.cause() - } - - fn backtrace(&self) -> Option<&Backtrace> { - self.inner.backtrace() - } - } - impl fmt::Display for $error { fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - fmt::Display::fmt(&self.inner, formatter) - } - } - - impl From<$kind> for $error { - fn from(kind: $kind) -> Self { - Context::new(kind).into() + fmt::Display::fmt(&self.kind, formatter) } } }; } -failure_boilerplate!(ApiError, ApiErrorKind); +impl_fmt_display!(ApiError, ApiErrorKind); macro_rules! from_error { ($from:ty, $to:ty, $to_kind:expr) => { diff --git a/src/web/error.rs b/src/web/error.rs index ab812418..4e71bd65 100644 --- a/src/web/error.rs +++ b/src/web/error.rs @@ -5,7 +5,7 @@ use std::fmt; use actix_web::http::{header::ToStrError, StatusCode}; use actix_web::Error as ActixError; use base64::DecodeError; -use failure::{Backtrace, Context, Fail}; + use hawk::Error as ParseError; use hmac::crypto_mac::{InvalidKeyLength, MacError}; use serde::{ @@ -17,15 +17,17 @@ use serde_json::{Error as JsonError, Value}; use super::extractors::RequestErrorLocation; use crate::error::ApiError; +use thiserror::Error; + /// An error occurred during HAWK authentication. #[derive(Debug)] pub struct HawkError { - inner: Context, + kind: HawkErrorKind, } impl HawkError { pub fn kind(&self) -> &HawkErrorKind { - self.inner.get_context() + &self.kind } pub fn is_reportable(&self) -> bool { @@ -52,58 +54,58 @@ impl HawkError { } /// Causes of HAWK errors. -#[derive(Debug, Fail)] +#[derive(Debug, Error)] pub enum HawkErrorKind { - #[fail(display = "{}", _0)] - Base64(#[cause] DecodeError), + #[error("{}", _0)] + Base64(DecodeError), - #[fail(display = "expired payload")] + #[error("expired payload")] Expired, - #[fail(display = "{}", _0)] - Header(#[cause] ToStrError), + #[error("{}", _0)] + Header(ToStrError), - #[fail(display = "{}", _0)] + #[error("{}", _0)] Hmac(MacError), - #[fail(display = "validation failed")] + #[error("validation failed")] InvalidHeader, - #[fail(display = "{}", _0)] + #[error("{}", _0)] InvalidKeyLength(InvalidKeyLength), - #[fail(display = "{}", _0)] - Json(#[cause] JsonError), + #[error("{}", _0)] + Json(JsonError), - #[fail(display = "missing header")] + #[error("missing header")] MissingHeader, - #[fail(display = "missing id property")] + #[error("missing id property")] MissingId, - #[fail(display = "missing path")] + #[error("missing path")] MissingPath, - #[fail(display = "missing \"Hawk \" prefix")] + #[error("missing \"Hawk \" prefix")] MissingPrefix, - #[fail(display = "{}", _0)] + #[error("{}", _0)] Parse(ParseError), - #[fail(display = "id property is too short")] + #[error("id property is too short")] TruncatedId, } /// An error occurred in an Actix extractor. -#[derive(Debug)] +#[derive(Error, Debug)] pub struct ValidationError { pub status: StatusCode, - inner: Context, + kind: ValidationErrorKind, } impl ValidationError { pub fn kind(&self) -> &ValidationErrorKind { - self.inner.get_context() + &self.kind } pub fn metric_label(&self) -> Option { @@ -123,21 +125,21 @@ impl ValidationError { } /// Causes of extractor errors. -#[derive(Debug, Fail)] +#[derive(Debug, Error)] pub enum ValidationErrorKind { - #[fail(display = "{}", _0)] + #[error("{}", _0)] FromDetails(String, RequestErrorLocation, Option, Option), - #[fail(display = "{}", _0)] + #[error("{}", _0)] FromValidationErrors( - #[cause] validator::ValidationErrors, + validator::ValidationErrors, RequestErrorLocation, Option, ), } -failure_boilerplate!(HawkError, HawkErrorKind); -failure_boilerplate!(ValidationError, ValidationErrorKind); +impl_fmt_display!(HawkError, HawkErrorKind); +impl_fmt_display!(ValidationError, ValidationErrorKind); from_error!(DecodeError, ApiError, HawkErrorKind::Base64); from_error!(InvalidKeyLength, ApiError, HawkErrorKind::InvalidKeyLength); @@ -145,16 +147,16 @@ from_error!(JsonError, ApiError, HawkErrorKind::Json); from_error!(MacError, ApiError, HawkErrorKind::Hmac); from_error!(ToStrError, ApiError, HawkErrorKind::Header); -impl From> for HawkError { - fn from(inner: Context) -> Self { - Self { inner } +impl From for HawkError { + fn from(kind: HawkErrorKind) -> Self { + Self { kind } } } -impl From> for ValidationError { - fn from(inner: Context) -> Self { - trace!("Validation Error: {:?}", inner.get_context()); - let status = match inner.get_context() { +impl From for ValidationError { + fn from(kind: ValidationErrorKind) -> Self { + trace!("Validation Error: {:?}", kind); + let status = match kind { ValidationErrorKind::FromDetails(ref _description, ref location, Some(ref name), _) if *location == RequestErrorLocation::Header => { @@ -173,13 +175,13 @@ impl From> for ValidationError { _ => StatusCode::BAD_REQUEST, }; - Self { status, inner } + Self { status, kind } } } impl From for ApiError { fn from(kind: HawkErrorKind) -> Self { - let hawk_error: HawkError = Context::new(kind).into(); + let hawk_error: HawkError = kind.into(); hawk_error.into() } } @@ -192,7 +194,7 @@ impl From for ApiError { impl From for ApiError { fn from(kind: ValidationErrorKind) -> Self { - let validation_error: ValidationError = Context::new(kind).into(); + let validation_error: ValidationError = kind.into(); validation_error.into() } } @@ -209,7 +211,7 @@ impl Serialize for ValidationError { where S: Serializer, { - Serialize::serialize(&self.inner.get_context(), serializer) + Serialize::serialize(&self.kind, serializer) } } diff --git a/src/web/middleware/sentry.rs b/src/web/middleware/sentry.rs index 3c85e7c3..0dfba167 100644 --- a/src/web/middleware/sentry.rs +++ b/src/web/middleware/sentry.rs @@ -1,3 +1,4 @@ +use std::error::Error as StdError; use std::task::Context; use std::{cell::RefCell, rc::Rc}; @@ -13,6 +14,7 @@ use std::task::Poll; use crate::error::ApiError; use crate::server::{metrics::Metrics, ServerState}; use crate::web::tags::Tags; +use sentry_backtrace::parse_stacktrace; pub struct SentryWrapper; @@ -142,7 +144,7 @@ where trace!("Sentry: Not reporting error: {:?}", apie); return future::ok(sresp); } - report(&tags, sentry::integrations::failure::event_from_fail(apie)); + report(&tags, event_from_error(apie)); } } } @@ -150,3 +152,51 @@ where })) } } + +/// Custom `sentry::event_from_error` for `ApiError` +/// +/// `sentry::event_from_error` can't access `std::Error` backtraces as its +/// `backtrace()` method is currently Rust nightly only. This function works +/// against `HandlerError` instead to access its backtrace. +pub fn event_from_error(err: &ApiError) -> Event<'static> { + let mut exceptions = vec![exception_from_error_with_backtrace(err)]; + + let mut source = err.source(); + while let Some(err) = source { + let exception = if let Some(err) = err.downcast_ref() { + exception_from_error_with_backtrace(err) + } else { + exception_from_error(err) + }; + exceptions.push(exception); + source = err.source(); + } + + exceptions.reverse(); + Event { + exception: exceptions.into(), + level: sentry::protocol::Level::Error, + ..Default::default() + } +} + +/// Custom `exception_from_error` support function for `ApiError` +/// +/// Based moreso on sentry_failure's `exception_from_single_fail`. +fn exception_from_error_with_backtrace(err: &ApiError) -> sentry::protocol::Exception { + let mut exception = exception_from_error(err); + // format the stack trace with alternate debug to get addresses + let bt = format!("{:#?}", err.backtrace); + exception.stacktrace = parse_stacktrace(&bt); + exception +} + +/// Exact copy of sentry's unfortunately private `exception_from_error` +fn exception_from_error(err: &E) -> sentry::protocol::Exception { + let dbg = format!("{:?}", err); + sentry::protocol::Exception { + ty: sentry::parse_type_from_debug(&dbg).to_owned(), + value: Some(err.to_string()), + ..Default::default() + } +}