don't retransmit a packet after receiving a belated ACK

fixes #139

ackhandler/interfaces.go

@@ -12,7 +12,7 @@ type SentPacketHandler interface {
 	SentPacket(packet *Packet) error
 	ReceivedAck(ackFrame *frames.AckFrame) error
 
-	HasPacketForRetransmission() bool
+	ProbablyHasPacketForRetransmission() bool
 	DequeuePacketForRetransmission() (packet *Packet)
 
 	BytesInFlight() protocol.ByteCount

ackhandler/sent_packet_handler.go

@@ -250,25 +250,36 @@ func (h *sentPacketHandler) ReceivedAck(ackFrame *frames.AckFrame) error {
 	return nil
 }
 
-func (h *sentPacketHandler) HasPacketForRetransmission() bool {
+// ProbablyHasPacketForRetransmission returns if there is a packet queued for retransmission
+// There is one case where it gets the answer wrong:
+// if a packet has already been queued for retransmission, but a belated ACK is received for this packet, this function will return true, although the packet will not be returend for retransmission by DequeuePacketForRetransmission()
+func (h *sentPacketHandler) ProbablyHasPacketForRetransmission() bool {
 	h.maybeQueuePacketsRTO()
 
-	if len(h.retransmissionQueue) > 0 {
-		return true
-	}
-	return false
+	return len(h.retransmissionQueue) > 0
 }
 
 func (h *sentPacketHandler) DequeuePacketForRetransmission() (packet *Packet) {
-	if !h.HasPacketForRetransmission() {
+	if !h.ProbablyHasPacketForRetransmission() {
 		return nil
 	}
 
-	queueLen := len(h.retransmissionQueue)
-	// packets are usually NACKed in descending order. So use the slice as a stack
-	packet = h.retransmissionQueue[queueLen-1]
-	h.retransmissionQueue = h.retransmissionQueue[:queueLen-1]
-	return packet
+	for len(h.retransmissionQueue) > 0 {
+		queueLen := len(h.retransmissionQueue)
+		// packets are usually NACKed in descending order. So use the slice as a stack
+		packet = h.retransmissionQueue[queueLen-1]
+		h.retransmissionQueue = h.retransmissionQueue[:queueLen-1]
+
+		// check if the packet was ACKed after it was already queued for retransmission
+		// if so, it doesn't exist in the packetHistory anymore. Skip it then
+		_, ok := h.packetHistory[packet.PacketNumber]
+		if !ok {
+			continue
+		}
+		return packet
+	}
+
+	return nil
 }
 
 func (h *sentPacketHandler) BytesInFlight() protocol.ByteCount {

ackhandler/sent_packet_handler_test.go

@@ -439,7 +439,7 @@ var _ = Describe("SentPacketHandler", func() {
 				_, err := handler.nackPacket(2)
 				Expect(err).ToNot(HaveOccurred())
 			}
-			Expect(handler.HasPacketForRetransmission()).To(BeFalse())
+			Expect(handler.ProbablyHasPacketForRetransmission()).To(BeFalse())
 			Expect(handler.DequeuePacketForRetransmission()).To(BeNil())
 		})
 
@@ -448,7 +448,7 @@ var _ = Describe("SentPacketHandler", func() {
 				_, err := handler.nackPacket(2)
 				Expect(err).ToNot(HaveOccurred())
 			}
-			Expect(handler.HasPacketForRetransmission()).To(BeTrue())
+			Expect(handler.ProbablyHasPacketForRetransmission()).To(BeTrue())
 			Expect(handler.retransmissionQueue).To(HaveLen(1))
 			Expect(handler.retransmissionQueue[0].PacketNumber).To(Equal(protocol.PacketNumber(2)))
 		})
@@ -507,6 +507,19 @@ var _ = Describe("SentPacketHandler", func() {
 			handler.nackPacket(3) // this is the second NACK for this packet
 			Expect(handler.highestInOrderAckedPacketNumber).To(Equal(protocol.PacketNumber(2)))
 		})
+
+		It("does not retransmit a packet if a belated was received", func() {
+			// lose packet by NACKing it often enough
+			for i := uint8(0); i < protocol.RetransmissionThreshold+1; i++ {
+				_, err := handler.nackPacket(2)
+				Expect(err).ToNot(HaveOccurred())
+			}
+			// this is the belated ACK
+			handler.ackPacket(2)
+			// this is the edge case where ProbablyHasPacketForRetransmission() get's it wrong: it says there's probably a packet for retransmission, but actually there isn't
+			Expect(handler.ProbablyHasPacketForRetransmission()).To(BeTrue())
+			Expect(handler.DequeuePacketForRetransmission()).To(BeNil())
+		})
 	})
 
 	Context("calculating bytes in flight", func() {
@@ -701,7 +714,7 @@ var _ = Describe("SentPacketHandler", func() {
 			err := handler.SentPacket(p)
 			Expect(err).NotTo(HaveOccurred())
 			handler.lastSentPacketTime = time.Now().Add(-time.Second)
-			Expect(handler.HasPacketForRetransmission()).To(BeTrue())
+			Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil())
 		})
 
 		It("works with DequeuePacketForRetransmission", func() {

packet_packer_test.go

@@ -18,7 +18,7 @@ type mockSentPacketHandler struct{}
 func (h *mockSentPacketHandler) SentPacket(packet *ackhandler.Packet) error         { return nil }
 func (h *mockSentPacketHandler) ReceivedAck(ackFrame *frames.AckFrame) error        { return nil }
 func (h *mockSentPacketHandler) DequeuePacketForRetransmission() *ackhandler.Packet { return nil }
-func (h *mockSentPacketHandler) HasPacketForRetransmission() bool                   { return false }
+func (h *mockSentPacketHandler) ProbablyHasPacketForRetransmission() bool           { return false }
 func (h *mockSentPacketHandler) BytesInFlight() protocol.ByteCount                  { return 0 }
 func (h *mockSentPacketHandler) GetLargestObserved() protocol.PacketNumber          { return 1 }
 func (h *mockSentPacketHandler) CongestionAllowsSending() bool                      { panic("not implemented") }

session.go

@@ -420,7 +420,8 @@ func (s *Session) maybeSendPacket() error {
 	}
 
 	// always send out retransmissions immediately. No need to check the size of the packet
-	if s.sentPacketHandler.HasPacketForRetransmission() {
+	// in the edge cases where a belated ACK was received for a packet that was already queued for retransmission, we might send out a small packet. However, this shouldn't happen very often
+	if s.sentPacketHandler.ProbablyHasPacketForRetransmission() {
 		return s.sendPacket()
 	}