implement packet send modes to determine what kind of packets are sent

internal/ackhandler/interfaces.go

@@ -14,11 +14,8 @@ type SentPacketHandler interface {
 	ReceivedAck(ackFrame *wire.AckFrame, withPacketNumber protocol.PacketNumber, encLevel protocol.EncryptionLevel, recvTime time.Time) error
 	SetHandshakeComplete()
 
-	// SendingAllowed says if a packet can be sent.
-	// Sending packets might not be possible because:
-	// * we're congestion limited
-	// * we're tracking the maximum number of sent packets
-	SendingAllowed() bool
+	// The SendMode determines if and what kind of packets can be sent.
+	SendMode() SendMode
 	// TimeUntilSend is the time when the next packet should be sent.
 	// It is used for pacing packets.
 	TimeUntilSend() time.Time

internal/ackhandler/send_mode.go

@@ -0,0 +1,32 @@
+package ackhandler
+
+import "fmt"
+
+// The SendMode says what kind of packets can be sent.
+type SendMode uint8
+
+const (
+	// SendNone means that no packets should be sent
+	SendNone SendMode = iota
+	// SendAck means an ACK-only packet should be sent
+	SendAck
+	// SendRetransmission means that retransmissions should be sent
+	SendRetransmission
+	// SendAny packet should be sent
+	SendAny
+)
+
+func (s SendMode) String() string {
+	switch s {
+	case SendNone:
+		return "none"
+	case SendAck:
+		return "ack"
+	case SendRetransmission:
+		return "retransmission"
+	case SendAny:
+		return "any"
+	default:
+		return fmt.Sprintf("invalid send mode: %d", s)
+	}
+}

internal/ackhandler/send_mode_test.go

@@ -0,0 +1,16 @@
+package ackhandler
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("Send Mode", func() {
+	It("has a string representation", func() {
+		Expect(SendNone.String()).To(Equal("none"))
+		Expect(SendAny.String()).To(Equal("any"))
+		Expect(SendAck.String()).To(Equal("ack"))
+		Expect(SendRetransmission.String()).To(Equal("retransmission"))
+		Expect(SendMode(123).String()).To(Equal("invalid send mode: 123"))
+	})
+})

internal/ackhandler/sent_packet_handler.go

@@ -357,18 +357,31 @@ func (h *sentPacketHandler) GetStopWaitingFrame(force bool) *wire.StopWaitingFra
 	return h.stopWaitingManager.GetStopWaitingFrame(force)
 }
 
