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 +22 -6
2 changed files with 22 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 0b2297a2e8 - Show all commits

16
handlers_test.go
View File

@ -57,7 +57,7 @@ func createTemporaryRouter(t *testing.T) (*mux.Router, *rushlink) {
fs: fileStore,
rootURL: rootURL,
}
return CreateMainRouter(&rl), &rl
return CreateMainRouter(&rl, true), &rl
mrngm commented 2020-06-28 13:37:13 +02:00
Outdated
Review
Copy Link

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

(see comment below on the use of `debug` as a function parameter)
}
// checkStatusCode checks whether the status code from a recorded response is equal
@ -145,5 +145,17 @@ func TestIssue53(t *testing.T) {
}
return nil
})
}
func TestIssue60(t *testing.T) {
srv, _ := createTemporaryRouter(t)
// Put a URL with a fragment identifier into the database.
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)
}

12
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,12 @@ func (w *statusResponseWriter) WriteHeader(statusCode int) {
}
// CreateMainRouter creates the main Gorilla router for the application.
func CreateMainRouter(rl *rushlink) *mux.Router {
func CreateMainRouter(rl *rushlink, debug bool) *mux.Router {
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?
router := mux.NewRouter()
router.Use(rl.recoveryMiddleware)
router.Use(rl.metricsMiddleware)
if !debug {
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")
@ -119,7 +123,7 @@ func StartMainServer(addr string, db *db.Database, fs *db.FileStore, rawRootURL
}
srv := &http.Server{
Handler: CreateMainRouter(&rl),
Handler: CreateMainRouter(&rl, false),
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)
Addr: addr,
WriteTimeout: 15 * time.Second,
ReadTimeout: 15 * time.Second,