Skip to content

Commit

Permalink
apiserver: forward panic in WithTimeout filter
Browse files Browse the repository at this point in the history
  • Loading branch information
sttts committed Aug 29, 2018
1 parent 9c4cf0a commit eec1b52
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 8 deletions.
1 change: 1 addition & 0 deletions staging/src/k8s.io/apiserver/pkg/server/filters/BUILD
Expand Up @@ -20,6 +20,7 @@ go_test(
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/authentication/user:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/endpoints/filters:go_default_library",
Expand Down
11 changes: 8 additions & 3 deletions staging/src/k8s.io/apiserver/pkg/server/filters/timeout.go
Expand Up @@ -91,14 +91,19 @@ func (t *timeoutHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
return
}

done := make(chan struct{})
result := make(chan interface{})
tw := newTimeoutWriter(w)
go func() {
defer func() {
result <- recover()

This comment has been minimized.

Copy link
@lavalamp

lavalamp Nov 14, 2018

Member

This change breaks the ability to get a useful stack trace when something panics.

This comment has been minimized.

Copy link
@sttts

sttts Nov 15, 2018

Author Contributor

Before this, panics were not forwarded at all beyond the timeout handler. But we should preserve the stacktrace, totally agree. Will improve this.

This comment has been minimized.

Copy link
@sttts

sttts Nov 15, 2018

Author Contributor

Even worse, this PR was against the apiserver process to terminate due to an uncatched panic.

This comment has been minimized.

Copy link
@sttts

sttts Nov 15, 2018

Author Contributor
}()
t.handler.ServeHTTP(tw, r)
close(done)
}()
select {
case <-done:
case err := <-result:
if err != nil {
panic(err)
}
return
case <-after:
postTimeoutFn()
Expand Down
32 changes: 27 additions & 5 deletions staging/src/k8s.io/apiserver/pkg/server/filters/timeout_test.go
Expand Up @@ -30,6 +30,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/diff"
"k8s.io/apimachinery/pkg/util/runtime"
)

type recorder struct {
Expand All @@ -50,22 +51,33 @@ func (r *recorder) Count() int {
}

func TestTimeout(t *testing.T) {
origReallyCrash := runtime.ReallyCrash
runtime.ReallyCrash = false
defer func() {
runtime.ReallyCrash = origReallyCrash
}()

sendResponse := make(chan struct{}, 1)
doPanic := make(chan struct{}, 1)
writeErrors := make(chan error, 1)
timeout := make(chan time.Time, 1)
resp := "test response"
timeoutErr := apierrors.NewServerTimeout(schema.GroupResource{Group: "foo", Resource: "bar"}, "get", 0)
record := &recorder{}

ts := httptest.NewServer(WithTimeout(http.HandlerFunc(
ts := httptest.NewServer(WithPanicRecovery(WithTimeout(http.HandlerFunc(
func(w http.ResponseWriter, r *http.Request) {
<-sendResponse
_, err := w.Write([]byte(resp))
writeErrors <- err
select {
case <-sendResponse:
_, err := w.Write([]byte(resp))
writeErrors <- err
case <-doPanic:
panic("inner handler panics")
}
}),
func(req *http.Request) (*http.Request, <-chan time.Time, func(), *apierrors.StatusError) {
return req, timeout, record.Record, timeoutErr
}))
})))
defer ts.Close()

// No timeouts
Expand Down Expand Up @@ -114,4 +126,14 @@ func TestTimeout(t *testing.T) {
if err := <-writeErrors; err != http.ErrHandlerTimeout {
t.Errorf("got Write error of %v; expected %v", err, http.ErrHandlerTimeout)
}

// Panics
doPanic <- struct{}{}
res, err = http.Get(ts.URL)
if err != nil {
t.Fatal(err)
}
if res.StatusCode != http.StatusInternalServerError {
t.Errorf("got res.StatusCode %d; expected %d due to panic", res.StatusCode, http.StatusInternalServerError)
}
}

0 comments on commit eec1b52

Please sign in to comment.