http2: prioritize RST_STREAM frames in priority write scheduler
The http2 priority write scheduler should not queue RST_STREAM
frames with the DATA frames, and instead treat them as control frames.
There can be deadlock situations if data frames block the queue,
because if the sender wants to close the stream it sends an RST frame,
but if the client is not draining the queue, the RST frame is stuck
and the sender is not able to finish.
Fixes golang/go#49741
Signed-off-by: Antonio Ojea <antonio.ojea.garcia@gmail.com>
Change-Id: Ie4462603380039f7aba8f701fe810d1d84376efa
Reviewed-on: https://go-review.googlesource.com/c/net/+/382014
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Run-TryBot: Dave Cheney <dave@cheney.net>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/http2/writesched_priority.go b/http2/writesched_priority.go
index 2618b2c..0a242c6 100644
--- a/http2/writesched_priority.go
+++ b/http2/writesched_priority.go
@@ -383,16 +383,15 @@
func (ws *priorityWriteScheduler) Push(wr FrameWriteRequest) {
var n *priorityNode
- if id := wr.StreamID(); id == 0 {
+ if wr.isControl() {
n = &ws.root
} else {
+ id := wr.StreamID()
n = ws.nodes[id]
if n == nil {
// id is an idle or closed stream. wr should not be a HEADERS or
- // DATA frame. However, wr can be a RST_STREAM. In this case, we
- // push wr onto the root, rather than creating a new priorityNode,
- // since RST_STREAM is tiny and the stream's priority is unknown
- // anyway. See issue #17919.
+ // DATA frame. In other case, we push wr onto the root, rather
+ // than creating a new priorityNode.
if wr.DataSize() > 0 {
panic("add DATA on non-open stream")
}
diff --git a/http2/writesched_priority_test.go b/http2/writesched_priority_test.go
index f2b535a..b579ef9 100644
--- a/http2/writesched_priority_test.go
+++ b/http2/writesched_priority_test.go
@@ -387,6 +387,29 @@
}
}
+// #49741 RST_STREAM and Control frames should have more priority than data
+// frames to avoid blocking streams caused by clients not able to drain the
+// queue.
+func TestPriorityRSTFrames(t *testing.T) {
+ ws := defaultPriorityWriteScheduler()
+ ws.OpenStream(1, OpenStreamOptions{})
+
+ sc := &serverConn{maxFrameSize: 16}
+ st1 := &stream{id: 1, sc: sc}
+
+ ws.Push(FrameWriteRequest{&writeData{1, make([]byte, 16), false}, st1, nil})
+ ws.Push(FrameWriteRequest{&writeData{1, make([]byte, 16), false}, st1, nil})
+ ws.Push(makeWriteRSTStream(1))
+ // No flow-control bytes available.
+ wr, ok := ws.Pop()
+ if !ok {
+ t.Fatalf("Pop should work for control frames and not be limited by flow control")
+ }
+ if _, ok := wr.write.(StreamError); !ok {
+ t.Fatal("expected RST stream frames first", wr)
+ }
+}
+
func TestPriorityPopFromLinearTree(t *testing.T) {
ws := defaultPriorityWriteScheduler()
ws.OpenStream(1, OpenStreamOptions{})
