diff --git a/internal/ackhandler/received_packet_history.go b/internal/ackhandler/received_packet_history.go index f9feae1db..091e7fc11 100644 --- a/internal/ackhandler/received_packet_history.go +++ b/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 { diff --git a/internal/ackhandler/received_packet_history_test.go b/internal/ackhandler/received_packet_history_test.go index b6f54167f..f53013735 100644 --- a/internal/ackhandler/received_packet_history_test.go +++ b/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) { diff --git a/internal/ackhandler/received_packet_tracker.go b/internal/ackhandler/received_packet_tracker.go index 291a8f122..32ec8a73c 100644 --- a/internal/ackhandler/received_packet_tracker.go +++ 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 { diff --git a/internal/ackhandler/received_packet_tracker_test.go b/internal/ackhandler/received_packet_tracker_test.go index 7c72dc643..ddda9d439 100644 --- a/internal/ackhandler/received_packet_tracker_test.go +++ b/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) {