Skip to content

Commit

Permalink
server error should show a message
Browse files Browse the repository at this point in the history
  • Loading branch information
amitu committed Nov 7, 2023
1 parent 4af75a8 commit 3ce144d
Show file tree
Hide file tree
Showing 7 changed files with 161 additions and 105 deletions.
103 changes: 2 additions & 101 deletions Cargo.lock

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

2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ format_num = "0.1"
ftd = { path = "ftd" }
fastn-js = { path = "fastn-js" }
futures = "0.3"
futures-util = { version = "0.3", default-features = false, features = ["std"] }
futures-core = "0.3"
home = "0.5"
ignore = "0.4"
include_dir = "0.7"
Expand Down
3 changes: 2 additions & 1 deletion fastn-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ github-auth = ["dep:oauth2"]

[dependencies]
actix-web.workspace = true
actix-web-lab.workspace = true
antidote.workspace = true
async-lock.workspace = true
dirs.workspace = true
Expand All @@ -41,6 +40,8 @@ clap.workspace = true
colored.workspace = true
native-tls.workspace = true
deadpool-postgres.workspace = true
futures-util.workspace = true
futures-core.workspace = true
postgres-types.workspace = true
postgres-native-tls.workspace = true
tokio-postgres.workspace = true
Expand Down
151 changes: 151 additions & 0 deletions fastn-core/src/catch_panic.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
// borrowed from https://github.com/robjtede/actix-web-lab/ (MIT)
use std::{
future::{ready, Ready},
panic::AssertUnwindSafe,
rc::Rc,
};

use actix_web::{
dev::{forward_ready, Service, ServiceRequest, ServiceResponse, Transform},
error,
};
use futures_core::future::LocalBoxFuture;
use futures_util::FutureExt as _;

/// A middleware to catch panics in wrapped handlers and middleware, returning empty 500 responses.
///
/// **This middleware should never be used as replacement for proper error handling.** See [this
/// thread](https://github.com/actix/actix-web/issues/1501#issuecomment-627517783) for historical
/// discussion on why Actix Web does not do this by default.
///
/// It is recommended that this middleware be registered last. That is, `wrap`ed after everything
/// else except `Logger`.
///
/// # Examples
///
/// ```
/// # use actix_web::App;
/// use actix_web_lab::middleware::CatchPanic;
///
/// App::new().wrap(CatchPanic::default())
/// # ;
/// ```
///
/// ```no_run
/// # use actix_web::App;
/// use actix_web::middleware::{Logger, NormalizePath};
/// use actix_web_lab::middleware::CatchPanic;
///
/// // recommended wrap order
/// App::new()
/// .wrap(NormalizePath::default())
/// .wrap(CatchPanic::default()) // <- after everything except logger
/// .wrap(Logger::default())
/// # ;
/// ```
#[derive(Debug, Clone, Default)]
#[non_exhaustive]
pub struct CatchPanic;

impl<S, B> Transform<S, ServiceRequest> for CatchPanic
where
S: Service<ServiceRequest, Response = ServiceResponse<B>, Error = actix_web::Error> + 'static,
{
type Response = ServiceResponse<B>;
type Error = actix_web::Error;
type Transform = CatchPanicMiddleware<S>;
type InitError = ();
type Future = Ready<Result<Self::Transform, Self::InitError>>;

fn new_transform(&self, service: S) -> Self::Future {
ready(Ok(CatchPanicMiddleware {
service: Rc::new(service),
}))
}
}

pub struct CatchPanicMiddleware<S> {
service: Rc<S>,
}

impl<S, B> Service<ServiceRequest> for CatchPanicMiddleware<S>
where
S: Service<ServiceRequest, Response = ServiceResponse<B>, Error = actix_web::Error> + 'static,
{
type Response = ServiceResponse<B>;
type Error = actix_web::Error;
type Future = LocalBoxFuture<'static, Result<Self::Response, Self::Error>>;

forward_ready!(service);

fn call(&self, req: ServiceRequest) -> Self::Future {
AssertUnwindSafe(self.service.call(req))
.catch_unwind()
.map(move |res| match res {
Ok(Ok(res)) => Ok(res),
Ok(Err(svc_err)) => Err(svc_err),
Err(_panic_err) => Err(error::ErrorInternalServerError("500 Server Error")),
})
.boxed_local()
}
}

#[cfg(test)]
mod tests {
use actix_web::{
body::{to_bytes, MessageBody},
dev::{Service as _, ServiceFactory},
http::StatusCode,
test, web, App, Error,
};

use super::*;

fn test_app() -> App<
impl ServiceFactory<
ServiceRequest,
Response = ServiceResponse<impl MessageBody>,
Config = (),
InitError = (),
Error = Error,
>,
> {
App::new()
.wrap(CatchPanic::default())
.route("/", web::get().to(|| async { "content" }))
.route(
"/disco",
#[allow(unreachable_code)]
web::get().to(|| async {
panic!("the disco");
""
}),
)
}

#[actix_web::test]
async fn pass_through_no_panic() {
let app = test::init_service(test_app()).await;

let req = test::TestRequest::default().to_request();
let res = test::call_service(&app, req).await;
assert_eq!(res.status(), StatusCode::OK);
let body = test::read_body(res).await;
assert_eq!(body, "content");
}

#[actix_web::test]
async fn catch_panic_return_internal_server_error_response() {
let app = test::init_service(test_app()).await;

let req = test::TestRequest::with_uri("/disco").to_request();
let err = match app.call(req).await {
Ok(_) => panic!("unexpected Ok response"),
Err(err) => err,
};
let res = err.error_response();
assert_eq!(res.status(), StatusCode::INTERNAL_SERVER_ERROR);
let body = to_bytes(res.into_body()).await.unwrap();
assert!(body.is_empty());
}
}
2 changes: 1 addition & 1 deletion fastn-core/src/commands/serve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -728,7 +728,7 @@ You can try without providing port, it will automatically pick unused port."#,
inline_css: inline_css.clone(),
package_name: package_name.clone(),
}))
.wrap(actix_web_lab::middleware::CatchPanic::default())
.wrap(fastn_core::catch_panic::CatchPanic::default())
.wrap(
actix_web::middleware::Logger::new(
r#""%r" %Ts %s %b %a "%{Referer}i" "%{User-Agent}i""#,
Expand Down
1 change: 1 addition & 0 deletions fastn-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ mod tracker;
mod translation;
mod version;
// mod wasm;
pub(crate) mod catch_panic;
mod library2022;
mod workspace;

Expand Down
4 changes: 2 additions & 2 deletions fastn-core/src/tutor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ pub async fn process(
};

let state = TutorState {
fs_state,
done: fs_state.done,
current: CURRENT_TUTORIAL.read().await.as_ref().map(|t| t.id.clone()),
};

Expand All @@ -110,7 +110,7 @@ pub async fn process(

#[derive(Debug, Default, serde::Serialize)]
struct TutorState {
fs_state: TutorStateFS,
done: Vec<String>,
current: Option<String>,
}

Expand Down

0 comments on commit 3ce144d

Please sign in to comment.