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 <[email protected]>
Change-Id: Ie4462603380039f7aba8f701fe810d1d84376efa
Reviewed-on: https://go-review.googlesource.com/c/net/+/382014
Reviewed-by: Ian Lance Taylor <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
Run-TryBot: Dave Cheney <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

http2/writesched_priority.go

@@ -383,16 +383,15 @@ func (ws *priorityWriteScheduler) AdjustStream(streamID uint32, priority Priorit
 
 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")
 			}

http2/writesched_priority_test.go

@@ -387,6 +387,29 @@ func TestPriorityPopFrom533Tree(t *testing.T) {
 	}
 }
 
+// #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{})