[internal-branch.go1.21-vendor] http2: send correct LastStreamID in stream-caused GOAWAY
When closing a connection because a stream contained a request we
didn't like (for example, because the request headers exceed
the maximum we will accept), set the LastStreamID in the GOAWAY
frame to include the offending stream. This informs the client
that retrying the request is unlikely to succeed, and avoids
retry loops.
This change requires passing the stream ID of the offending
stream from Framer.ReadFrame up to the caller. The most sensible
way to do this would probably be in the error. However,
ReadFrame currently returns a defined error type for
connection-ending errors (ConnectionError), and that type is a
uint32 with no place to put the stream ID. Rather than changing
the returned errors, ReadFrame now returns an error along with
a non-nil Frame containing the stream ID, when a stream is
responsible for a connection-ending error.
Merge conflicts were avoided by cherry-picking CL 576235 (test deflake)
prior to this, and then by squashing CL 576175 (typo fix) into this CL.
For golang/go#66668.
For golang/go#66697.
Change-Id: Iba07ccbd70ab4939aa56903605474d01703ac6e4
Reviewed-on: https://go-review.googlesource.com/c/net/+/576756
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-on: https://go-review.googlesource.com/c/net/+/578336
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
diff --git a/http2/frame.go b/http2/frame.go
index 175c154..f9b6cbd 100644
--- a/http2/frame.go
+++ b/http2/frame.go
@@ -490,6 +490,9 @@
// returned error is ErrFrameTooLarge. Other errors may be of type
// ConnectionError, StreamError, or anything else from the underlying
// reader.
+//
+// If ReadFrame returns an error and a non-nil Frame, the Frame's StreamID
+// indicates the stream responsible for the error.
func (fr *Framer) ReadFrame() (Frame, error) {
fr.errDetail = nil
if fr.lastFrame != nil {
@@ -1522,7 +1525,7 @@
// readMetaFrame returns 0 or more CONTINUATION frames from fr and
// merge them into the provided hf and returns a MetaHeadersFrame
// with the decoded hpack values.
-func (fr *Framer) readMetaFrame(hf *HeadersFrame) (*MetaHeadersFrame, error) {
+func (fr *Framer) readMetaFrame(hf *HeadersFrame) (Frame, error) {
if fr.AllowIllegalReads {
return nil, errors.New("illegal use of AllowIllegalReads with ReadMetaHeaders")
}
@@ -1592,8 +1595,8 @@
log.Printf("http2: header list too large")
}
// It would be nice to send a RST_STREAM before sending the GOAWAY,
- // but the struture of the server's frame writer makes this difficult.
- return nil, ConnectionError(ErrCodeProtocol)
+ // but the structure of the server's frame writer makes this difficult.
+ return mh, ConnectionError(ErrCodeProtocol)
}
// Also close the connection after any CONTINUATION frame following an
@@ -1604,12 +1607,12 @@
log.Printf("http2: invalid header: %v", invalid)
}
// It would be nice to send a RST_STREAM before sending the GOAWAY,
- // but the struture of the server's frame writer makes this difficult.
- return nil, ConnectionError(ErrCodeProtocol)
+ // but the structure of the server's frame writer makes this difficult.
+ return mh, ConnectionError(ErrCodeProtocol)
}
if _, err := hdec.Write(frag); err != nil {
- return nil, ConnectionError(ErrCodeCompression)
+ return mh, ConnectionError(ErrCodeCompression)
}
if hc.HeadersEnded() {
@@ -1626,7 +1629,7 @@
mh.HeadersFrame.invalidate()
if err := hdec.Close(); err != nil {
- return nil, ConnectionError(ErrCodeCompression)
+ return mh, ConnectionError(ErrCodeCompression)
}
if invalid != nil {
fr.errDetail = invalid
diff --git a/http2/server.go b/http2/server.go
index 739f69f..890528f 100644
--- a/http2/server.go
+++ b/http2/server.go
@@ -1489,6 +1489,11 @@
sc.goAway(ErrCodeFlowControl)
return true
case ConnectionError:
+ if res.f != nil {
+ if id := res.f.Header().StreamID; id > sc.maxClientStreamID {
+ sc.maxClientStreamID = id
+ }
+ }
sc.logf("http2: server connection error from %v: %v", sc.conn.RemoteAddr(), ev)
sc.goAway(ErrCode(ev))
return true // goAway will handle shutdown
diff --git a/http2/server_test.go b/http2/server_test.go
index 3690163..7ab1d7d 100644
--- a/http2/server_test.go
+++ b/http2/server_test.go
@@ -4901,9 +4901,17 @@
if err != nil {
break
}
- switch f.(type) {
+ switch f := f.(type) {
case *HeadersFrame:
t.Fatalf("received HEADERS frame; want GOAWAY and a closed connection")
+ case *GoAwayFrame:
+ // We might not see the GOAWAY (see below), but if we do it should
+ // indicate that the server processed this request so the client doesn't
+ // attempt to retry it.
+ if got, want := f.LastStreamID, uint32(1); got != want {
+ t.Errorf("received GOAWAY with LastStreamId %v, want %v", got, want)
+ }
+
}
}
// We expect to have seen a GOAWAY before the connection closes,
