quic: don't defer MAX_STREAMS frames indefinitely

Avoid a state where we can have a MAX_STREAMS frame to send,
but do not send the frame for an indefinite amount of time.

Conn.appendStreamFrames writes stream-related frames to
the current packet. It also handles removing streams
from the Conn when we no longer need to track their state.

Removing streams can affect the frames we want to send.
In particular, we may want to send a MAX_STREAMS to the
peer indicating that it can open more streams because
we've closed out some of the existing ones.

Add MAX_STREAMS after removing streams, to ensure we
pick up any changes to the sent value before adding it.

This case doesn't show up in  tests, because the test harness's
idleness detection causes the Conn's event loop to run and notice
the pending MAX_STREAMS frame. Changing tests to use
testing/synctest (a followup CL) causes the problem to
appear, because the event loop isn't run while the Conn
is idle.

Change-Id: Ia7394891317dae6ecfd529a9b3501ac082cb453e
Reviewed-on: https://go-review.googlesource.com/c/net/+/714481
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Damien Neil <[email protected]>
Reviewed-by: Nicholas Husin <[email protected]>
Reviewed-by: Nicholas Husin <[email protected]>

Author: neild

quic/conn_streams.go

@@ -283,19 +283,14 @@ func (c *Conn) appendStreamFrames(w *packetWriter, pnum packetNumber, pto bool)
 		return false
 	}
 
-	// MAX_STREAM_DATA
-	if !c.streams.remoteLimit[uniStream].appendFrame(w, uniStream, pnum, pto) {
-		return false
-	}
-	if !c.streams.remoteLimit[bidiStream].appendFrame(w, bidiStream, pnum, pto) {
-		return false
-	}
-
 	if pto {
 		return c.appendStreamFramesPTO(w, pnum)
 	}
 	if !c.streams.needSend.Load() {
-		return true
+		// If queueMeta includes newly-finished streams, we may extend the peer's
+		// stream limits. When there are no streams to process, add MAX_STREAMS
+		// frames here. Otherwise, wait until after we've processed queueMeta.
+		return c.appendMaxStreams(w, pnum, pto)
 	}
 	c.streams.sendMu.Lock()
 	defer c.streams.sendMu.Unlock()
@@ -354,6 +349,12 @@ func (c *Conn) appendStreamFrames(w *packetWriter, pnum packetNumber, pto bool)
 		// If so, put the stream back on a queue.
 		c.queueStreamForSendLocked(s, state)
 	}
+
+	// MAX_STREAMS (possibly triggered by finalization of remote streams above).
+	if !c.appendMaxStreams(w, pnum, pto) {
+		return false
+	}
+
 	// queueData contains streams with flow-controlled frames.
 	for c.streams.queueData.head != nil {
 		avail := c.streams.outflow.avail()
@@ -408,9 +409,12 @@ func (c *Conn) appendStreamFrames(w *packetWriter, pnum packetNumber, pto bool)
 // It returns true if no more frames need appending,
 // false if not everything fit in the current packet.
 func (c *Conn) appendStreamFramesPTO(w *packetWriter, pnum packetNumber) bool {
+	const pto = true
+	if !c.appendMaxStreams(w, pnum, pto) {
+		return false
+	}
 	c.streams.sendMu.Lock()
 	defer c.streams.sendMu.Unlock()
-	const pto = true
 	for _, ms := range c.streams.streams {
 		s := ms.s
 		if s == nil {
@@ -434,6 +438,16 @@ func (c *Conn) appendStreamFramesPTO(w *packetWriter, pnum packetNumber) bool {
 	return true
 }
 
+func (c *Conn) appendMaxStreams(w *packetWriter, pnum packetNumber, pto bool) bool {
+	if !c.streams.remoteLimit[uniStream].appendFrame(w, uniStream, pnum, pto) {
+		return false
+	}
+	if !c.streams.remoteLimit[bidiStream].appendFrame(w, bidiStream, pnum, pto) {
+		return false
+	}
+	return true
+}
+
 // A streamRing is a circular linked list of streams.
 type streamRing struct {
 	head *Stream