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: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/net/http/clientserver_test.go b/src/net/http/clientserver_test.go
index 19dc156003bf69852f68eb957377ec9a320cbca0..0d231b87b043837df851d0ae0c6623836b1b3111 100644 (file)
--- a/src/net/http/clientserver_test.go
+++ b/src/net/http/clientserver_test.go
@@ -64,21 +64,23 @@ func newClientServerTest(t *testing.T, h2 bool, h Handler, opts ...interface{})
                tr: &Transport{},
        }
        cst.c = &Client{Transport: cst.tr}
+       cst.ts = httptest.NewUnstartedServer(h)
 
        for _, opt := range opts {
                switch opt := opt.(type) {
                case func(*Transport):
                        opt(cst.tr)
+               case func(*httptest.Server):
+                       opt(cst.ts)
                default:
                        t.Fatalf("unhandled option type %T", opt)
                }
        }
 
        if !h2 {
-               cst.ts = httptest.NewServer(h)
+               cst.ts.Start()
                return cst
        }
-       cst.ts = httptest.NewUnstartedServer(h)
        ExportHttp2ConfigureServer(cst.ts.Config, nil)
        cst.ts.TLS = cst.ts.Config.TLSConfig
        cst.ts.StartTLS()
@@ -1143,19 +1145,30 @@ func testBogusStatusWorks(t *testing.T, h2 bool) {
        }
 }
 
-func TestInterruptWithPanic_h1(t *testing.T) { testInterruptWithPanic(t, h1Mode) }
-func TestInterruptWithPanic_h2(t *testing.T) { testInterruptWithPanic(t, h2Mode) }
-func testInterruptWithPanic(t *testing.T, h2 bool) {
-       log.SetOutput(ioutil.Discard) // is noisy otherwise
-       defer log.SetOutput(os.Stderr)
-
+func TestInterruptWithPanic_h1(t *testing.T)     { testInterruptWithPanic(t, h1Mode, "boom") }
+func TestInterruptWithPanic_h2(t *testing.T)     { testInterruptWithPanic(t, h2Mode, "boom") }
+func TestInterruptWithPanic_nil_h1(t *testing.T) { testInterruptWithPanic(t, h1Mode, nil) }
+func TestInterruptWithPanic_nil_h2(t *testing.T) { testInterruptWithPanic(t, h2Mode, nil) }
+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"
        defer afterTest(t)
+
+       var errorLog lockedBytesBuffer
+
        cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) {
                io.WriteString(w, msg)
                w.(Flusher).Flush()
-               panic("no more")
-       }))
+               panic(panicValue)
+       }), func(ts *httptest.Server) {
+               ts.Config.ErrorLog = log.New(&errorLog, "", 0)
+       })
        defer cst.close()
        res, err := cst.c.Get(cst.ts.URL)
        if err != nil {
@@ -1169,6 +1182,35 @@ func testInterruptWithPanic(t *testing.T, h2 bool) {
        if err == nil {
                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

diff --git a/src/net/http/server.go b/src/net/http/server.go
index 90e723358770be07bf6c931c71107f76fb662bc8..2bc71c7dd5796a84f1172442fbdaf58c6594c196 100644 (file)
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -1674,11 +1674,17 @@ type badRequestError string
 
 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.
 func (c *conn) serve(ctx context.Context) {
        c.remoteAddr = c.rwc.RemoteAddr().String()
        defer func() {
-               if err := recover(); err != nil {
+               if err := recover(); err != nil && err != ErrAbortHandler {
                        const size = 64 << 10
                        buf := make([]byte, size)
                        buf = buf[:runtime.Stack(buf, false)]