Browse Source

http2: fix optimized write scheduling

Fixes regression from https://golang.org/cl/31495 which generated
massive stacks like:

net/http.(*http2serverConn).startFrameWrite(0x1890e7e0, 0x859bc70, 0x18c26050, 0x1897c1c0, 0x0)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3664 +0x34a
net/http.(*http2serverConn).scheduleFrameWrite(0x1890e7e0)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3779 +0x114
net/http.(*http2serverConn).wroteFrame(0x1890e7e0, 0x859bb50, 0x18c26060, 0x0, 0x0, 0x0, 0x0)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3742 +0xc8
net/http.(*http2serverConn).startFrameWrite(0x1890e7e0, 0x859bb50, 0x18c26060, 0x0, 0x0)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3689 +0x23f
net/http.(*http2serverConn).scheduleFrameWrite(0x1890e7e0)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3779 +0x114
net/http.(*http2serverConn).writeFrame(0x1890e7e0, 0x859bb50, 0x18c26060, 0x0, 0x0)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3648 +0x6b
net/http.(*http2serverConn).resetStream(0x1890e7e0, 0x1, 0x8, 0x0, 0x0)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3815 +0x91
net/http.(*http2serverConn).wroteFrame(0x1890e7e0, 0x859b6d0, 0x1890db00, 0x1897c1c0, 0x18ede040, 0x0, 0x0)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3736 +0x124
net/http.(*http2serverConn).startFrameWrite(0x1890e7e0, 0x859b6d0, 0x1890db00, 0x1897c1c0, 0x18ede040)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3689 +0x23f
net/http.(*http2serverConn).scheduleFrameWrite(0x1890e7e0)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3779 +0x114
net/http.(*http2serverConn).wroteFrame(0x1890e7e0, 0x859bc70, 0x18c26018, 0x0, 0x0, 0x0, 0x0)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3742 +0xc8
net/http.(*http2serverConn).startFrameWrite(0x1890e7e0, 0x859bc70, 0x18c26018, 0x0, 0x0)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3689 +0x23f
net/http.(*http2serverConn).scheduleFrameWrite(0x1890e7e0)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3779 +0x114
net/http.(*http2serverConn).wroteFrame(0x1890e7e0, 0x859b730, 0x18ede000, 0x1897c1c0, 0x0, 0x0, 0x0)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3742 +0xc8
net/http.(*http2serverConn).serve(0x1890e7e0)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3504 +0x50d
net/http.(*http2Server).ServeConn(0x18ab49a0, 0x859e040, 0x18df2000, 0x1894ae18)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3152 +0x6d9
net/http.http2ConfigureServer.func1(0x18d364e0, 0x18df2000, 0x859a1b0, 0x18ab2308)
    /tmp/workdir/go/src/net/http/h2_bundle.go:3033 +0x70
net/http.(*conn).serve(0x18ad60f0, 0x859cbc0, 0x18aa42a0)
    /tmp/workdir/go/src/net/http/server.go:1633 +0xf03

This changes it to be a loop instead.

This also fixes the "internal error: attempt to send frame on
half-closed-local stream" crash from golang/go#17627 because
wroteFrame set stateHalfClosedLocal temporarily while calling
resetStream which itself does a write in some cases. Prior to CL 31495
those writes were processed breadth-first. CL 31495 made the writes
generated from resetStream (like the window updates returning unused
flow-control) more aggressive and scheduled immediately. This loop
restores the old write scheduling.

Fixes golang/go#17627

Change-Id: I9651568e4105791d24d46819499efc7e018c86c3
Reviewed-on: https://go-review.googlesource.com/32217
Reviewed-by: Tom Bergan <tombergan@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Brad Fitzpatrick 9 năm trước cách đây
mục cha
commit
b626cca987
1 tập tin đã thay đổi với 37 bổ sung26 xóa
  1. 37 26
      http2/server.go

+ 37 - 26
http2/server.go

