Add CatchPanic
#214
Add CatchPanic
#214
Conversation
|
|
||
| #[inline] | ||
| fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> { | ||
| self.inner.poll_ready(cx) |
There was a problem hiding this comment.
Not sure, but should this also catch panics? 🤔
There was a problem hiding this comment.
Hm so thats a good question actually. I'm actually thinking no, panics from poll_ready shouldn't be handled for a few reasons:
- We cannot return a response from
poll_readyso we'd have to return an error. That would require changing the error type toBoxErrorwhich means a service that previously returnedInfalliblewould now returnBoxError. This makes it more annoying to use with axum. - If
poll_readyreturns an error theServicecontract dictates that the service must be discarded and recreated. Hyper handles that and does the same thing regardless ifpoll_readyerrors or panics. So since we cannot return a response it kinda doesn't matter what we do 🤷 - In practice its rare for services to actually care about
poll_readyso panics from here should be very rare. I would guess they're much more common fromcall.
@hawkw What do you think?
There was a problem hiding this comment.
Yeah, I don't think we should catch panics here. Those reasons track for me.
|
Hmm, I kind of wonder if it makes sense to have a general-purpose |
|
@hawkw yep that's definitely something I want to explore as well before merging this! I would like tower-http to not have a public dependency on tower (just service and layer) but I'll see what I can come up with 😊 |
I've opted not to add anything to tower since I guess such a middleware would convert panics to errors. So to make this middleware easier to use with axum I'd prefer keeping the error type and instead changing the response. @hawkw let me know if you had something else in mind! |
|
Note to self: Read actix/actix-web#1501 |
|
I think we should make a decision about this soon. From talking to people the main criticism I hear is "don't use panics for error handling", which I agree with, but sadly you sometimes have dependencies that behave badly and have to guard against that. I'll add a note about that to the docs. It sounds to me like there isn't much reason not to provide a middleware like this. Thoughts @LucioFranco @olix0r @hawkw? |
| ResBody: Body<Data = Bytes> + Send + 'static, | ||
| ResBody::Error: Into<BoxError>, | ||
| T: ResponseForPanic + Clone, | ||
| T::ResponseBody: Body<Data = Bytes> + Send + 'static, | ||
| <T::ResponseBody as Body>::Error: Into<BoxError>, |
There was a problem hiding this comment.
Do we actually need to include these send bounds?
There was a problem hiding this comment.
Yes. They're required because we call http_body::Body::boxed_unsync which requires the body to be Send. We call that on the body from inner service and from the callback that creates the panic response.
* Release 0.2.4 - Added `CatchPanic` middleware which catches panics and converts them into `500 Internal Server` responses ([#214]) [#214]: #214 * Fix parsing of `Accept-Encoding` request header (#220) * Fix parsing of Accept-Encoding request header * Add unit tests to content_encoding * Represent quality values (qvalues) by a separate type * Parse encodings case-insensitively * Parse qvalues as specified in RFC 7231 section 5.3.1 Refs: #215 * Do not use or-pattern syntax This syntax is not supported in rust 1.51 (the minimum toolchain version). * Add comments to QValue::parse * Remove redundant SupportedEncodingsAll::new function * Add unit tests for all content-encodings (gzip, deflate, br) * Update changelog * add changelog groups Co-authored-by: Martin Dickopp <martin@zero-based.org>
This adds a
CatchPanicmiddleware that catches panics and converts them into500responses.TODO