only set the loss detection alarm if packets are outstanding

internal/ackhandler/sent_packet_handler.go

@@ -301,12 +301,12 @@ func (h *sentPacketHandler) maybeUpdateRTT(largestAcked protocol.PacketNumber, a
 
 func (h *sentPacketHandler) updateLossDetectionAlarm() {
 	// Cancel the alarm if no packets are outstanding
-	if h.packetHistory.Len() == 0 {
+	if !h.packetHistory.HasOutstandingPackets() {
 		h.alarm = time.Time{}
 		return
 	}
 
-	if !h.handshakeComplete {
+	if h.packetHistory.HasOutstandingHandshakePackets() {
 		h.alarm = h.lastSentHandshakePacketTime.Add(h.computeHandshakeTimeout())
 	} else if !h.lossTime.IsZero() {
 		// Early retransmit timer or time loss detection.

internal/ackhandler/sent_packet_handler_test.go

@@ -782,6 +782,15 @@ var _ = Describe("SentPacketHandler", func() {
 		})
 	})
 
+	It("doesn't set an alarm if there are no outstanding packets", func() {
+		handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 10}))
+		handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 11}))
+		ack := &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 10, Largest: 11}}}
+		err := handler.ReceivedAck(ack, 1, protocol.EncryptionForwardSecure, time.Now())
+		Expect(err).ToNot(HaveOccurred())
+		Expect(handler.GetAlarmTimeout()).To(BeZero())
+	})
+
 	Context("TLPs", func() {
 		It("uses the RTT from RTT stats", func() {
 			rtt := 2 * time.Second
@@ -997,7 +1006,6 @@ var _ = Describe("SentPacketHandler", func() {
 			// Packet 1 should be considered lost (1+1/8) RTTs after it was sent.
 			Expect(handler.lossTime.IsZero()).To(BeFalse())
 			Expect(handler.lossTime.Sub(getPacket(1).SendTime)).To(Equal(time.Second * 9 / 8))
-			// Expect(time.Until(handler.GetAlarmTimeout())).To(BeNumerically("~", time.Hour*9/8, time.Minute))
 
 			err = handler.OnAlarm()
 			Expect(err).ToNot(HaveOccurred())
@@ -1016,13 +1024,11 @@ var _ = Describe("SentPacketHandler", func() {
 			now := time.Now()
 			sendTime := now.Add(-time.Minute)
 			lastHandshakePacketSendTime := now.Add(-30 * time.Second)
-			// send handshake packets: 1, 2, 4
-			// send a forward-secure packet: 3
+			// send handshake packets: 1, 3
+			// send a forward-secure packet: 2
 			handler.SentPacket(handshakePacket(&Packet{PacketNumber: 1, SendTime: sendTime}))
-			handler.SentPacket(handshakePacket(&Packet{PacketNumber: 2, SendTime: sendTime}))
-			handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 3, SendTime: sendTime}))
-			handler.SentPacket(handshakePacket(&Packet{PacketNumber: 4, SendTime: lastHandshakePacketSendTime}))
-			handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 5, SendTime: now}))
+			handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 2, SendTime: sendTime}))
+			handler.SentPacket(handshakePacket(&Packet{PacketNumber: 3, SendTime: sendTime}))
 
 			ack := &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 1, Largest: 1}}}
 			err := handler.ReceivedAck(ack, 1, protocol.EncryptionForwardSecure, now)
