Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only chain on 404 errors, log and return others as 500 #802

Merged
merged 2 commits into from
Jun 3, 2020
Merged

Only chain on 404 errors, log and return others as 500 #802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
287f291
Only chain to different handlers for 404 errors
Nemo157 Jun 2, 2020
75fba10
Log and return 500 for other errors
Nemo157 Jun 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/web/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ pub enum Nope {
ResourceNotFound,
CrateNotFound,
NoResults,
InternalServerError,
}

impl fmt::Display for Nope {
Expand All @@ -18,6 +19,7 @@ impl fmt::Display for Nope {
Nope::ResourceNotFound => "Requested resource not found",
Nope::CrateNotFound => "Requested crate not found",
Nope::NoResults => "Search yielded no results",
Nope::InternalServerError => "Internal server error",
})
}
}
Expand Down Expand Up @@ -59,6 +61,13 @@ impl Handler for Nope {
.to_resp("releases")
}
}
Nope::InternalServerError => {
// something went wrong, details should have been logged
Page::new("internal server error".to_owned())
.set_status(status::InternalServerError)
.title("Internal server error")
.to_resp("error")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to link to the GitHub page so people can make an issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a good followup issue to do once the template changes are in, all the error pages could do with a bit of a sprucing up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think asking to open an issue on GitHub this way is much beneficial for us: without a backtrace it's hard to then debug those issues.

A thing we could do is generate a random UUID, print it in the same line as the error and show it in the error page, so when people open the issue we can simply do a journalctl -u docs.rs | grep UUID to get the exact error the user encountered.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UUID sounds useful but it can go in a follow-up, for now I want to get the bug fix in.

}
}
}
}
40 changes: 28 additions & 12 deletions src/web/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,26 +138,42 @@ impl CratesfyiHandler {

impl Handler for CratesfyiHandler {
fn handle(&self, req: &mut Request) -> IronResult<Response> {
fn if_404(
e: IronError,
handle: impl FnOnce() -> IronResult<Response>,
) -> IronResult<Response> {
if e.response.status == Some(status::NotFound) {
handle()
} else {
Err(e)
}
};

// try serving shared rustdoc resources first, then router, then db/static file handler
// return 404 if none of them return Ok
self.shared_resource_handler
.handle(req)
.or_else(|e| self.router_handler.handle(req).or(Err(e)))
.or_else(|e| {
// if router fails try to serve files from database first
self.database_file_handler.handle(req).or(Err(e))
})
.or_else(|e| {
// and then try static handler. if all of them fails, return 404
self.static_handler.handle(req).or(Err(e))
})
.or_else(|e| if_404(e, || self.router_handler.handle(req)))
.or_else(|e| if_404(e, || self.database_file_handler.handle(req)))
.or_else(|e| if_404(e, || self.static_handler.handle(req)))
.or_else(|e| {
let err = if let Some(err) = e.error.downcast::<error::Nope>() {
*err
} else if e.error.downcast::<NoRoute>().is_some() {
} else if e.error.downcast::<NoRoute>().is_some()
|| e.response.status == Some(status::NotFound)
{
error::Nope::ResourceNotFound
} else if e.response.status == Some(status::InternalServerError) {
log::error!("internal server error: {}", e.error);
error::Nope::InternalServerError
} else {
panic!("all cratesfyi errors should be of type Nope");
log::error!(
"No error page for status {:?}; {}",
e.response.status,
e.error
);
// TODO: add in support for other errors that are actually used
error::Nope::InternalServerError
};

if let error::Nope::ResourceNotFound = err {
Expand All @@ -181,7 +197,7 @@ impl Handler for CratesfyiHandler {
}
}

debug!("Path not found: {}", DebugPath(&req.url));
debug!("Path not found: {}; {}", DebugPath(&req.url), e.error);
}

Self::chain(self.pool.clone(), err).handle(req)
Expand Down