http2: fix state tracking for pushed streams
Fix sc.state, which was returning "idle" instead of "closed" if the max
pushed stream ID happened to exceed the max client stream ID. This caused
us to erroneously generate connection errors because we believed a state
invariant had been violated when it had not.
I also renamed maxStreamID to maxClientStreamID for clarity, since we
need to track client-generated and server-generated stream IDs separately.
Fixes golang/go#17777
Change-Id: Id3d5700d79cc699a267bd91d6ebace0770fa62fc
Reviewed-on: https://go-review.googlesource.com/32755
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
diff --git a/http2/server.go b/http2/server.go
index b8bf4ef..e020c29 100644
--- a/http2/server.go
+++ b/http2/server.go
@@ -386,8 +386,8 @@
advMaxStreams uint32 // our SETTINGS_MAX_CONCURRENT_STREAMS advertised the client
curClientStreams uint32 // number of open streams initiated by the client
curPushedStreams uint32 // number of open streams initiated by server push
- maxStreamID uint32 // max ever seen from client
- maxPushPromiseID uint32 // ID of the last push promise, or 0 if there have been no pushes
+ maxClientStreamID uint32 // max ever seen from client (odd), or 0 if there have been no client requests
+ maxPushPromiseID uint32 // ID of the last push promise (even), or 0 if there have been no pushes
streams map[uint32]*stream
initialWindowSize int32
maxFrameSize int32
@@ -477,8 +477,14 @@
// a client sends a HEADERS frame on stream 7 without ever sending a
// frame on stream 5, then stream 5 transitions to the "closed"
// state when the first frame for stream 7 is sent or received."
- if streamID <= sc.maxStreamID {
- return stateClosed, nil
+ if streamID%2 == 1 {
+ if streamID <= sc.maxClientStreamID {
+ return stateClosed, nil
+ }
+ } else {
+ if streamID <= sc.maxPushPromiseID {
+ return stateClosed, nil
+ }
}
return stateIdle, nil
}
@@ -1011,7 +1017,7 @@
sc.needToSendGoAway = false
sc.startFrameWrite(FrameWriteRequest{
write: &writeGoAway{
- maxStreamID: sc.maxStreamID,
+ maxStreamID: sc.maxClientStreamID,
code: sc.goAwayCode,
},
})
@@ -1495,10 +1501,10 @@
// endpoint has opened or reserved. [...] An endpoint that
// receives an unexpected stream identifier MUST respond with
// a connection error (Section 5.4.1) of type PROTOCOL_ERROR.
- if id <= sc.maxStreamID {
+ if id <= sc.maxClientStreamID {
return ConnectionError(ErrCodeProtocol)
}
- sc.maxStreamID = id
+ sc.maxClientStreamID = id
if sc.idleTimer != nil {
sc.idleTimer.Stop()
diff --git a/http2/server_push_test.go b/http2/server_push_test.go
index 5668839..3017be4 100644
--- a/http2/server_push_test.go
+++ b/http2/server_push_test.go
@@ -381,3 +381,49 @@
return nil
})
}
+
+func TestServer_Push_StateTransitions(t *testing.T) {
+ const body = "foo"
+
+ startedPromise := make(chan bool)
+ finishedPush := make(chan bool)
+ st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
+ switch r.URL.RequestURI() {
+ case "/":
+ if err := w.(http.Pusher).Push("/pushed", nil); err != nil {
+ t.Errorf("Push error: %v", err)
+ }
+ close(startedPromise)
+ // Don't finish this request until the push finishes so we don't
+ // nondeterministically interleave output frames with the push.
+ <-finishedPush
+ }
+ w.Header().Set("Content-Type", "text/html")
+ w.Header().Set("Content-Length", strconv.Itoa(len(body)))
+ w.WriteHeader(200)
+ io.WriteString(w, body)
+ })
+ defer st.Close()
+
+ st.greet()
+ if st.stream(2) != nil {
+ t.Fatal("stream 2 should be empty")
+ }
+ if got, want := st.streamState(2), stateIdle; got != want {
+ t.Fatalf("streamState(2)=%v, want %v", got, want)
+ }
+ getSlash(st)
+ <-startedPromise
+ if got, want := st.streamState(2), stateHalfClosedRemote; got != want {
+ t.Fatalf("streamState(2)=%v, want %v", got, want)
+ }
+ st.wantPushPromise()
+ st.wantHeaders()
+ if df := st.wantData(); !df.StreamEnded() {
+ t.Fatal("expected END_STREAM flag on DATA")
+ }
+ if got, want := st.streamState(2), stateClosed; got != want {
+ t.Fatalf("streamState(2)=%v, want %v", got, want)
+ }
+ close(finishedPush)
+}