@@ -1030,18 +1036,15 @@ var _ = Describe("SentPacketHandler", func() {
 			Expect(handler.rttStats.SmoothedRTT()).To(Equal(time.Minute))
 			Expect(err).NotTo(HaveOccurred())
 			Expect(handler.lossTime.IsZero()).To(BeTrue())
-			Expect(handler.GetAlarmTimeout().Sub(lastHandshakePacketSendTime)).To(Equal(2 * time.Minute))
+			Expect(handler.GetAlarmTimeout().Sub(sendTime)).To(Equal(2 * time.Minute))
 
 			err = handler.OnAlarm()
 			Expect(err).ToNot(HaveOccurred())
 			p := handler.DequeuePacketForRetransmission()
 			Expect(p).ToNot(BeNil())
-			Expect(p.PacketNumber).To(Equal(protocol.PacketNumber(2)))
-			p = handler.DequeuePacketForRetransmission()
-			Expect(p).ToNot(BeNil())
-			Expect(p.PacketNumber).To(Equal(protocol.PacketNumber(4)))
-			Expect(getPacket(3)).ToNot(BeNil())
+			Expect(p.PacketNumber).To(Equal(protocol.PacketNumber(3)))
 			Expect(handler.handshakeCount).To(BeEquivalentTo(1))
+			handler.SentPacket(handshakePacket(&Packet{PacketNumber: 4, SendTime: lastHandshakePacketSendTime}))
 			// make sure the exponential backoff is used
 			Expect(handler.GetAlarmTimeout().Sub(lastHandshakePacketSendTime)).To(Equal(4 * time.Minute))
 		})

internal/ackhandler/sent_packet_history.go

@@ -10,6 +10,9 @@ type sentPacketHistory struct {
 	packetList *PacketList
 	packetMap  map[protocol.PacketNumber]*PacketElement
 
+	numOutstandingPackets          int
+	numOutstandingHandshakePackets int
+
 	firstOutstanding *PacketElement
 }
 
@@ -30,6 +33,12 @@ func (h *sentPacketHistory) sentPacketImpl(p *Packet) *PacketElement {
 	if h.firstOutstanding == nil {
 		h.firstOutstanding = el
 	}
+	if p.canBeRetransmitted {
+		h.numOutstandingPackets++
+		if p.EncryptionLevel < protocol.EncryptionForwardSecure {
+			h.numOutstandingHandshakePackets++
+		}
+	}
 	return el
 }
 
@@ -92,6 +101,18 @@ func (h *sentPacketHistory) MarkCannotBeRetransmitted(pn protocol.PacketNumber)
 	if !ok {
 		return fmt.Errorf("sent packet history: packet %d not found", pn)
 	}
+	if el.Value.canBeRetransmitted {
+		h.numOutstandingPackets--
+		if h.numOutstandingPackets < 0 {
+			panic("numOutstandingHandshakePackets negative")
+		}
+		if el.Value.EncryptionLevel < protocol.EncryptionForwardSecure {
+			h.numOutstandingHandshakePackets--
+			if h.numOutstandingHandshakePackets < 0 {
+				panic("numOutstandingHandshakePackets negative")
+			}
+		}
+	}
 	el.Value.canBeRetransmitted = false
 	if el == h.firstOutstanding {
 		h.readjustFirstOutstanding()
@@ -121,7 +142,27 @@ func (h *sentPacketHistory) Remove(p protocol.PacketNumber) error {
 	if el == h.firstOutstanding {
 		h.readjustFirstOutstanding()
 	}
+	if el.Value.canBeRetransmitted {
+		h.numOutstandingPackets--
+		if h.numOutstandingPackets < 0 {
+			panic("numOutstandingHandshakePackets negative")
+		}
+		if el.Value.EncryptionLevel < protocol.EncryptionForwardSecure {
+			h.numOutstandingHandshakePackets--
+			if h.numOutstandingHandshakePackets < 0 {
+				panic("numOutstandingHandshakePackets negative")
+			}
+		}
+	}
 	h.packetList.Remove(el)
 	delete(h.packetMap, p)
 	return nil
 }
+
+func (h *sentPacketHistory) HasOutstandingPackets() bool {
+	return h.numOutstandingPackets > 0
+}
+
+func (h *sentPacketHistory) HasOutstandingHandshakePackets() bool {
+	return h.numOutstandingHandshakePackets > 0
+}

internal/ackhandler/sent_packet_history_test.go

@@ -197,4 +197,101 @@ var _ = Describe("SentPacketHistory", func() {
 			Expect(hist.GetPacket(13).retransmissionOf).To(BeZero())
 		})
 	})