-func (h *sentPacketHandler) SendingAllowed() bool {
-	cwnd := h.congestion.GetCongestionWindow()
-	congestionLimited := h.bytesInFlight > cwnd
-	maxTrackedLimited := protocol.PacketNumber(len(h.retransmissionQueue)+h.packetHistory.Len()) >= protocol.MaxTrackedSentPackets
-	if congestionLimited {
-		utils.Debugf("Congestion limited: bytes in flight %d, window %d", h.bytesInFlight, cwnd)
+func (h *sentPacketHandler) SendMode() SendMode {
+	numTrackedPackets := len(h.retransmissionQueue) + h.packetHistory.Len()
+
+	// Don't send any packets if we're keeping track of the maximum number of packets.
+	// Note that since MaxOutstandingSentPackets is smaller than MaxTrackedSentPackets,
+	// we will stop sending out new data when reaching MaxOutstandingSentPackets,
+	// but still allow sending of retransmissions and ACKs.
+	if numTrackedPackets >= protocol.MaxTrackedSentPackets {
+		utils.Debugf("Limited by the number of tracked packets: tracking %d packets, maximum %d", numTrackedPackets, protocol.MaxTrackedSentPackets)
+		return SendNone
 	}
-	// Workaround for #555:
-	// Always allow sending of retransmissions. This should probably be limited
-	// to RTOs, but we currently don't have a nice way of distinguishing them.
-	haveRetransmissions := len(h.retransmissionQueue) > 0
-	return !maxTrackedLimited && (!congestionLimited || haveRetransmissions)
+	// Send retransmissions first, if there are any.
+	if len(h.retransmissionQueue) > 0 {
+		return SendRetransmission
+	}
+	// Only send ACKs if we're congestion limited.
+	if cwnd := h.congestion.GetCongestionWindow(); h.bytesInFlight > cwnd {
+		utils.Debugf("Congestion limited: bytes in flight %d, window %d", h.bytesInFlight, cwnd)
+		return SendAck
+	}
+	if numTrackedPackets >= protocol.MaxOutstandingSentPackets {
+		utils.Debugf("Max outstanding limited: tracking %d packets, maximum: %d", numTrackedPackets, protocol.MaxOutstandingSentPackets)
+		return SendAck
+	}
+	return SendAny
 }
 
 func (h *sentPacketHandler) TimeUntilSend() time.Time {

internal/ackhandler/sent_packet_handler_test.go

@@ -694,28 +694,39 @@ var _ = Describe("SentPacketHandler", func() {
 			handler.OnAlarm() // RTO, meaning 2 lost packets
 		})
 
-		It("allows or denies sending based on congestion", func() {
+		It("only allows sending of ACKs when congestion limited", func() {
 			handler.bytesInFlight = 100
 			cong.EXPECT().GetCongestionWindow().Return(protocol.ByteCount(200))
-			Expect(handler.SendingAllowed()).To(BeTrue())
+			Expect(handler.SendMode()).To(Equal(SendAny))
 			cong.EXPECT().GetCongestionWindow().Return(protocol.ByteCount(75))
-			Expect(handler.SendingAllowed()).To(BeFalse())
+			Expect(handler.SendMode()).To(Equal(SendAck))
 		})
 
-		It("allows or denies sending based on the number of tracked packets", func() {
-			cong.EXPECT().GetCongestionWindow().Times(2)
-			Expect(handler.SendingAllowed()).To(BeTrue())
-			handler.retransmissionQueue = make([]*Packet, protocol.MaxTrackedSentPackets)
-			Expect(handler.SendingAllowed()).To(BeFalse())
+		It("only allows sending of ACKs when we're keeping track of MaxOutstandingSentPackets packets", func() {
+			cong.EXPECT().GetCongestionWindow().Return(protocol.MaxByteCount).AnyTimes()
+			cong.EXPECT().TimeUntilSend(gomock.Any()).AnyTimes()
+			cong.EXPECT().OnPacketSent(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()
+			for i := 1; i < protocol.MaxOutstandingSentPackets; i++ {
+				handler.SentPacket(retransmittablePacket(protocol.PacketNumber(i)))
+				Expect(handler.SendMode()).To(Equal(SendAny))
+			}
+			handler.SentPacket(retransmittablePacket(protocol.MaxOutstandingSentPackets))
+			Expect(handler.SendMode()).To(Equal(SendAck))
 		})
 
-		It("allows sending if there are retransmisisons outstanding", func() {
-			cong.EXPECT().GetCongestionWindow().Times(2)
-			handler.bytesInFlight = 100
-			Expect(handler.retransmissionQueue).To(BeEmpty())
-			Expect(handler.SendingAllowed()).To(BeFalse())
+		It("allows sending retransmissions", func() {
+			// note that we don't EXPECT a call to GetCongestionWindow
+			// that means retransmissions are sent without considering the congestion window
 			handler.retransmissionQueue = []*Packet{{PacketNumber: 3}}
-			Expect(handler.SendingAllowed()).To(BeTrue())
+			Expect(handler.SendMode()).To(Equal(SendRetransmission))
+		})
+
+		It("allow retransmissions, if we're keeping track of between MaxOutstandingSentPackets and MaxTrackedSentPackets packets", func() {
+			Expect(protocol.MaxOutstandingSentPackets).To(BeNumerically("<", protocol.MaxTrackedSentPackets))
+			handler.retransmissionQueue = make([]*Packet, protocol.MaxOutstandingSentPackets+10)
+			Expect(handler.SendMode()).To(Equal(SendRetransmission))
+			handler.retransmissionQueue = make([]*Packet, protocol.MaxTrackedSentPackets)
+			Expect(handler.SendMode()).To(Equal(SendNone))
 		})
 
 		It("gets the pacing delay", func() {