ackhandler: generalize check for missing packets below threshold (#5260)

* ackhandler: check for missing packets below reordering treshold

No functional change expected.

With the Acknowledgement Frequency extension, the reordering threshold
will become configurable. With this change, it will be easy to use the
peer-requested value instead of the predefined constant.

* improve documentation

* call HighestMissingUpTo in randomized test

internal/ackhandler/received_packet_history.go

@@ -23,7 +23,9 @@ type receivedPacketHistory struct {
 }
 
 func newReceivedPacketHistory() *receivedPacketHistory {
-	return &receivedPacketHistory{}
+	return &receivedPacketHistory{
+		deletedBelow: protocol.InvalidPacketNumber,
+	}
 }
 
 // ReceivedPacket registers a packet with PacketNumber p and updates the ranges
@@ -115,13 +117,26 @@ func (h *receivedPacketHistory) AppendAckRanges(ackRanges []wire.AckRange) []wir
 	return ackRanges
 }
 
-func (h *receivedPacketHistory) GetHighestAckRange() wire.AckRange {
-	ackRange := wire.AckRange{}
-	if len(h.ranges) > 0 {
-		ackRange.Smallest = h.ranges[len(h.ranges)-1].Start
-		ackRange.Largest = h.ranges[len(h.ranges)-1].End
+func (h *receivedPacketHistory) HighestMissingUpTo(p protocol.PacketNumber) protocol.PacketNumber {
+	if len(h.ranges) == 0 || (h.deletedBelow != protocol.InvalidPacketNumber && p < h.deletedBelow) {
+		return protocol.InvalidPacketNumber
 	}
-	return ackRange
+	p = min(h.ranges[len(h.ranges)-1].End, p)
+	for i := len(h.ranges) - 1; i >= 0; i-- {
+		r := h.ranges[i]
+		if p >= r.Start && p <= r.End { // p is contained in this range
+			highest := r.Start - 1 // highest packet in the gap before this range
+			if h.deletedBelow != protocol.InvalidPacketNumber && highest < h.deletedBelow {
+				return protocol.InvalidPacketNumber
+			}
+			return highest
+		}
+		if i >= 1 && p > h.ranges[i-1].End && p <= r.Start {
+			// p is in the gap between the previous range and this range
+			return p
+		}
+	}
+	return p
 }
 
 func (h *receivedPacketHistory) IsPotentiallyDuplicate(p protocol.PacketNumber) bool {

internal/ackhandler/received_packet_history_test.go

@@ -37,15 +37,19 @@ func TestReceivedPacketHistorySingleRange(t *testing.T) {
 
 func TestReceivedPacketHistoryRanges(t *testing.T) {
 	hist := newReceivedPacketHistory()
-	require.Zero(t, hist.GetHighestAckRange())
+	require.Equal(t, protocol.InvalidPacketNumber, hist.HighestMissingUpTo(1000))
 
 	require.True(t, hist.ReceivedPacket(4))
+	require.Equal(t, protocol.PacketNumber(3), hist.HighestMissingUpTo(1000))
+	require.Equal(t, protocol.PacketNumber(3), hist.HighestMissingUpTo(4))
+	require.Equal(t, protocol.PacketNumber(3), hist.HighestMissingUpTo(3))
+	require.Equal(t, protocol.PacketNumber(2), hist.HighestMissingUpTo(2))
 	require.True(t, hist.ReceivedPacket(10))
+	require.Equal(t, protocol.PacketNumber(9), hist.HighestMissingUpTo(1000))
 	require.Equal(t, []wire.AckRange{
 		{Smallest: 10, Largest: 10},
 		{Smallest: 4, Largest: 4},
 	}, hist.AppendAckRanges(nil))
-	require.Equal(t, wire.AckRange{Smallest: 10, Largest: 10}, hist.GetHighestAckRange())
 
 	// create a new range in the middle
 	require.True(t, hist.ReceivedPacket(7))
@@ -118,7 +122,10 @@ func TestReceivedPacketHistoryDeleteBelow(t *testing.T) {
 	require.True(t, hist.ReceivedPacket(6))
 	require.True(t, hist.ReceivedPacket(10))
 
+	require.Equal(t, protocol.PacketNumber(3), hist.HighestMissingUpTo(6))
 	hist.DeleteBelow(6)
+	require.Equal(t, protocol.InvalidPacketNumber, hist.HighestMissingUpTo(6))
+	require.Equal(t, protocol.PacketNumber(9), hist.HighestMissingUpTo(10))
 	require.Equal(t, []wire.AckRange{
 		{Smallest: 10, Largest: 10},
 		{Smallest: 6, Largest: 6},
@@ -172,17 +179,17 @@ func TestReceivedPacketHistoryDuplicateDetection(t *testing.T) {
 
 func TestReceivedPacketHistoryRandomized(t *testing.T) {
 	hist := newReceivedPacketHistory()
-	packets := make(map[protocol.PacketNumber]int)
+	packets := make(map[protocol.PacketNumber]struct{})
 	const num = 2 * protocol.MaxNumAckRanges
 	numLostPackets := rand.IntN(protocol.MaxNumAckRanges)
 	numRcvdPackets := num - numLostPackets
 
-	for i := 0; i < num; i++ {
-		packets[protocol.PacketNumber(i)] = 0
+	for i := range num {
+		packets[protocol.PacketNumber(i)] = struct{}{}
 	}
 	lostPackets := make([]protocol.PacketNumber, 0, numLostPackets)
 	for len(lostPackets) < numLostPackets {
-		p := protocol.PacketNumber(rand.IntN(num))
+		p := protocol.PacketNumber(rand.IntN(num - 1)) // lose a random packet, but not the last one
 		if _, ok := packets[p]; ok {
 			lostPackets = append(lostPackets, p)
 			delete(packets, p)
@@ -216,6 +223,28 @@ func TestReceivedPacketHistoryRandomized(t *testing.T) {
 		}
 	}
 	require.Equal(t, numRcvdPackets, counter)
+
+	deletedBelow := protocol.PacketNumber(rand.IntN(num * 2 / 3))
+	t.Logf("Deleting below %d", deletedBelow)
+	hist.DeleteBelow(deletedBelow)
+	for pn := range protocol.PacketNumber(num) {
+		if pn < deletedBelow {
+			require.Equal(t, protocol.InvalidPacketNumber, hist.HighestMissingUpTo(pn))
+			continue
+		}
+		expected := protocol.InvalidPacketNumber
+		for _, lost := range lostPackets {
+			if lost < deletedBelow {
+				continue
+			}
+			if lost > pn {
+				break
+			}
+			expected = lost
+		}
+		hm := hist.HighestMissingUpTo(pn)
+		require.Equalf(t, expected, hm, "highest missing up to %d: %d", pn, hm)
+	}
 }
 
 func BenchmarkHistoryReceiveSequentialPackets(b *testing.B) {

internal/ackhandler/received_packet_tracker.go

@@ -9,6 +9,8 @@ import (
 	"github.com/quic-go/quic-go/internal/wire"
 )
 
+const reorderingThreshold = 1
+
 // The receivedPacketTracker tracks packets for the Initial and Handshake packet number space.
 // Every received packet is acknowledged immediately.
 type receivedPacketTracker struct {
@@ -150,11 +152,18 @@ func (h *appDataReceivedPacketTracker) isMissing(p protocol.PacketNumber) bool {
 }
 
 func (h *appDataReceivedPacketTracker) hasNewMissingPackets() bool {
-	if h.lastAck == nil {
+	if h.largestObserved < reorderingThreshold {
 		return false
 	}
-	highestRange := h.packetHistory.GetHighestAckRange()
-	return highestRange.Smallest > h.lastAck.LargestAcked()+1 && highestRange.Len() == 1
+	highestMissing := h.packetHistory.HighestMissingUpTo(h.largestObserved - reorderingThreshold)
+	if highestMissing == protocol.InvalidPacketNumber {
+		return false
+	}
+	if highestMissing < h.lastAck.LargestAcked() {
+		// the packet was already reported missing in the last ACK
+		return false
+	}
+	return highestMissing > h.lastAck.LargestAcked()-reorderingThreshold
 }
 
 func (h *appDataReceivedPacketTracker) shouldQueueACK(pn protocol.PacketNumber, ecn protocol.ECN, wasMissing bool) bool {

internal/ackhandler/received_packet_tracker_test.go

@@ -54,11 +54,11 @@ func TestAppDataReceivedPacketTrackerECN(t *testing.T) {
 
 	require.NoError(t, tr.ReceivedPacket(0, protocol.ECT0, time.Now(), true))
 	pn := protocol.PacketNumber(1)
-	for i := 0; i < 2; i++ {
+	for range 2 {
 		require.NoError(t, tr.ReceivedPacket(pn, protocol.ECT1, time.Now(), true))
 		pn++
 	}
-	for i := 0; i < 3; i++ {
+	for range 3 {
 		require.NoError(t, tr.ReceivedPacket(pn, protocol.ECNCE, time.Now(), true))
 		pn++
 	}
@@ -123,18 +123,30 @@ func TestAppDataReceivedPacketTrackerQueuesECNCE(t *testing.T) {
 func TestAppDataReceivedPacketTrackerMissingPackets(t *testing.T) {
 	tr := newAppDataReceivedPacketTracker(utils.DefaultLogger)
 
+	now := time.Now()
 	// the first packet is always acknowledged
-	require.NoError(t, tr.ReceivedPacket(0, protocol.ECNNon, time.Now(), true))
-	require.NotNil(t, tr.GetAckFrame(time.Now(), true))
+	require.NoError(t, tr.ReceivedPacket(0, protocol.ECNNon, now, true))
+	require.NotNil(t, tr.GetAckFrame(now, true))
 
-	require.NoError(t, tr.ReceivedPacket(5, protocol.ECNNon, time.Now(), true))
-	ack := tr.GetAckFrame(time.Now(), true) // ACK: 0 and 5, missing: 1, 2, 3, 4
+	require.NoError(t, tr.ReceivedPacket(5, protocol.ECNNon, now, true))
+	ack := tr.GetAckFrame(now, true) // ACK: 0 and 5, missing: 1, 2, 3, 4
 	require.NotNil(t, ack)
 	require.Equal(t, []wire.AckRange{{Smallest: 5, Largest: 5}, {Smallest: 0, Largest: 0}}, ack.AckRanges)
 
 	// now receive one of the missing packets
-	require.NoError(t, tr.ReceivedPacket(3, protocol.ECNNon, time.Now(), true))
-	require.NotNil(t, tr.GetAckFrame(time.Now(), true))
+	require.NoError(t, tr.ReceivedPacket(3, protocol.ECNNon, now, true))
+	ack = tr.GetAckFrame(now, true)
+	require.NotNil(t, ack)
+	require.Equal(t, []wire.AckRange{
+		{Smallest: 5, Largest: 5},
+		{Smallest: 3, Largest: 3},
+		{Smallest: 0, Largest: 0},
+	}, ack.AckRanges)
+
+	require.NoError(t, tr.ReceivedPacket(6, protocol.ECNNon, now, true))
+	require.Nil(t, tr.GetAckFrame(now, true))
+	require.NoError(t, tr.ReceivedPacket(8, protocol.ECNNon, now, true))
+	require.NotNil(t, tr.GetAckFrame(now, true))
 }
 
 func TestAppDataReceivedPacketTrackerDelayTime(t *testing.T) {