panic handling middleware

[kosta]

Feature Request

Motivation

From web application frameworks in other languages, I would expect unhandled panics to be converted to http status 500 Internal Server errors. In axum, the connection is simply terminated, which is unexpected for me. I have found no mention of how to handle panics in documentation of axum, tower and hyper (but of course, it might be written somewhere I didnt see it).

Proposal

Implement a panic handler middleware or document how to implement one (with obligatory link to "you cannot catch all panics").

Can you comment on this (naive?) code? (I have zero experience with handling panics in rust and must say that I do not fully understand UnwindSafe/AssertUnwindSafe)

// can be used as .layer(middleware::from_fn(dont_panic))
async fn dont_panic<B>(req: Request<B>, next: Next<B>) -> impl IntoResponse {
    AssertUnwindSafe(next.run(req))
        .catch_unwind()
        .await
        .map_err(|payload| {
            let message = extract_panic_message(payload);
            error!("Caught panic: {:?}", message);
            StatusCode::INTERNAL_SERVER_ERROR
        })
}

fn extract_panic_message(payload: Box<(dyn Any + Send + 'static)>) -> Option<String> {
    payload
        .downcast_ref::<&str>()
        .cloned()
        .map(String::from)
        .or_else(|| payload.downcast_ref::<String>().cloned())
}

Alternatives

Document why handling panics is not a good idea.

[davidpdrsn]

This feels like something we should have in tower-http.

[davidpdrsn]

I've made a draft PR for tower-http tower-rs/tower-http#214

[BenJeau]

I just switched from actix-web and thought this was built in and just encountered this (or something similar) where a database call (within docker and using diesel) errored out and caused a stack overflow.

Is this the current behaviour of axum when panicking or is this something else?

Here's the related logs:

rest_server        | 2022-01-26T23:29:42.951243Z  INFO postgres_broker::core::drawing: Created drawing 3f65557c-559e-4dc7-92f3-ba4526c469a0
pg                 | 2022-01-26 23:29:42.975 UTC [149] ERROR:  value too long for type character varying(6)
pg                 | 2022-01-26 23:29:42.975 UTC [149] STATEMENT:  INSERT INTO "drawing_strokes" ("drawing_id", "color", "thickness", "opacity") VALUES ($1, $2, $3, $4) RETURNING "drawing_strokes"."id"
rest_server        |
rest_server        | thread 'tokio-runtime-worker' has overflowed its stack
rest_server        | fatal runtime error: stack overflow
rest_server exited with code 133

[davidpdrsn]

Can you catch stack overflows?

[kosta]

Can you catch stack overflows?

As far as I know, stack overflows abort on rust, they do not panic. So they cannot be caught.

Source:

• Great stack overflow error messages rust-lang/rust#51405

If you want to have a stack trace of what went wrong, maybe try this for debugging the issue(from the rust issue above)? https://docs.rs/backtrace-on-stack-overflow/latest/backtrace_on_stack_overflow/fn.enable.html

[kosta]

This feels like something we should have in tower-http.
I've made a draft PR for tower-http tower-rs/tower-http#214

I agree and thank you for implementing it right away!

Do think it's sensible to document how to "roll your own" (like my code snippet above) in axum? Just to show how easy it is (compared to tower-http) and so that people with different needs can modify it as needed? Or is that too much of an edge case?

[davidpdrsn]

What do you imagine users would wanna customize?

I personally don't think its necessary to document, given that we something for it in tower-http. Using panics for error handling is already a bit of anti-pattern imo.

[kosta]

For logging/handling: I everything should be possible by the (optional) closure.

The only other thing I can think of is "extract something else from the panic payload" but I don't think that's something we should support/encourage/mention.

So I think once tower-rs/tower-http#214 is resolved, the only thing to do here is mention it in the error-handling docs.

[davidpdrsn]

For logging/handling: I everything should be possible by the (optional) closure.

Yeah thats also what I'm thinking

The only other thing I can think of is "extract something else from the panic payload" but I don't think that's something we should support/encourage/mention.

I imagine the custom closure/callback would get the Box<dyn Any + Send> that we get from catch_unwind as that gives users the most amount of flexibility. I also wanna document downcasting that to a &str/String as users would probably want that for constructing a JSON response.

[davidpdrsn]

I'll close this since there is nothing to do in axum. Its being implemented in tower-http here.