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 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() {
defer func() { defer func() {
if err := recover(); err != nil { if err := recover(); err != nil {
w.WriteHeader(500)
log.Printf("error: panic while recovering from another panic: %v\n", err) log.Printf("error: panic while recovering from another panic: %v\n", err)
debug.PrintStack() debug.PrintStack()
fmt.Fprintf(w, "internal server error: %v\n", err) 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 { 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) log.Printf("error: %v\n", err)
debug.PrintStack() debug.PrintStack()
rl.renderInternalServerError(w, r, err) rl.renderInternalServerError(w, r, err)
@ -80,10 +82,13 @@ func (w *statusResponseWriter) WriteHeader(statusCode int) {
} }
// CreateMainRouter creates the main Gorilla router for the application. // 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 { func CreateMainRouter(rl *rushlink) *mux.Router {
router := mux.NewRouter() router := mux.NewRouter()
router.Use(rl.recoveryMiddleware)
router.Use(rl.metricsMiddleware)
router.HandleFunc("/{path:img/"+staticFilenameExpr+"}", rl.staticGetHandler).Methods("GET", "HEAD") router.HandleFunc("/{path:img/"+staticFilenameExpr+"}", rl.staticGetHandler).Methods("GET", "HEAD")
router.HandleFunc("/{path:css/"+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") 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, rootURL: rootURL,
} }
router := mux.NewRouter()
router.Use(rl.metricsMiddleware)
router.Use(rl.recoveryMiddleware)
router.Handle("/", CreateMainRouter(&rl))
srv := &http.Server{ srv := &http.Server{
Handler: CreateMainRouter(&rl), Handler: router,
Addr: addr, Addr: addr,
WriteTimeout: 15 * time.Second, WriteTimeout: 15 * time.Second,
ReadTimeout: 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) { func (rl *rushlink) renderStatic(w http.ResponseWriter, r *http.Request, path string) {
var modTime time.Time var modTime time.Time
if info, err := AssetInfo(path); err != nil { if info, err := AssetInfo(path); err == nil {
modTime = info.ModTime() modTime = info.ModTime()
} }
contents, err := Asset(path) 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) err = fmt.Errorf("'%v' not in textTemplates", tmplName)
break break
} }
if status != 0 {
w.WriteHeader(status)
}
if r.Method != "HEAD" {
err = tmpl.Execute(w, data) err = tmpl.Execute(w, data)
}
case "text/html": case "text/html":
w.Header().Set("Content-Type", "text/html") w.Header().Set("Content-Type", "text/html")
tmpl := htmlTemplates[tmplName] tmpl := htmlTemplates[tmplName]
@ -142,7 +147,6 @@ func (rl *rushlink) render(w http.ResponseWriter, r *http.Request, status int, t
if status != 0 { if status != 0 {
w.WriteHeader(status) w.WriteHeader(status)
} }
if r.Method != "HEAD" { if r.Method != "HEAD" {
err = tmpl.Execute(w, data) err = tmpl.Execute(w, data)
} }