http2: fix readFrames goroutine spin between ConnectionError and conn close

When the http2 Server reads a frame resulting in a ConnectionError it
first sends a GOAWAY frame (optionally with debug data telling the
peer's implementation what they did wrong) and then pauses for a bit
before shutting down the connection. The pause is because in general,
GOAWAY frames are used for graceful shutdown where the server tells
the client which maximum stream ID it received and will process. and
the client knows it can retry higher stream IDs.

The Go server was spinning in readFrames in the time between reading
certain types of errors and the connection going away.

Change-Id: Ie8c4988684a82cbb87be20086ba371355947163a
Reviewed-on: https://go-review.googlesource.com/18105
Reviewed-by: Ian Lance Taylor <iant@golang.org>

http2/frame.go

@@ -361,10 +361,22 @@ func (fr *Framer) SetMaxReadFrameSize(v uint32) {
 // sends a frame that is larger than declared with SetMaxReadFrameSize.
 var ErrFrameTooLarge = errors.New("http2: frame too large")
 
+// terminalReadFrameError reports whether err is an unrecoverable
+// error from ReadFrame and no other frames should be read.
+func terminalReadFrameError(err error) bool {
+	if _, ok := err.(StreamError); ok {
+		return false
+	}
+	return err != nil
+}
+
 // ReadFrame reads a single frame. The returned Frame is only valid
 // until the next call to ReadFrame.
-// If the frame is larger than previously set with SetMaxReadFrameSize,
-// the returned error is ErrFrameTooLarge.
+//
+// If the frame is larger than previously set with SetMaxReadFrameSize, the
+// returned error is ErrFrameTooLarge. Other errors may of type
+// ConnectionError, StreamError, or anything else from from the underlying
+// reader.
 func (fr *Framer) ReadFrame() (Frame, error) {
 	if fr.lastFrame != nil {
 		fr.lastFrame.invalidate()

http2/server.go

@@ -621,6 +621,9 @@ func (sc *serverConn) readFrames() {
 		case <-sc.doneServing:
 			return
 		}
+		if terminalReadFrameError(err) {
+			return
+		}
 	}
 }
 

http2/server_test.go

@@ -941,6 +941,15 @@ func TestServer_RejectsLargeFrames(t *testing.T) {
 	if gf.ErrCode != ErrCodeFrameSize {
 		t.Errorf("GOAWAY err = %v; want %v", gf.ErrCode, ErrCodeFrameSize)
 	}
+	if st.logBuf.Len() != 0 {
+		// Previously we spun here for a bit until the GOAWAY disconnect
+		// timer fired, logging while we fired.
+		trunc := st.logBuf.Len()
+		if trunc > 500 {
+			trunc = 500
+		}
+		t.Errorf("unexpected server output: %s\n", st.logBuf.Bytes()[:trunc])
+	}
 }
 
 func TestServer_Handler_Sends_WindowUpdate(t *testing.T) {