From b5e8f7aa4184aba79100ddfc221f7f97dd0f7708 Mon Sep 17 00:00:00 2001
From: Jordan Liggitt <jliggitt@redhat.com>
Date: Fri, 28 Aug 2015 14:01:31 -0400
Subject: [PATCH] Recover panics in finishRequest, write correct API response

---
 pkg/apiserver/apiserver.go   | 22 ++++++++++++++++++++++
 pkg/apiserver/resthandler.go | 10 ++++++++++
 pkg/master/master.go         | 18 +-----------------
 pkg/util/util.go             |  6 +++++-
 4 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/pkg/apiserver/apiserver.go b/pkg/apiserver/apiserver.go
index 49fe11c7a28..2f2f11d6921 100644
--- a/pkg/apiserver/apiserver.go
+++ b/pkg/apiserver/apiserver.go
@@ -25,6 +25,7 @@ import (
 	"net"
 	"net/http"
 	"path"
+	rt "runtime"
 	"strconv"
 	"strings"
 	"time"
@@ -157,6 +158,27 @@ func InstallLogsSupport(mux Mux) {
 	mux.Handle("/logs/", http.StripPrefix("/logs/", http.FileServer(http.Dir("/var/log/"))))
 }
 
+func InstallRecoverHandler(container *restful.Container) {
+	container.RecoverHandler(logStackOnRecover)
+}
+
+//TODO: Unify with RecoverPanics?
+func logStackOnRecover(panicReason interface{}, httpWriter http.ResponseWriter) {
+	var buffer bytes.Buffer
+	buffer.WriteString(fmt.Sprintf("recover from panic situation: - %v\r\n", panicReason))
+	for i := 2; ; i += 1 {
+		_, file, line, ok := rt.Caller(i)
+		if !ok {
+			break
+		}
+		buffer.WriteString(fmt.Sprintf("    %s:%d\r\n", file, line))
+	}
+	glog.Errorln(buffer.String())
+
+	// TODO: make status unversioned or plumb enough of the request to deduce the requested API version
+	errorJSON(apierrors.NewGenericServerResponse(http.StatusInternalServerError, "", "", "", "", 0, false), latest.Codec, httpWriter)
+}
+
 func InstallServiceErrorHandler(container *restful.Container, requestResolver *APIRequestInfoResolver, apiVersions []string) {
 	container.ServiceErrorHandler(func(serviceErr restful.ServiceError, request *restful.Request, response *restful.Response) {
 		serviceErrorHandler(requestResolver, apiVersions, serviceErr, request, response)
diff --git a/pkg/apiserver/resthandler.go b/pkg/apiserver/resthandler.go
index 890eb41a007..87f6c6daa23 100644
--- a/pkg/apiserver/resthandler.go
+++ b/pkg/apiserver/resthandler.go
@@ -29,6 +29,7 @@ import (
 	"k8s.io/kubernetes/pkg/api/rest"
 	"k8s.io/kubernetes/pkg/fields"
 	"k8s.io/kubernetes/pkg/runtime"
+	"k8s.io/kubernetes/pkg/util"
 	"k8s.io/kubernetes/pkg/util/strategicpatch"
 
 	"github.com/emicklei/go-restful"
@@ -618,7 +619,14 @@ func finishRequest(timeout time.Duration, fn resultFunc) (result runtime.Object,
 	// when the select statement reads something other than the one the goroutine sends on.
 	ch := make(chan runtime.Object, 1)
 	errCh := make(chan error, 1)
+	panicCh := make(chan interface{}, 1)
 	go func() {
+		// panics don't cross goroutine boundaries, so we have to handle ourselves
+		defer util.HandleCrash(func(panicReason interface{}) {
+			// Propagate to parent goroutine
+			panicCh <- panicReason
+		})
+
 		if result, err := fn(); err != nil {
 			errCh <- err
 		} else {
@@ -634,6 +642,8 @@ func finishRequest(timeout time.Duration, fn resultFunc) (result runtime.Object,
 		return result, nil
 	case err = <-errCh:
 		return nil, err
+	case p := <-panicCh:
+		panic(p)
 	case <-time.After(timeout):
 		return nil, errors.NewTimeoutError("request did not complete within allowed duration", 0)
 	}
diff --git a/pkg/master/master.go b/pkg/master/master.go
index c1a9ab96ab6..378e2a65f58 100644
--- a/pkg/master/master.go
+++ b/pkg/master/master.go
@@ -17,7 +17,6 @@ limitations under the License.
 package master
 
 import (
-	"bytes"
 	"fmt"
 	"io/ioutil"
 	"math/rand"
@@ -26,7 +25,6 @@ import (
 	"net/http/pprof"
 	"net/url"
 	"os"
-	rt "runtime"
 	"strconv"
 	"strings"
 	"sync"
@@ -410,24 +408,10 @@ func (m *Master) HandleFuncWithAuth(pattern string, handler func(http.ResponseWr
 func NewHandlerContainer(mux *http.ServeMux) *restful.Container {
 	container := restful.NewContainer()
 	container.ServeMux = mux
-	container.RecoverHandler(logStackOnRecover)
+	apiserver.InstallRecoverHandler(container)
 	return container
 }
 
-//TODO: Unify with RecoverPanics?
-func logStackOnRecover(panicReason interface{}, httpWriter http.ResponseWriter) {
-	var buffer bytes.Buffer
-	buffer.WriteString(fmt.Sprintf("recover from panic situation: - %v\r\n", panicReason))
-	for i := 2; ; i += 1 {
-		_, file, line, ok := rt.Caller(i)
-		if !ok {
-			break
-		}
-		buffer.WriteString(fmt.Sprintf("    %s:%d\r\n", file, line))
-	}
-	glog.Errorln(buffer.String())
-}
-
 // init initializes master.
 func (m *Master) init(c *Config) {
 	enableCacher := true
diff --git a/pkg/util/util.go b/pkg/util/util.go
index 5a702fc037e..f75b59c709b 100644
--- a/pkg/util/util.go
+++ b/pkg/util/util.go
@@ -44,7 +44,8 @@ var ReallyCrash bool
 var PanicHandlers = []func(interface{}){logPanic}
 
 // HandleCrash simply catches a crash and logs an error. Meant to be called via defer.
-func HandleCrash() {
+// Additional context-specific handlers can be provided, and will be called in case of panic
+func HandleCrash(additionalHandlers ...func(interface{})) {
 	if ReallyCrash {
 		return
 	}
@@ -52,6 +53,9 @@ func HandleCrash() {
 		for _, fn := range PanicHandlers {
 			fn(r)
 		}
+		for _, fn := range additionalHandlers {
+			fn(r)
+		}
 	}
 }