Catch and gracefully handle panics in routes and catchers. #1491
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Update: I tried removing TL;DR:
The first issue is in the As a test, I manually added a requirement for pub trait Handler: Cloneable + Send + Sync + 'static {
/// Called by Rocket when a `Request` with its associated `Data` should be
/// handled by this handler.
///
/// The variant of `Outcome` returned by the returned `Future` determines
/// what Rocket does next. If the return value is a `Success(Response)`, the
/// wrapped `Response` is used to respond to the client. If the return value
/// is a `Failure(Status)`, the error catcher for `Status` is invoked to
/// generate a response. Otherwise, if the return value is `Forward(Data)`,
/// the next matching route is attempted. If there are no other matching
/// routes, the `404` error catcher is invoked.
fn handle<'r, 's: 'r, 't, 'x>(&'s self, request: &'r Request<'t>, data: Data) -> Pin<Box<dyn Future<Output = Outcome<'r>> + Send + UnwindSafe + 'x>> where 'r: 'x, 's: 'x, 't: 'x, Self: 'x;
}
impl<F: Clone + Sync + Send + 'static> Handler for F
where for<'x> F: Fn(&'x Request<'_>, Data) -> HandlerFuture<'x>
{
#[inline(always)]
fn handle<'r, 's: 'r, 't, 'x>(&'s self, req: &'r Request<'t>, data: Data) -> Pin<Box<dyn Future<Output = Outcome<'r>> + Send + UnwindSafe + 'x>> where 'r: 'x, 's: 'x, 't: 'x, Self: 'x {
Box::pin(self(req, data))
}
}
// A handler to use when one is needed temporarily. Don't use outside of Rocket!
#[doc(hidden)]
pub fn dummy<'r>(r: &'r Request<'_>, _: Data) -> HandlerFuture<'r> {
Box::pin(async move { Outcome::from(r, ()) })
}Which uncovered several non-unwind-safe fields:
All of these errors are resolved by adding Finally, note this from https://doc.rust-lang.org/std/panic/trait.UnwindSafe.html#what-is-unwindsafe:
All in all, I see several false positives and not much evidence suggesting there is a potential unwind-safety problem as far as Rocket's own data structures go. I skimmed a few of the usages I was a bit worried about (e.g. I think this is ready for a high-level review, and I'd love to hear some ideas for either dealing with the unwind safety issue or ignoring it at this time as it pertains to user-implemented handlers. |
|
I think this generally looks good to me. It's incredibly difficult to determine if As far as the actual implementation goes, can we dedup all of these diff --git a/core/lib/src/handler.rs b/core/lib/src/handler.rs
index afb42b34..9919c155 100644
--- a/core/lib/src/handler.rs
+++ b/core/lib/src/handler.rs
@@ -149,6 +149,13 @@ pub trait Handler: Cloneable + Send + Sync + 'static {
/// the next matching route is attempted. If there are no other matching
/// routes, the `404` error catcher is invoked.
async fn handle<'r, 's: 'r>(&'s self, request: &'r Request<'_>, data: Data) -> Outcome<'r>;
+
+ async fn _handle<'r, 's: 'r>(&'s self, r: &'r Request<'_>, d: Data) -> Option<Outcome<'r>> {
+ use std::panic::AssertUnwindSafe;
+ use futures::future::FutureExt;
+
+ AssertUnwindSafe(self.handle(r, d)).catch_unwind().await.ok()
+ }
}
#[crate::async_trait]This results in a double-box (once in |
|
This seems like it should be doable with a free function as well, without the additional box: async fn handle_catching_unwind<'..., H: Handler>(handler: &H, req: &Request<'_>, data: Data) -> Option<Outcome<'...>> {
AssertUnwindSafe(handler.handle(req, data)).catch_unwind().await.ok()
}I should have time to try both options some time this weekend. |
jebrosen
force-pushed
the
catch-unwind
branch
from
5e5c9ca
to
4252fcd
Compare
|
Update: one call is to |
|
Can we get this rebased on master and polished for merging? In particular, we need a test to ensure this is working as expected. |
|
Rebased, and a test added! I ought to briefly explain the motivation for using
Downsides of
|
15 tasks
The only mechanism that I can imagine would allow a consumer of Rocket to break unwind safety is through request-local state. This, in turn, will likely be visible only in fairings and error catchers. Am I missing some other clear cases? I think this is largely a documentation issue. In general, I think we should state, repeat, and repeat again that handlers should not panic. I see far too much Rocket code written with unwraps and expects when Rocket makes it trivial to return an |
|
If panicking in handlers is discouraged, maybe Rocket could e.g. abort the whole server on panic when running with |
|
Merged in a0784b4 with some changes in a0784b4 related to this. @MikailBag I don't think we want to kill the server midway. But, here's my compromise: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, panics are left uncaught and will be caught at the next
catch_unwind(if any). Today this is intokio, and a panic in a route looks like this:This PR adds a
catch_unwindaround calls toHandler::handleandErrorHandler::handleand handles errors as 500s.Outcome::Failure(Status::InternalServerError)InternalServerErrorTODO:
catch_unwindsitesAssertUnwindSafeis placed. I put it directly at thecatch_unwindsite mostly out of convenience for testing if the approach would even work.