@@ -392,9 +392,11 @@ type serverConn struct {
 	headerTableSize       uint32
 	peerMaxHeaderListSize uint32            // zero means unknown (default)
 	canonHeader           map[string]string // http2-lower-case -> Go-Canonical-Case
-	writingFrame          bool              // started write goroutine but haven't heard back on wroteFrameCh
+	writingFrame          bool              // started writing a frame (on serve goroutine or separate)
+	writingFrameAsync     bool              // started a frame on its own goroutine but haven't heard back on wroteFrameCh
 	needsFrameFlush       bool              // last frame write wasn't a flush
 	inGoAway              bool              // we've started to or sent GOAWAY
+	inFrameScheduleLoop   bool              // whether we're in the scheduleFrameWrite loop
 	needToSendGoAway      bool              // we need to schedule a GOAWAY frame write
 	goAwayCode            ErrCode
 	shutdownTimerCh       <-chan time.Time // nil until used
@@ -893,6 +895,7 @@ func (sc *serverConn) startFrameWrite(wr FrameWriteRequest) {
 		var err error
 		wpp.promisedID, err = wpp.allocatePromisedID()
 		if err != nil {
+			sc.writingFrameAsync = false
 			if wr.done != nil {
 				wr.done <- err
 			}
@@ -903,9 +906,11 @@ func (sc *serverConn) startFrameWrite(wr FrameWriteRequest) {
 	sc.writingFrame = true
 	sc.needsFrameFlush = true
 	if wr.write.staysWithinBuffer(sc.bw.Available()) {
+		sc.writingFrameAsync = false
 		err := wr.write.writeFrame(sc)
 		sc.wroteFrame(frameWriteResult{wr, err})
 	} else {
+		sc.writingFrameAsync = true
 		go sc.writeFrameAsync(wr)
 	}
 }
@@ -923,6 +928,7 @@ func (sc *serverConn) wroteFrame(res frameWriteResult) {
 		panic("internal error: expected to be already writing a frame")
 	}
 	sc.writingFrame = false
+	sc.writingFrameAsync = false
 
 	wr := res.wr
 	st := wr.stream
@@ -982,35 +988,40 @@ func (sc *serverConn) wroteFrame(res frameWriteResult) {
 // flush the write buffer.
 func (sc *serverConn) scheduleFrameWrite() {
 	sc.serveG.check()
-	if sc.writingFrame {
+	if sc.writingFrame || sc.inFrameScheduleLoop {
 		return
 	}
-	if sc.needToSendGoAway {
-		sc.needToSendGoAway = false
-		sc.startFrameWrite(FrameWriteRequest{
-			write: &writeGoAway{
-				maxStreamID: sc.maxStreamID,
-				code:        sc.goAwayCode,
-			},
-		})
-		return
-	}
-	if sc.needToSendSettingsAck {
-		sc.needToSendSettingsAck = false
-		sc.startFrameWrite(FrameWriteRequest{write: writeSettingsAck{}})
-		return
-	}
-	if !sc.inGoAway {
-		if wr, ok := sc.writeSched.Pop(); ok {
-			sc.startFrameWrite(wr)
-			return
+	sc.inFrameScheduleLoop = true
+	for !sc.writingFrameAsync {
+		if sc.needToSendGoAway {
+			sc.needToSendGoAway = false
+			sc.startFrameWrite(FrameWriteRequest{
+				write: &writeGoAway{
+					maxStreamID: sc.maxStreamID,
+					code:        sc.goAwayCode,
+				},
+			})
+			continue
 		}
+		if sc.needToSendSettingsAck {
+			sc.needToSendSettingsAck = false
+			sc.startFrameWrite(FrameWriteRequest{write: writeSettingsAck{}})
+			continue
+		}
+		if !sc.inGoAway {
+			if wr, ok := sc.writeSched.Pop(); ok {
+				sc.startFrameWrite(wr)
+				continue
+			}
+		}
+		if sc.needsFrameFlush {
+			sc.startFrameWrite(FrameWriteRequest{write: flushFrameWriter{}})
+			sc.needsFrameFlush = false // after startFrameWrite, since it sets this true
+			continue
+		}
+		break
 	}
-	if sc.needsFrameFlush {
-		sc.startFrameWrite(FrameWriteRequest{write: flushFrameWriter{}})
-		sc.needsFrameFlush = false // after startFrameWrite, since it sets this true
-		return
-	}
+	sc.inFrameScheduleLoop = false
 }
 
 func (sc *serverConn) goAway(code ErrCode) {