Fix error handling in renderStatic #64

Merged
electricdusk merged 2 commits from issue-60 into master 2020-07-06 17:05:00 +02:00
3 changed files with 33 additions and 7 deletions

View File

@ -145,5 +145,17 @@ func TestIssue53(t *testing.T) {
}
return nil
})
}
func TestIssue60(t *testing.T) {
srv, _ := createTemporaryRouter(t)
// Request a nonexistent static file
req, err := http.NewRequest("GET", "/css/nonexistent_file.css", nil)
if err != nil {
t.Fatal(err)
}
rr := httptest.NewRecorder()
srv.ServeHTTP(rr, req)
checkStatusCode(t, rr, http.StatusNotFound)
}

View File

@ -33,6 +33,7 @@ func (rl *rushlink) recoveryMiddleware(next http.Handler) http.Handler {
defer func() {
defer func() {
if err := recover(); err != nil {
w.WriteHeader(500)
log.Printf("error: panic while recovering from another panic: %v\n", err)
debug.PrintStack()
fmt.Fprintf(w, "internal server error: %v\n", err)
@ -40,6 +41,7 @@ func (rl *rushlink) recoveryMiddleware(next http.Handler) http.Handler {
}()
if err := recover(); err != nil {
w.WriteHeader(500)
Review

Can we definitively state that panic()s are caused due to errors on the server side?

Can we definitively state that `panic()`s are caused due to errors on the server side?
Review

Yes. panics during runtime should always indicate a bug.

Yes. `panic`s during runtime should always indicate a bug.
log.Printf("error: %v\n", err)
debug.PrintStack()
rl.renderInternalServerError(w, r, err)
@ -80,10 +82,13 @@ func (w *statusResponseWriter) WriteHeader(statusCode int) {
}
// CreateMainRouter creates the main Gorilla router for the application.
//
// This function will not populate the router with an error-recovery and
// metrics-reporting middleware. If these middleware are required, then the
// caller should encapsulate this router inside of another router and register
// the middlewares on the encapsulating router.
func CreateMainRouter(rl *rushlink) *mux.Router {
router := mux.NewRouter()
router.Use(rl.recoveryMiddleware)
router.Use(rl.metricsMiddleware)
router.HandleFunc("/{path:img/"+staticFilenameExpr+"}", rl.staticGetHandler).Methods("GET", "HEAD")
router.HandleFunc("/{path:css/"+staticFilenameExpr+"}", rl.staticGetHandler).Methods("GET", "HEAD")
router.HandleFunc("/{path:js/"+staticFilenameExpr+"}", rl.staticGetHandler).Methods("GET", "HEAD")
@ -118,8 +123,13 @@ func StartMainServer(addr string, db *db.Database, fs *db.FileStore, rawRootURL
rootURL: rootURL,
}
router := mux.NewRouter()
router.Use(rl.metricsMiddleware)
router.Use(rl.recoveryMiddleware)
router.Handle("/", CreateMainRouter(&rl))
srv := &http.Server{
Handler: CreateMainRouter(&rl),
Handler: router,
Addr: addr,
WriteTimeout: 15 * time.Second,
ReadTimeout: 15 * time.Second,

View File

@ -88,7 +88,7 @@ func mapExtend(m map[string]interface{}, key string, value interface{}) {
func (rl *rushlink) renderStatic(w http.ResponseWriter, r *http.Request, path string) {
var modTime time.Time
if info, err := AssetInfo(path); err != nil {
if info, err := AssetInfo(path); err == nil {
modTime = info.ModTime()
}
contents, err := Asset(path)
@ -118,7 +118,12 @@ func (rl *rushlink) render(w http.ResponseWriter, r *http.Request, status int, t
err = fmt.Errorf("'%v' not in textTemplates", tmplName)
break
}
err = tmpl.Execute(w, data)
if status != 0 {
w.WriteHeader(status)
}
if r.Method != "HEAD" {
err = tmpl.Execute(w, data)
}
case "text/html":
w.Header().Set("Content-Type", "text/html")
tmpl := htmlTemplates[tmplName]
@ -142,7 +147,6 @@ func (rl *rushlink) render(w http.ResponseWriter, r *http.Request, status int, t
if status != 0 {
w.WriteHeader(status)
}
if r.Method != "HEAD" {
err = tmpl.Execute(w, data)
}