+
+	Context("outstanding packets", func() {
+		It("says if it has outstanding handshake packets", func() {
+			Expect(hist.HasOutstandingHandshakePackets()).To(BeFalse())
+			hist.SentPacket(&Packet{
+				EncryptionLevel:    protocol.EncryptionUnencrypted,
+				canBeRetransmitted: true,
+			})
+			Expect(hist.HasOutstandingHandshakePackets()).To(BeTrue())
+		})
+
+		It("says if it has outstanding packets", func() {
+			Expect(hist.HasOutstandingHandshakePackets()).To(BeFalse())
+			Expect(hist.HasOutstandingPackets()).To(BeFalse())
+			hist.SentPacket(&Packet{
+				EncryptionLevel:    protocol.EncryptionForwardSecure,
+				canBeRetransmitted: true,
+			})
+			Expect(hist.HasOutstandingHandshakePackets()).To(BeFalse())
+			Expect(hist.HasOutstandingPackets()).To(BeTrue())
+		})
+
+		It("doesn't consider non-retransmittable packets as outstanding", func() {
+			hist.SentPacket(&Packet{
+				EncryptionLevel: protocol.EncryptionUnencrypted,
+			})
+			Expect(hist.HasOutstandingHandshakePackets()).To(BeFalse())
+			Expect(hist.HasOutstandingPackets()).To(BeFalse())
+		})
+
+		It("accounts for deleted handshake packets", func() {
+			hist.SentPacket(&Packet{
+				PacketNumber:       5,
+				EncryptionLevel:    protocol.EncryptionSecure,
+				canBeRetransmitted: true,
+			})
+			Expect(hist.HasOutstandingHandshakePackets()).To(BeTrue())
+			err := hist.Remove(5)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(hist.HasOutstandingHandshakePackets()).To(BeFalse())
+		})
+
+		It("accounts for deleted packets", func() {
+			hist.SentPacket(&Packet{
+				PacketNumber:       10,
+				EncryptionLevel:    protocol.EncryptionForwardSecure,
+				canBeRetransmitted: true,
+			})
+			Expect(hist.HasOutstandingPackets()).To(BeTrue())
+			err := hist.Remove(10)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(hist.HasOutstandingPackets()).To(BeFalse())
+		})
+
+		It("doesn't count handshake packets marked as non-retransmittable", func() {
+			hist.SentPacket(&Packet{
+				PacketNumber:       5,
+				EncryptionLevel:    protocol.EncryptionUnencrypted,
+				canBeRetransmitted: true,
+			})
+			Expect(hist.HasOutstandingHandshakePackets()).To(BeTrue())
+			err := hist.MarkCannotBeRetransmitted(5)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(hist.HasOutstandingHandshakePackets()).To(BeFalse())
+		})
+
+		It("doesn't count packets marked as non-retransmittable", func() {
+			hist.SentPacket(&Packet{
+				PacketNumber:       10,
+				EncryptionLevel:    protocol.EncryptionForwardSecure,
+				canBeRetransmitted: true,
+			})
+			Expect(hist.HasOutstandingPackets()).To(BeTrue())
+			err := hist.MarkCannotBeRetransmitted(10)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(hist.HasOutstandingPackets()).To(BeFalse())
+		})
+
+		It("counts the number of packets", func() {
+			hist.SentPacket(&Packet{
+				PacketNumber:       10,
+				EncryptionLevel:    protocol.EncryptionForwardSecure,
+				canBeRetransmitted: true,
+			})
+			hist.SentPacket(&Packet{
+				PacketNumber:       11,
+				EncryptionLevel:    protocol.EncryptionForwardSecure,
+				canBeRetransmitted: true,
+			})
+			err := hist.Remove(11)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(hist.HasOutstandingPackets()).To(BeTrue())
+			err = hist.Remove(10)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(hist.HasOutstandingPackets()).To(BeFalse())
+		})
+	})
 })