[net] http2: fix handling of received GOAWAY frames in Transport
2,373 views
Skip to first unread message
Brad Fitzpatrick (Gerrit)
Dec 5, 2016, 7:14:48 PM12/5/16
to Tom Bergan, Ian Lance Taylor, Brad Fitzpatrick, golang-co...@googlegroups.com
Brad Fitzpatrick would like Tom Bergan to review this change.
http2: fix handling of received GOAWAY frames in Transport Also, use Go 1.8's Request.GetBody to rewind and replay requests when the server does a graceful shutdown. DO NOT SUBMIT Change-Id: I00a1cb748fe9c0f01c5bd4b8d1ac4438b56f1f8c --- M http2/go18.go M http2/not_go18.go M http2/transport.go M http2/transport_test.go 4 files changed, 95 insertions(+), 13 deletions(-)
diff --git a/http2/go18.go b/http2/go18.go
index 8c0dd25..a75296e 100644
--- a/http2/go18.go
+++ b/http2/go18.go
@@ -8,6 +8,7 @@
import (
"crypto/tls"
+ "io"
"net/http"
)
@@ -39,3 +40,7 @@
func shouldLogPanic(panicValue interface{}) bool {
return panicValue != nil && panicValue != http.ErrAbortHandler
}
+
+func reqGetBody(req *http.Request) func() (io.ReadCloser, error) {
+ return req.GetBody
+}
diff --git a/http2/not_go18.go b/http2/not_go18.go
index 2e600dc..41182fe 100644
--- a/http2/not_go18.go
+++ b/http2/not_go18.go
@@ -6,7 +6,10 @@
package http2
-import "net/http"
+import (
+ "io"
+ "net/http"
+)
func configureServer18(h1 *http.Server, h2 *Server) error {
// No IdleTimeout to sync prior to Go 1.8.
@@ -16,3 +19,7 @@
func shouldLogPanic(panicValue interface{}) bool {
return panicValue != nil
}
+
+func reqGetBody(req *http.Request) func() (io.ReadCloser, error) {
+ return nil
+}
diff --git a/http2/transport.go b/http2/transport.go
index 8f5f844..2439e43 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -191,6 +191,7 @@
ID uint32
resc chan resAndError
bufPipe pipe // buffered pipe with the flow-controlled response payload
+ startedWrite bool // started request body write; guarded by cc.mu
requestedGzip bool
on100 func() // optional code to run if get a 100 continue response
@@ -332,8 +333,10 @@
}
traceGotConn(req, cc)
res, err := cc.RoundTrip(req)
- if shouldRetryRequest(req, err) {
- continue
+ if err != nil {
+ if req, err = shouldRetryRequest(req, err); err == nil {
+ continue
+ }
}
if err != nil {
t.vlogf("RoundTrip failure: %v", err)
@@ -355,12 +358,30 @@
var (
errClientConnClosed = errors.New("http2: client conn is closed")
errClientConnUnusable = errors.New("http2: client conn not usable")
+
+ errClientConnGotGoAway = errors.New("http2: Transport received Server's graceful shutdown GOAWAY")
+ errClientConnGotGoAwayAfterSomeReqBody = errors.New("http2: Transport received Server's graceful shutdown GOAWAY; some request body already written")
)
-func shouldRetryRequest(req *http.Request, err error) bool {
- // TODO: retry GET requests (no bodies) more aggressively, if shutdown
- // before response.
- return err == errClientConnUnusable
+func shouldRetryRequest(req *http.Request, err error) (*http.Request, error) {
+ switch err {
+ default:
+ return nil, err
+ case errClientConnUnusable, errClientConnGotGoAway:
+ return req, nil
+ case errClientConnGotGoAwayAfterSomeReqBody:
+ getBody := reqGetBody(req)
+ if getBody == nil {
+ return nil, errors.New("http2: Transport: peer server initiated graceful shutdown after some of Request.Body was written; define Request.GetBody to avoid this error")
+ }
+ body, err := getBody()
+ if err != nil {
+ return nil, err
+ }
+ newReq := *req
+ newReq.Body = body
+ return &newReq, nil
+ }
}
func (t *Transport) dialClientConn(addr string, singleUse bool) (*ClientConn, error) {
@@ -512,6 +533,15 @@
}
if old != nil && old.ErrCode != ErrCodeNo {
cc.goAway.ErrCode = old.ErrCode
+ }
+ last := f.LastStreamID
+ for streamID, cs := range cc.streams {
+ if streamID > last {
+ select {
+ case cs.resc <- resAndError{err: errClientConnGotGoAway}:
+ default:
+ }
+ }
}
}
@@ -773,6 +803,13 @@
cs.abortRequestBodyWrite(errStopReqBodyWrite)
}
if re.err != nil {
+ if re.err == errClientConnGotGoAway {
+ cc.mu.Lock()
+ if cs.startedWrite {
+ re.err = errClientConnGotGoAwayAfterSomeReqBody
+ }
+ cc.mu.Unlock()
+ }
cc.forgetStreamID(cs.ID)
return nil, re.err
}
@@ -1997,12 +2034,13 @@
// of the request body, particularly regarding doing delayed writes of the body
// when the request contains "Expect: 100-continue".
type bodyWriterState struct {
- cs *clientStream
- timer *time.Timer // if non-nil, we're doing a delayed write
- fnonce *sync.Once // to call fn with
- fn func() // the code to run in the goroutine, writing the body
- resc chan error // result of fn's execution
- delay time.Duration // how long we should delay a delayed write for
+ cs *clientStream
+ timer *time.Timer // if non-nil, we're doing a delayed write
+ fnonce *sync.Once // to call fn with
+ fn func() // the code to run in the goroutine, writing the body
+ resc chan error // result of fn's execution
+ delay time.Duration // how long we should delay a delayed write for
+ started int32 // atomic
}
func (t *Transport) getBodyWriterState(cs *clientStream, body io.Reader) (s bodyWriterState) {
@@ -2013,6 +2051,9 @@
resc := make(chan error, 1)
s.resc = resc
s.fn = func() {
+ cs.cc.mu.Lock()
+ cs.startedWrite = true
+ cs.cc.mu.Unlock()
resc <- cs.writeRequestBody(body, cs.req.Body)
}
s.delay = t.expectContinueTimeout()
diff --git a/http2/transport_test.go b/http2/transport_test.go
index f9287e5..9a2d7f1 100644
--- a/http2/transport_test.go
+++ b/http2/transport_test.go
@@ -2745,3 +2745,32 @@
t.Errorf("Got = %q; want %q", slurp, msg)
}
}
+
+func TestTransportGetsGOAWAYLessThanStreamID(t *testing.T) {
+ ct := newClientTester(t)
+
+ ct.client = func() error {
+ req, _ := http.NewRequest("GET", "https://dummy.tld/", nil)
+ res, err := ct.tr.RoundTrip(req)
+ t.Logf("client got %T, %v", res, err)
+
+ return nil
+ }
+ ct.server = func() error {
+ ct.greet()
+
+ hf, err := ct.firstHeaders()
+ if err != nil {
+ return err
+ }
+ t.Logf("Got %v", hf)
+
+ if err := ct.fr.WriteGoAway(0 /*max id*/, ErrCodeNo, nil); err != nil {
+ return err
+ }
+ ct.cc.(*net.TCPConn).Close()
+ println("server done")
+ return nil
+ }
+ ct.run()
+}
To view, visit this change. To unsubscribe, visit settings.
Brad Fitzpatrick (Gerrit)
Dec 5, 2016, 7:30:49 PM12/5/16
to Brad Fitzpatrick, Tom Bergan, golang-co...@googlegroups.com
Brad Fitzpatrick uploaded patch set #2 to this change.
http2: fix handling of received GOAWAY frames in Transport Also, use Go 1.8's Request.GetBody to rewind and replay requests when the server does a graceful shutdown. DO NOT SUBMIT Change-Id: I00a1cb748fe9c0f01c5bd4b8d1ac4438b56f1f8c --- M http2/go18.go M http2/not_go18.go M http2/transport.go M http2/transport_test.go 4 files changed, 88 insertions(+), 7 deletions(-)
To view, visit this change. To unsubscribe, visit settings.
Tom Bergan (Gerrit)
Dec 5, 2016, 8:13:29 PM12/5/16
to Brad Fitzpatrick, golang-co...@googlegroups.com
Tom Bergan posted comments on this change.
Patch Set 2:
(1 comment)
Patch Set #2, Line 2749:
func TestTransportGetsGOAWAYLessThanStreamID(t *testing.T) {LGTM pending the new test
To view, visit this change. To unsubscribe, visit settings.
Brad Fitzpatrick (Gerrit)
Dec 5, 2016, 8:41:04 PM12/5/16
to Brad Fitzpatrick, Tom Bergan, golang-co...@googlegroups.com
Brad Fitzpatrick uploaded patch set #3 to this change.
http2: fix handling of received GOAWAY frames in Transport Also, use Go 1.8's Request.GetBody to rewind and replay requests when the server does a graceful shutdown. DO NOT SUBMIT Change-Id: I00a1cb748fe9c0f01c5bd4b8d1ac4438b56f1f8c --- M http2/go18.go M http2/not_go18.go M http2/transport.go M http2/transport_test.go 4 files changed, 83 insertions(+), 12 deletions(-)
To view, visit this change. To unsubscribe, visit settings.
Brad Fitzpatrick (Gerrit)
Dec 5, 2016, 8:47:42 PM12/5/16
to Brad Fitzpatrick, Tom Bergan, golang-co...@googlegroups.com
Brad Fitzpatrick uploaded patch set #4 to this change.
http2: fix handling of received GOAWAY frames in Transport Also, use Go 1.8's Request.GetBody to rewind and replay requests when the server does a graceful shutdown. DO NOT SUBMIT Change-Id: I00a1cb748fe9c0f01c5bd4b8d1ac4438b56f1f8c --- M http2/go18.go M http2/not_go18.go M http2/transport.go M http2/transport_test.go 4 files changed, 87 insertions(+), 12 deletions(-)
To view, visit this change. To unsubscribe, visit settings.
Brad Fitzpatrick (Gerrit)
Dec 5, 2016, 8:48:09 PM12/5/16
to Brad Fitzpatrick, Tom Bergan, golang-co...@googlegroups.com
Brad Fitzpatrick uploaded patch set #5 to this change.
http2: fix handling of received GOAWAY frames in Transport Also, use Go 1.8's Request.GetBody to rewind and replay requests when the server does a graceful shutdown. Updates golang/go#18083 Change-Id: I00a1cb748fe9c0f01c5bd4b8d1ac4438b56f1f8c --- M http2/go18.go M http2/not_go18.go M http2/transport.go M http2/transport_test.go 4 files changed, 87 insertions(+), 12 deletions(-)
To view, visit this change. To unsubscribe, visit settings.
Brad Fitzpatrick (Gerrit)
Dec 6, 2016, 12:25:12 PM12/6/16
to Brad Fitzpatrick, Tom Bergan, golang-co...@googlegroups.com
Brad Fitzpatrick uploaded patch set #6 to this change.
http2: make Transport retry on server's GOAWAY graceful shutdown Updates golang/go#18083 Change-Id: I00a1cb748fe9c0f01c5bd4b8d1ac4438b56f1f8c --- M http2/go18.go M http2/not_go18.go M http2/transport.go M http2/transport_test.go 4 files changed, 104 insertions(+), 12 deletions(-)
To view, visit this change. To unsubscribe, visit settings.
Brad Fitzpatrick (Gerrit)
Dec 6, 2016, 12:28:59 PM12/6/16
to Brad Fitzpatrick, Tom Bergan, golang-co...@googlegroups.com
Brad Fitzpatrick posted comments on this change.
Patch Set 6:
Tom, PTAL.
To view, visit this change. To unsubscribe, visit settings.
Tom Bergan (Gerrit)
Dec 6, 2016, 12:58:20 PM12/6/16
to Brad Fitzpatrick, golang-co...@googlegroups.com
Tom Bergan posted comments on this change.
Patch Set 6: Code-Review+2
(1 comment)
thanks for the test cleanups
Patch Set #6, Line 369:
replaydreplayed
To view, visit this change. To unsubscribe, visit settings.
Brad Fitzpatrick (Gerrit)
Dec 6, 2016, 1:14:31 PM12/6/16
to Brad Fitzpatrick, Tom Bergan, golang-co...@googlegroups.com
Brad Fitzpatrick uploaded patch set #7 to this change.
http2: make Transport retry on server's GOAWAY graceful shutdown Debugged & wrote with Tom Bergan. Updates golang/go#18083 Change-Id: I00a1cb748fe9c0f01c5bd4b8d1ac4438b56f1f8c --- M http2/go18.go M http2/not_go18.go M http2/transport.go M http2/transport_test.go 4 files changed, 104 insertions(+), 12 deletions(-)
To view, visit this change. To unsubscribe, visit settings.
Brad Fitzpatrick (Gerrit)
Dec 6, 2016, 1:14:47 PM12/6/16
to Brad Fitzpatrick, Tom Bergan, golang-co...@googlegroups.com
Brad Fitzpatrick posted comments on this change.
Patch Set 7: Run-TryBot+1
To view, visit this change. To unsubscribe, visit settings.
Gobot Gobot (Gerrit)
Dec 6, 2016, 1:14:50 PM12/6/16
to Brad Fitzpatrick, Tom Bergan, golang-co...@googlegroups.com
Gobot Gobot posted comments on this change.
Patch Set 7:
TryBots beginning. Status page: http://farmer.golang.org/try?commit=1705717c
To view, visit this change. To unsubscribe, visit settings.
Gobot Gobot (Gerrit)
Dec 6, 2016, 1:18:53 PM12/6/16
to Brad Fitzpatrick, Tom Bergan, golang-co...@googlegroups.com
Gobot Gobot posted comments on this change.
Patch Set 7: TryBot-Result+1
TryBots are happy.
To view, visit this change. To unsubscribe, visit settings.
Brad Fitzpatrick (Gerrit)
Dec 6, 2016, 1:33:03 PM12/6/16
to Brad Fitzpatrick, golang-...@googlegroups.com, Gobot Gobot, Tom Bergan, golang-co...@googlegroups.com
Brad Fitzpatrick merged this change.
Approvals:
Tom Bergan: Looks good to me, approved
Brad Fitzpatrick: Run TryBots
Gobot Gobot: TryBots succeeded
http2: make Transport retry on server's GOAWAY graceful shutdown Debugged & wrote with Tom Bergan. Updates golang/go#18083 Change-Id: I00a1cb748fe9c0f01c5bd4b8d1ac4438b56f1f8c Reviewed-on: https://go-review.googlesource.com/33971 Run-TryBot: Brad Fitzpatrick <brad...@golang.org> TryBot-Result: Gobot Gobot <go...@golang.org> Reviewed-by: Tom Bergan <tomb...@google.com> --- M http2/go18.go M http2/not_go18.go M http2/transport.go M http2/transport_test.go 4 files changed, 104 insertions(+), 12 deletions(-)
diff --git a/http2/go18.go b/http2/go18.go
index 8c0dd25..633202c 100644
--- a/http2/go18.go
+++ b/http2/go18.go
@@ -8,6 +8,7 @@
import (
"crypto/tls"
+ "io"
"net/http"
)
@@ -39,3 +40,11 @@
func shouldLogPanic(panicValue interface{}) bool {
return panicValue != nil && panicValue != http.ErrAbortHandler
}
+
+func reqGetBody(req *http.Request) func() (io.ReadCloser, error) {
+ return req.GetBody
+}
+
+func reqBodyIsNoBody(body io.ReadCloser) bool {
+ return body == http.NoBody
+}
diff --git a/http2/not_go18.go b/http2/not_go18.go
index 2e600dc..efbf83c 100644
--- a/http2/not_go18.go
+++ b/http2/not_go18.go
@@ -6,7 +6,10 @@
package http2
-import "net/http"
+import (
+ "io"
+ "net/http"
+)
func configureServer18(h1 *http.Server, h2 *Server) error {
// No IdleTimeout to sync prior to Go 1.8.
@@ -16,3 +19,9 @@
func shouldLogPanic(panicValue interface{}) bool {
return panicValue != nil
}
+
+func reqGetBody(req *http.Request) func() (io.ReadCloser, error) {
+ return nil
+}
+
+func reqBodyIsNoBody(io.ReadCloser) bool { return false }
diff --git a/http2/transport.go b/http2/transport.go
index 8f5f844..68c325a 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -191,6 +191,7 @@
ID uint32
resc chan resAndError
bufPipe pipe // buffered pipe with the flow-controlled response payload
+ startedWrite bool // started request body write; guarded by cc.mu
requestedGzip bool
on100 func() // optional code to run if get a 100 continue response
@@ -332,8 +333,10 @@
}
traceGotConn(req, cc)
res, err := cc.RoundTrip(req)
- if shouldRetryRequest(req, err) {
- continue
+ if err != nil {
+ if req, err = shouldRetryRequest(req, err); err == nil {
+ continue
+ }
}
if err != nil {
t.vlogf("RoundTrip failure: %v", err)
@@ -355,12 +358,41 @@
var (
errClientConnClosed = errors.New("http2: client conn is closed")
errClientConnUnusable = errors.New("http2: client conn not usable")
+
+ errClientConnGotGoAway = errors.New("http2: Transport received Server's graceful shutdown GOAWAY")
+ errClientConnGotGoAwayAfterSomeReqBody = errors.New("http2: Transport received Server's graceful shutdown GOAWAY; some request body already written")
)
-func shouldRetryRequest(req *http.Request, err error) bool {
- // TODO: retry GET requests (no bodies) more aggressively, if shutdown
- // before response.
- return err == errClientConnUnusable
+// shouldRetryRequest is called by RoundTrip when a request fails to get
+// response headers. It is always called with a non-nil error.
+// It returns either a request to retry (either the same request, or a
+// modified clone), or an error if the request can't be replayed.
+func shouldRetryRequest(req *http.Request, err error) (*http.Request, error) {
+ switch err {
+ default:
+ return nil, err
+ case errClientConnUnusable, errClientConnGotGoAway:
+ return req, nil
+ case errClientConnGotGoAwayAfterSomeReqBody:
+ // If the Body is nil (or http.NoBody), it's safe to reuse
+ // this request and its Body.
+ if req.Body == nil || reqBodyIsNoBody(req.Body) {
+ return req, nil
+ }
+ // Otherwise we depend on the Request having its GetBody
+ // func defined.
+ getBody := reqGetBody(req) // Go 1.8: getBody = req.GetBody
+ if getBody == nil {
+ return nil, errors.New("http2: Transport: peer server initiated graceful shutdown after some of Request.Body was written; define Request.GetBody to avoid this error")
+ }
+ body, err := getBody()
+ if err != nil {
+ return nil, err
+ }
+ newReq := *req
+ newReq.Body = body
+ return &newReq, nil
+ }
}
func (t *Transport) dialClientConn(addr string, singleUse bool) (*ClientConn, error) {
@@ -512,6 +544,15 @@
}
if old != nil && old.ErrCode != ErrCodeNo {
cc.goAway.ErrCode = old.ErrCode
+ }
+ last := f.LastStreamID
+ for streamID, cs := range cc.streams {
+ if streamID > last {
+ select {
+ case cs.resc <- resAndError{err: errClientConnGotGoAway}:
+ default:
+ }
+ }
}
}
@@ -773,6 +814,13 @@
cs.abortRequestBodyWrite(errStopReqBodyWrite)
}
if re.err != nil {
+ if re.err == errClientConnGotGoAway {
+ cc.mu.Lock()
+ if cs.startedWrite {
+ re.err = errClientConnGotGoAwayAfterSomeReqBody
+ }
+ cc.mu.Unlock()
+ }
cc.forgetStreamID(cs.ID)
return nil, re.err
}
@@ -2013,6 +2061,9 @@
resc := make(chan error, 1)
s.resc = resc
s.fn = func() {
+ cs.cc.mu.Lock()
+ cs.startedWrite = true
+ cs.cc.mu.Unlock()
resc <- cs.writeRequestBody(body, cs.req.Body)
}
s.delay = t.expectContinueTimeout()
diff --git a/http2/transport_test.go b/http2/transport_test.go
index cd8e7a1..d10b5f7 100644
--- a/http2/transport_test.go
+++ b/http2/transport_test.go
@@ -2747,7 +2747,6 @@
}
func TestTransportRetryAfterGOAWAY(t *testing.T) {
- t.Skip("to be unskipped by https://go-review.googlesource.com/c/33971/")
var dialer struct {
sync.Mutex
count int
@@ -2765,6 +2764,9 @@
dialer.Lock()
defer dialer.Unlock()
dialer.count++
+ if dialer.count == 3 {
+ return nil, errors.New("unexpected number of dials")
+ }
cc, err := net.Dial("tcp", ln.Addr().String())
if err != nil {
return nil, fmt.Errorf("dial error: %v", err)
@@ -2797,9 +2799,19 @@
go func() {
req, _ := http.NewRequest("GET", "https://dummy.tld/", nil)
res, err := tr.RoundTrip(req)
- t.Logf("client got %T, %v", res, err)
+ if res != nil {
+ res.Body.Close()
+ if got := res.Header.Get("Foo"); got != "bar" {
+ err = fmt.Errorf("foo header = %q; want bar", got)
+ }
+ }
+ if err != nil {
+ err = fmt.Errorf("RoundTrip: %v", err)
+ }
errs <- err
}()
+
+ connToClose := make(chan io.Closer, 2)
// Server for the first request.
go func() {
@@ -2810,6 +2822,7 @@
return
}
+ connToClose <- ct.cc
ct.greet()
hf, err := ct.firstHeaders()
if err != nil {
@@ -2821,7 +2834,6 @@
errs <- fmt.Errorf("server1 failed writing GOAWAY: %v", err)
return
}
- ct.cc.(*net.TCPConn).Close()
errs <- nil
}()
@@ -2834,17 +2846,19 @@
return
}
+ connToClose <- ct.cc
ct.greet()
hf, err := ct.firstHeaders()
if err != nil {
errs <- fmt.Errorf("server2 failed reading HEADERS: %v", err)
return
}
- t.Logf("server2 Got %v", hf)
+ t.Logf("server2 got %v", hf)
var buf bytes.Buffer
enc := hpack.NewEncoder(&buf)
enc.WriteField(hpack.HeaderField{Name: ":status", Value: "200"})
+ enc.WriteField(hpack.HeaderField{Name: "foo", Value: "bar"})
err = ct.fr.WriteHeaders(HeadersFrameParam{
StreamID: hf.StreamID,
EndHeaders: true,
@@ -2852,7 +2866,7 @@
BlockFragment: buf.Bytes(),
})
if err != nil {
- errs <- fmt.Errorf("server2 failed writin responseg HEADERS: %v", err)
+ errs <- fmt.Errorf("server2 failed writing response HEADERS: %v", err)
} else {
errs <- nil
}
@@ -2868,4 +2882,13 @@
t.Errorf("timed out")
}
}
+
+ for {
+ select {
+ case c := <-connToClose:
+ c.Close()
+ default:
+ return
+ }
+ }
}
To view, visit this change. To unsubscribe, visit settings.
Reply all
Reply to author
Forward
0 new messages