[trafficcontrol] branch master updated: Return a 500 response rather than an empty 200 response on panic recovery

[ro...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

rob pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficcontrol.git


The following commit(s) were added to refs/heads/master by this push:
     new a1f6b43  Return a 500 response rather than an empty 200 response on panic recovery
a1f6b43 is described below

commit a1f6b43352a0b86e9dba0e7e5c301136260dd838
Author: Rawlin Peters <ra...@comcast.com>
AuthorDate: Thu Jan 17 13:38:31 2019 -0700

    Return a 500 response rather than an empty 200 response on panic recovery
    
    It doesn't really make sense to return an empty 200 response if the
    server recovers from a panic and most likely didn't fulfill the request
    successfully.
---
 traffic_ops/traffic_ops_golang/wrappers.go      |  3 ++-
 traffic_ops/traffic_ops_golang/wrappers_test.go | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/traffic_ops/traffic_ops_golang/wrappers.go b/traffic_ops/traffic_ops_golang/wrappers.go
index ef68b51..980ccfb 100644
--- a/traffic_ops/traffic_ops_golang/wrappers.go
+++ b/traffic_ops/traffic_ops_golang/wrappers.go
@@ -100,7 +100,8 @@ func wrapPanicRecover(h http.HandlerFunc) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		defer func() {
 			if err := recover(); err != nil {
-				log.Errorf("panic: (err: %v) stacktrace:\n%s\n", err, stacktrace())
+				api.HandleErr(w, r, nil, http.StatusInternalServerError, nil, fmt.Errorf("panic: (err: %v) stacktrace:\n%s\n", err, stacktrace()))
+				return
 			}
 		}()
 		h(w, r)
diff --git a/traffic_ops/traffic_ops_golang/wrappers_test.go b/traffic_ops/traffic_ops_golang/wrappers_test.go
index 710bb80..d0f7cef 100644
--- a/traffic_ops/traffic_ops_golang/wrappers_test.go
+++ b/traffic_ops/traffic_ops_golang/wrappers_test.go
@@ -83,6 +83,29 @@ func TestWrapHeaders(t *testing.T) {
 	}
 }
 
+// TestWrapPanicRecover checks that a recovered panic returns a 500
+func TestWrapPanicRecover(t *testing.T) {
+	f := wrapPanicRecover(func(w http.ResponseWriter, r *http.Request) {
+		var foo *string
+		bar := *foo // will throw nil dereference panic
+		w.Write([]byte(bar))
+	})
+	f = wrapHeaders(f)
+
+	w := httptest.NewRecorder()
+	r, err := http.NewRequest("", "/", nil)
+	if err != nil {
+		t.Error("Error creating new request")
+	}
+
+	// Call to wrap the panic recovery
+	f(w, r)
+
+	if w.Code != http.StatusInternalServerError {
+		t.Error("expected panic recovery to return a 500, got", w.Code)
+	}
+}
+
 // TestGzip checks that if Accept-Encoding contains "gzip" that the body is indeed gzip'd
 func TestGzip(t *testing.T) {
 	body := "am I gzip'd?"