electricdusk/rushlink
3
3
Fork 3
Code Issues 21 Pull Requests 2 Releases Activity

Fix error handling in renderStatic #64

Merged
electricdusk merged 2 commits from issue-60 into master 2020-07-06 17:05:00 +02:00
Conversation 0 Commits 2 Files Changed 3 +33 -7
3 changed files with 33 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
handlers_test.go
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)
}

16
router.go
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)
mrngm commented 2020-06-28 13:36:42 +02:00
Review
Copy Link

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?
electricdusk commented 2020-06-28 14:39:45 +02:00
Review
Copy Link

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.
//
mrngm commented 2020-06-28 13:35:20 +02:00
Outdated
Review
Copy Link

Instead of adding a debug parameter here, would it make sense to set router.Use(rl.recoveryMiddleware) inside TestIssue60?

func TestIssue60(t *testing.T) {
	srv, rl := createTemporaryRouter(t)
	srv.Use(rl.recoveryMiddleware)
	srv.Use(rl.metricsMiddleware)

	// 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)

And, if we want debugging options, this should be a globally configurable option, instead of a function parameter.

Instead of adding a debug parameter here, would it make sense to set `router.Use(rl.recoveryMiddleware)` inside `TestIssue60`? ```go func TestIssue60(t *testing.T) { srv, rl := createTemporaryRouter(t) srv.Use(rl.recoveryMiddleware) srv.Use(rl.metricsMiddleware) // 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) ``` And, if we want debugging options, this should be a globally configurable option, instead of a function parameter.
electricdusk commented 2020-06-28 14:42:09 +02:00
Outdated
Review
Copy Link

So you mean, as a global variable? So in this case, debug describes that the panic recovery middleware should be disabled, because otherwise we cannot run the tests. We could also change the name debug to noPanicMiddleware.

What kind of solution would be good here?

So you mean, as a global variable? So in this case, `debug` describes that the panic recovery middleware should be disabled, because otherwise we cannot run the tests. We could also change the name `debug` to `noPanicMiddleware`. What kind of solution would be good here?
// 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()
mrngm commented 2020-06-28 13:37:40 +02:00
Outdated
Review
Copy Link

(see comment above on the use of debug as a function parameter)

(see comment above on the use of `debug` as a function parameter)
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,

10
views.go
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)
}