Commit 9997545a authored by Brad Fitzpatrick's avatar Brad Fitzpatrick

net/http: add ErrAbortHandler, make Server quiet if used as panic value

Add an explicit way for Handlers to abort their response to the client
and also not spam their error log with stack traces.

panic(nil) also worked in the past (for http1 at least), so continue
to make that work (and test it). But ErrAbortHandler is more explicit.

Updates #17790 (needs http2 updates also)

Change-Id: Ib1456905b27e2ae8cf04c0983dc73e314a4a751e
Reviewed-on: https://go-review.googlesource.com/33099
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: default avatarJoe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
parent 1c541193
...@@ -64,21 +64,23 @@ func newClientServerTest(t *testing.T, h2 bool, h Handler, opts ...interface{}) ...@@ -64,21 +64,23 @@ func newClientServerTest(t *testing.T, h2 bool, h Handler, opts ...interface{})
tr: &Transport{}, tr: &Transport{},
} }
cst.c = &Client{Transport: cst.tr} cst.c = &Client{Transport: cst.tr}
cst.ts = httptest.NewUnstartedServer(h)
for _, opt := range opts { for _, opt := range opts {
switch opt := opt.(type) { switch opt := opt.(type) {
case func(*Transport): case func(*Transport):
opt(cst.tr) opt(cst.tr)
case func(*httptest.Server):
opt(cst.ts)
default: default:
t.Fatalf("unhandled option type %T", opt) t.Fatalf("unhandled option type %T", opt)
} }
} }
if !h2 { if !h2 {
cst.ts = httptest.NewServer(h) cst.ts.Start()
return cst return cst
} }
cst.ts = httptest.NewUnstartedServer(h)
ExportHttp2ConfigureServer(cst.ts.Config, nil) ExportHttp2ConfigureServer(cst.ts.Config, nil)
cst.ts.TLS = cst.ts.Config.TLSConfig cst.ts.TLS = cst.ts.Config.TLSConfig
cst.ts.StartTLS() cst.ts.StartTLS()
...@@ -1143,19 +1145,30 @@ func testBogusStatusWorks(t *testing.T, h2 bool) { ...@@ -1143,19 +1145,30 @@ func testBogusStatusWorks(t *testing.T, h2 bool) {
} }
} }
func TestInterruptWithPanic_h1(t *testing.T) { testInterruptWithPanic(t, h1Mode) } func TestInterruptWithPanic_h1(t *testing.T) { testInterruptWithPanic(t, h1Mode, "boom") }
func TestInterruptWithPanic_h2(t *testing.T) { testInterruptWithPanic(t, h2Mode) } func TestInterruptWithPanic_h2(t *testing.T) { testInterruptWithPanic(t, h2Mode, "boom") }
func testInterruptWithPanic(t *testing.T, h2 bool) { func TestInterruptWithPanic_nil_h1(t *testing.T) { testInterruptWithPanic(t, h1Mode, nil) }
log.SetOutput(ioutil.Discard) // is noisy otherwise func TestInterruptWithPanic_nil_h2(t *testing.T) { testInterruptWithPanic(t, h2Mode, nil) }
defer log.SetOutput(os.Stderr) func TestInterruptWithPanic_ErrAbortHandler_h1(t *testing.T) {
testInterruptWithPanic(t, h1Mode, ErrAbortHandler)
}
func TestInterruptWithPanic_ErrAbortHandler_h2(t *testing.T) {
testInterruptWithPanic(t, h2Mode, ErrAbortHandler)
}
func testInterruptWithPanic(t *testing.T, h2 bool, panicValue interface{}) {
setParallel(t)
const msg = "hello" const msg = "hello"
defer afterTest(t) defer afterTest(t)
var errorLog lockedBytesBuffer
cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) { cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) {
io.WriteString(w, msg) io.WriteString(w, msg)
w.(Flusher).Flush() w.(Flusher).Flush()
panic("no more") panic(panicValue)
})) }), func(ts *httptest.Server) {
ts.Config.ErrorLog = log.New(&errorLog, "", 0)
})
defer cst.close() defer cst.close()
res, err := cst.c.Get(cst.ts.URL) res, err := cst.c.Get(cst.ts.URL)
if err != nil { if err != nil {
...@@ -1169,6 +1182,35 @@ func testInterruptWithPanic(t *testing.T, h2 bool) { ...@@ -1169,6 +1182,35 @@ func testInterruptWithPanic(t *testing.T, h2 bool) {
if err == nil { if err == nil {
t.Errorf("client read all successfully; want some error") t.Errorf("client read all successfully; want some error")
} }
wantStackLogged := panicValue != nil && panicValue != ErrAbortHandler
errorLog.Lock()
gotLog := errorLog.String()
if !wantStackLogged {
if gotLog == "" {
return
}
if h2 {
t.Skip("TODO: make http2.Server respect ErrAbortHandler")
}
t.Fatalf("want no log output; got: %s", gotLog)
}
if gotLog == "" {
t.Fatalf("wanted a stack trace logged; got nothing")
}
if !strings.Contains(gotLog, "created by ") && strings.Count(gotLog, "\n") < 6 {
t.Errorf("output doesn't look like a panic stack trace. Got: %s", gotLog)
}
}
type lockedBytesBuffer struct {
sync.Mutex
bytes.Buffer
}
func (b *lockedBytesBuffer) Write(p []byte) (int, error) {
b.Lock()
defer b.Unlock()
return b.Buffer.Write(p)
} }
// Issue 15366 // Issue 15366
......
...@@ -1674,11 +1674,17 @@ type badRequestError string ...@@ -1674,11 +1674,17 @@ type badRequestError string
func (e badRequestError) Error() string { return "Bad Request: " + string(e) } func (e badRequestError) Error() string { return "Bad Request: " + string(e) }
// ErrAbortHandler is a sentinel panic value to abort a handler.
// While any panic from ServeHTTP aborts the response to the client,
// panicking with ErrAbortHandler also suppresses logging of a stack
// trace to the server's error log.
var ErrAbortHandler = errors.New("net/http: abort Handler")
// Serve a new connection. // Serve a new connection.
func (c *conn) serve(ctx context.Context) { func (c *conn) serve(ctx context.Context) {
c.remoteAddr = c.rwc.RemoteAddr().String() c.remoteAddr = c.rwc.RemoteAddr().String()
defer func() { defer func() {
if err := recover(); err != nil { if err := recover(); err != nil && err != ErrAbortHandler {
const size = 64 << 10 const size = 64 << 10
buf := make([]byte, size) buf := make([]byte, size)
buf = buf[:runtime.Stack(buf, false)] buf = buf[:runtime.Stack(buf, false)]
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment