Merge pull request #100 from matrix-org/kegan/service-isolation

Prevent panicking code from taking down the entire process

src/github.com/matrix-org/go-neb/goneb.go

@@ -193,9 +193,9 @@ func main() {
 	http.Handle("/metrics", prometheus.Handler())
 	http.Handle("/test", prometheus.InstrumentHandler("test", server.MakeJSONAPI(&heartbeatHandler{})))
 	wh := &webhookHandler{db: db, clients: clients}
-	http.HandleFunc("/services/hooks/", prometheus.InstrumentHandlerFunc("webhookHandler", wh.handle))
+	http.HandleFunc("/services/hooks/", prometheus.InstrumentHandlerFunc("webhookHandler", server.Protect(wh.handle)))
 	rh := &realmRedirectHandler{db: db}
-	http.HandleFunc("/realms/redirects/", prometheus.InstrumentHandlerFunc("realmRedirectHandler", rh.handle))
+	http.HandleFunc("/realms/redirects/", prometheus.InstrumentHandlerFunc("realmRedirectHandler", server.Protect(rh.handle)))
 
 	// Read exclusively from the config file if one was supplied.
 	// Otherwise, add HTTP listeners for new Services/Sessions/Clients/etc.

src/github.com/matrix-org/go-neb/matrix/worker.go

@@ -1,5 +1,10 @@
 package matrix
 
+import (
+	log "github.com/Sirupsen/logrus"
+	"runtime/debug"
+)
+
 // Worker processes incoming events and updates the Matrix client's data structures. It also informs
 // any attached listeners of the new events.
 type Worker struct {
@@ -38,6 +43,21 @@ func (worker *Worker) notifyListeners(event *Event) {
 }
 
 func (worker *Worker) onSyncHTTPResponse(res syncHTTPResponse) {
+	defer func() {
+		if r := recover(); r != nil {
+			userID := ""
+			if worker.client != nil {
+				userID = worker.client.UserID
+			}
+			log.WithFields(log.Fields{
+				"panic":   r,
+				"user_id": userID,
+			}).Errorf(
+				"onSyncHTTPResponse panicked!\n%s", debug.Stack(),
+			)
+		}
+	}()
+
 	for roomID, roomData := range res.Rooms.Join {
 		room := worker.client.getOrCreateRoom(roomID)
 		for _, event := range roomData.State.Events {

src/github.com/matrix-org/go-neb/polling/polling.go

@@ -5,6 +5,7 @@ import (
 	"github.com/matrix-org/go-neb/clients"
 	"github.com/matrix-org/go-neb/database"
 	"github.com/matrix-org/go-neb/types"
+	"runtime/debug"
 	"sync"
 	"time"
 )
@@ -71,6 +72,17 @@ func pollLoop(service types.Service, ts int64) {
 		"service_id":   service.ServiceID(),
 		"service_type": service.ServiceType(),
 	})
+
+	defer func() {
+		// Kill the poll loop entirely as it is likely that whatever made us panic will
+		// make us panic again. We can whine bitterly about it though.
+		if r := recover(); r != nil {
+			logger.WithField("panic", r).Errorf(
+				"pollLoop panicked!\n%s", debug.Stack(),
+			)
+		}
+	}()
+
 	poller, ok := service.(types.Poller)
 	if !ok {
 		logger.Error("Service is not a Poller.")

src/github.com/matrix-org/go-neb/server/server.go

@@ -6,6 +6,7 @@ import (
 	log "github.com/Sirupsen/logrus"
 	"github.com/matrix-org/go-neb/errors"
 	"net/http"
+	"runtime/debug"
 )
 
 // JSONRequestHandler represents an interface that must be satisfied in order to respond to incoming
@@ -34,14 +35,38 @@ func WithCORSOptions(handler http.HandlerFunc) http.HandlerFunc {
 	}
 }
 
+// Protect panicking HTTP requests from taking down the entire process, and log them using
+// the correct logger, returning a 500 with a JSON response rather than abruptly closing the
+// connection.
+func Protect(handler http.HandlerFunc) http.HandlerFunc {
+	return func(w http.ResponseWriter, req *http.Request) {
+		defer func() {
+			if r := recover(); r != nil {
+				log.WithFields(log.Fields{
+					"panic":  r,
+					"method": req.Method,
+					"url":    req.URL,
+				}).Errorf(
+					"Request panicked!\n%s", debug.Stack(),
+				)
+				jsonErrorResponse(
+					w, req, &errors.HTTPError{nil, "Internal Server Error", 500},
+				)
+			}
+		}()
+		handler(w, req)
+	}
+}
+
 // MakeJSONAPI creates an HTTP handler which always responds to incoming requests with JSON responses.
 func MakeJSONAPI(handler JSONRequestHandler) http.HandlerFunc {
-	return func(w http.ResponseWriter, req *http.Request) {
+	return Protect(func(w http.ResponseWriter, req *http.Request) {
 		logger := log.WithFields(log.Fields{
 			"method": req.Method,
 			"url":    req.URL,
 		})
-		logger.Print(">>> Incoming request")
+		logger.Print("Incoming request")
+
 		res, httpErr := handler.OnIncomingRequest(req)
 
 		// Set common headers returned regardless of the outcome of the request
@@ -67,7 +92,7 @@ func MakeJSONAPI(handler JSONRequestHandler) http.HandlerFunc {
 			resBytes = r
 		}
 		w.Write(resBytes)
-	}
+	})
 }
 
 func jsonErrorResponse(w http.ResponseWriter, req *http.Request, httpErr *errors.HTTPError) {

src/github.com/matrix-org/go-neb/server/server_test.go

@@ -0,0 +1,29 @@
+package server
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+func TestProtect(t *testing.T) {
+	mockWriter := httptest.NewRecorder()
+	mockReq, _ := http.NewRequest("GET", "http://example.com/foo", nil)
+	h := Protect(func(w http.ResponseWriter, req *http.Request) {
+		var array []string
+		w.Write([]byte(array[5])) // NPE
+	})
+
+	h(mockWriter, mockReq)
+
+	expectCode := 500
+	if mockWriter.Code != expectCode {
+		t.Errorf("TestProtect wanted HTTP status %d, got %d", expectCode, mockWriter.Code)
+	}
+
+	expectBody := `{"message":"Internal Server Error"}`
+	actualBody := mockWriter.Body.String()
+	if actualBody != expectBody {
+		t.Errorf("TestProtect wanted body %s, got %s", expectBody, actualBody)
+	}
+}