From d1bef43daf850912b52f6f743f8fce31dc7c7481 Mon Sep 17 00:00:00 2001 From: Artyom Pervukhin Date: Sun, 1 Oct 2017 14:24:16 +0300 Subject: [PATCH] net/http: make TimeoutHandler recover child handler panics Fixes #22084. Change-Id: If405ffdc57fcf81de3c0e8473c45fc504db735bc Reviewed-on: https://go-review.googlesource.com/67410 Run-TryBot: Tom Bergan TryBot-Result: Gobot Gobot Reviewed-by: Tom Bergan --- src/net/http/serve_test.go | 28 ++++++++++++++++++++-------- src/net/http/server.go | 8 ++++++++ 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index 68b78301cb..5520ac78e2 100644 --- a/src/net/http/serve_test.go +++ b/src/net/http/serve_test.go @@ -2438,6 +2438,14 @@ func TestTimeoutHandlerEmptyResponse(t *testing.T) { } } +// https://golang.org/issues/22084 +func TestTimeoutHandlerPanicRecovery(t *testing.T) { + wrapper := func(h Handler) Handler { + return TimeoutHandler(h, time.Second, "") + } + testHandlerPanic(t, false, false, wrapper, "intentional death for testing") +} + func TestRedirectBadPath(t *testing.T) { // This used to crash. It's not valid input (bad path), but it // shouldn't crash. @@ -2551,22 +2559,22 @@ func testZeroLengthPostAndResponse(t *testing.T, h2 bool) { } } -func TestHandlerPanicNil_h1(t *testing.T) { testHandlerPanic(t, false, h1Mode, nil) } -func TestHandlerPanicNil_h2(t *testing.T) { testHandlerPanic(t, false, h2Mode, nil) } +func TestHandlerPanicNil_h1(t *testing.T) { testHandlerPanic(t, false, h1Mode, nil, nil) } +func TestHandlerPanicNil_h2(t *testing.T) { testHandlerPanic(t, false, h2Mode, nil, nil) } func TestHandlerPanic_h1(t *testing.T) { - testHandlerPanic(t, false, h1Mode, "intentional death for testing") + testHandlerPanic(t, false, h1Mode, nil, "intentional death for testing") } func TestHandlerPanic_h2(t *testing.T) { - testHandlerPanic(t, false, h2Mode, "intentional death for testing") + testHandlerPanic(t, false, h2Mode, nil, "intentional death for testing") } func TestHandlerPanicWithHijack(t *testing.T) { // Only testing HTTP/1, and our http2 server doesn't support hijacking. - testHandlerPanic(t, true, h1Mode, "intentional death for testing") + testHandlerPanic(t, true, h1Mode, nil, "intentional death for testing") } -func testHandlerPanic(t *testing.T, withHijack, h2 bool, panicValue interface{}) { +func testHandlerPanic(t *testing.T, withHijack, h2 bool, wrapper func(Handler) Handler, panicValue interface{}) { defer afterTest(t) // Unlike the other tests that set the log output to ioutil.Discard // to quiet the output, this test uses a pipe. The pipe serves three @@ -2589,7 +2597,7 @@ func testHandlerPanic(t *testing.T, withHijack, h2 bool, panicValue interface{}) defer log.SetOutput(os.Stderr) defer pw.Close() - cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) { + var handler Handler = HandlerFunc(func(w ResponseWriter, r *Request) { if withHijack { rwc, _, err := w.(Hijacker).Hijack() if err != nil { @@ -2598,7 +2606,11 @@ func testHandlerPanic(t *testing.T, withHijack, h2 bool, panicValue interface{}) defer rwc.Close() } panic(panicValue) - })) + }) + if wrapper != nil { + handler = wrapper(handler) + } + cst := newClientServerTest(t, h2, handler) defer cst.close() // Do a blocking read on the log output pipe so its logging diff --git a/src/net/http/server.go b/src/net/http/server.go index d89f66058a..60575926bd 100644 --- a/src/net/http/server.go +++ b/src/net/http/server.go @@ -3081,11 +3081,19 @@ func (h *timeoutHandler) ServeHTTP(w ResponseWriter, r *Request) { w: w, h: make(Header), } + panicChan := make(chan interface{}, 1) go func() { + defer func() { + if p := recover(); p != nil { + panicChan <- p + } + }() h.handler.ServeHTTP(tw, r) close(done) }() select { + case p := <-panicChan: + panic(p) case <-done: tw.mu.Lock() defer tw.mu.Unlock() -- 2.50.0