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>

2 files changed, 59 insertions, 11 deletions

diff --git a/src/net/http/clientserver_test.go b/src/net/http/clientserver_test.go
index 19dc156003..0d231b87b0 100644
--- 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 90e7233587..2bc71c7dd5 100644
--- 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)]