only save timestamp of largest observed packet in ReceivedPacketHandler

fixes #336

ackhandler/received_packet_handler.go

@@ -6,23 +6,16 @@ import (
 
 	"github.com/lucas-clemente/quic-go/frames"
 	"github.com/lucas-clemente/quic-go/protocol"
-	"github.com/lucas-clemente/quic-go/qerr"
-	"github.com/lucas-clemente/quic-go/utils"
 )
 
 var (
 	// ErrDuplicatePacket occurres when a duplicate packet is received
 	ErrDuplicatePacket = errors.New("ReceivedPacketHandler: Duplicate Packet")
-	// ErrMapAccess occurs when a NACK contains invalid NACK ranges
-	ErrMapAccess = qerr.Error(qerr.InvalidAckData, "Packet does not exist in PacketHistory")
 	// ErrPacketSmallerThanLastStopWaiting occurs when a packet arrives with a packet number smaller than the largest LeastUnacked of a StopWaitingFrame. If this error occurs, the packet should be ignored
 	ErrPacketSmallerThanLastStopWaiting = errors.New("ReceivedPacketHandler: Packet number smaller than highest StopWaiting")
 )
 
-var (
-	errInvalidPacketNumber               = errors.New("ReceivedPacketHandler: Invalid packet number")
-	errTooManyOutstandingReceivedPackets = qerr.Error(qerr.TooManyOutstandingReceivedPackets, "Too many outstanding received packets")
-)
+var errInvalidPacketNumber = errors.New("ReceivedPacketHandler: Invalid packet number")
 
 type receivedPacketHandler struct {
 	largestObserved    protocol.PacketNumber
@@ -32,14 +25,12 @@ type receivedPacketHandler struct {
 
 	packetHistory *receivedPacketHistory
 
-	receivedTimes         map[protocol.PacketNumber]time.Time
-	lowestInReceivedTimes protocol.PacketNumber
+	largestObservedReceivedTime time.Time
 }
 
 // NewReceivedPacketHandler creates a new receivedPacketHandler
 func NewReceivedPacketHandler() ReceivedPacketHandler {
 	return &receivedPacketHandler{
-		receivedTimes: make(map[protocol.PacketNumber]time.Time),
 		packetHistory: newReceivedPacketHistory(),
 	}
 }
@@ -69,12 +60,7 @@ func (h *receivedPacketHandler) ReceivedPacket(packetNumber protocol.PacketNumbe
 
 	if packetNumber > h.largestObserved {
 		h.largestObserved = packetNumber
-	}
-
-	h.receivedTimes[packetNumber] = time.Now()
-
-	if len(h.receivedTimes) > protocol.MaxTrackedReceivedPackets {
-		return errTooManyOutstandingReceivedPackets
+		h.largestObservedReceivedTime = time.Now()
 	}
 
 	return nil
@@ -87,10 +73,8 @@ func (h *receivedPacketHandler) ReceivedStopWaiting(f *frames.StopWaitingFrame)
 	}
 
 	h.ignorePacketsBelow = f.LeastUnacked - 1
-	h.garbageCollectReceivedTimes()
 
 	h.packetHistory.DeleteBelow(f.LeastUnacked)
-
 	return nil
 }
 
@@ -107,16 +91,11 @@ func (h *receivedPacketHandler) GetAckFrame(dequeue bool) (*frames.AckFrame, err
 		return h.currentAckFrame, nil
 	}
 
-	packetReceivedTime, ok := h.receivedTimes[h.largestObserved]
-	if !ok {
-		return nil, ErrMapAccess
-	}
-
 	ackRanges := h.packetHistory.GetAckRanges()
 	h.currentAckFrame = &frames.AckFrame{
 		LargestAcked:       h.largestObserved,
 		LowestAcked:        ackRanges[len(ackRanges)-1].FirstPacketNumber,
-		PacketReceivedTime: packetReceivedTime,
+		PacketReceivedTime: h.largestObservedReceivedTime,
 	}
 
 	if len(ackRanges) > 1 {
@@ -125,13 +104,3 @@ func (h *receivedPacketHandler) GetAckFrame(dequeue bool) (*frames.AckFrame, err
 
 	return h.currentAckFrame, nil
 }
-
-func (h *receivedPacketHandler) garbageCollectReceivedTimes() {
-	// the highest element in the receivedTimes map is the largest observed packet
-	for i := h.lowestInReceivedTimes; i <= utils.MinPacketNumber(h.ignorePacketsBelow, h.largestObserved); i++ {
-		delete(h.receivedTimes, i)
-	}
-	if h.ignorePacketsBelow > h.lowestInReceivedTimes {
-		h.lowestInReceivedTimes = h.ignorePacketsBelow + 1
-	}
-}

ackhandler/received_packet_handler_test.go

@@ -23,13 +23,10 @@ var _ = Describe("receivedPacketHandler", func() {
 		It("handles a packet that arrives late", func() {
 			err := handler.ReceivedPacket(protocol.PacketNumber(1))
 			Expect(err).ToNot(HaveOccurred())
-			Expect(handler.receivedTimes).To(HaveKey(protocol.PacketNumber(1)))
 			err = handler.ReceivedPacket(protocol.PacketNumber(3))
 			Expect(err).ToNot(HaveOccurred())
-			Expect(handler.receivedTimes).To(HaveKey(protocol.PacketNumber(3)))
 			err = handler.ReceivedPacket(protocol.PacketNumber(2))
 			Expect(err).ToNot(HaveOccurred())
-			Expect(handler.receivedTimes).To(HaveKey(protocol.PacketNumber(2)))
 		})
 
 		It("rejects packets with packet number 0", func() {
@@ -67,8 +64,26 @@ var _ = Describe("receivedPacketHandler", func() {
 		It("saves the time when each packet arrived", func() {
 			err := handler.ReceivedPacket(protocol.PacketNumber(3))
 			Expect(err).ToNot(HaveOccurred())
-			Expect(handler.receivedTimes).To(HaveKey(protocol.PacketNumber(3)))
-			Expect(handler.receivedTimes[3]).To(BeTemporally("~", time.Now(), 10*time.Millisecond))
+			Expect(handler.largestObservedReceivedTime).To(BeTemporally("~", time.Now(), 10*time.Millisecond))
+		})
+
+		It("updates the largestObserved and the largestObservedReceivedTime", func() {
+			handler.largestObserved = 3
+			handler.largestObservedReceivedTime = time.Now().Add(-1 * time.Second)
+			err := handler.ReceivedPacket(5)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(handler.largestObserved).To(Equal(protocol.PacketNumber(5)))
+			Expect(handler.largestObservedReceivedTime).To(BeTemporally("~", time.Now(), 10*time.Millisecond))
+		})
+
+		It("doesn't update the largestObserved and the largestObservedReceivedTime for a belated packet", func() {
+			timestamp := time.Now().Add(-1 * time.Second)
+			handler.largestObserved = 5
+			handler.largestObservedReceivedTime = timestamp
+			err := handler.ReceivedPacket(4)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(handler.largestObserved).To(Equal(protocol.PacketNumber(5)))
+			Expect(handler.largestObservedReceivedTime).To(Equal(timestamp))
 		})
 
 		It("doesn't store more than MaxTrackedReceivedPackets packets", func() {
@@ -252,61 +267,4 @@ var _ = Describe("receivedPacketHandler", func() {
 			Expect(ack.HasMissingRanges()).To(BeFalse())
 		})
 	})
-
-	Context("Garbage Collector", func() {
-		It("garbage collects receivedTimes after receiving a StopWaiting, if there are no missing packets", func() {
-			for i := 1; i <= 4; i++ {
-				err := handler.ReceivedPacket(protocol.PacketNumber(i))
-				Expect(err).ToNot(HaveOccurred())
-			}
-			err := handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: 3})
-			Expect(err).ToNot(HaveOccurred())
-			Expect(handler.receivedTimes).ToNot(HaveKey(protocol.PacketNumber(1)))
-			Expect(handler.receivedTimes).ToNot(HaveKey(protocol.PacketNumber(2)))
-			Expect(handler.receivedTimes).To(HaveKey(protocol.PacketNumber(3)))
-			Expect(handler.receivedTimes).To(HaveKey(protocol.PacketNumber(4)))
-			Expect(handler.lowestInReceivedTimes).To(Equal(protocol.PacketNumber(3)))
-		})
-
-		It("garbage collects the receivedTimes after receiving multiple StopWaitings", func() {
-			for i := 1; i <= 9; i++ {
-				err := handler.ReceivedPacket(protocol.PacketNumber(i))
-				Expect(err).ToNot(HaveOccurred())
-			}
-			err := handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: 4})
-			Expect(err).ToNot(HaveOccurred())
-			err = handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: 8})
-			Expect(err).ToNot(HaveOccurred())
-			Expect(handler.receivedTimes).To(HaveLen(2)) // packets 8 and 9
-			Expect(handler.receivedTimes).To(HaveKey(protocol.PacketNumber(8)))
-			Expect(handler.receivedTimes).To(HaveKey(protocol.PacketNumber(9)))
-			Expect(handler.lowestInReceivedTimes).To(Equal(protocol.PacketNumber(8)))
-		})
-
-		It("garbage collects receivedTimes after receiving a StopWaiting, if there are missing packets", func() {
-			err := handler.ReceivedPacket(1)
-			Expect(err).ToNot(HaveOccurred())
-			err = handler.ReceivedPacket(2)
-			Expect(err).ToNot(HaveOccurred())
-			err = handler.ReceivedPacket(4)
-			Expect(err).ToNot(HaveOccurred())
-			err = handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: 4})
-			Expect(err).ToNot(HaveOccurred())
-			Expect(handler.receivedTimes).ToNot(HaveKey(protocol.PacketNumber(1)))
-			Expect(handler.receivedTimes).ToNot(HaveKey(protocol.PacketNumber(2)))
-			Expect(handler.receivedTimes).To(HaveKey(protocol.PacketNumber(4)))
-			Expect(handler.lowestInReceivedTimes).To(Equal(protocol.PacketNumber(4)))
-		})
-
-		// this prevents a DoS where a client sends us an unreasonably high LeastUnacked value
-		It("does not garbage collect packets higher than the LargestObserved packet number", func() {
-			err := handler.ReceivedPacket(10)
-			Expect(err).ToNot(HaveOccurred())
-			handler.largestObserved = 5
-			err = handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: 20})
-			Expect(err).ToNot(HaveOccurred())
-			Expect(handler.receivedTimes).To(HaveKey(protocol.PacketNumber(10)))
-			Expect(handler.lowestInReceivedTimes).To(Equal(protocol.PacketNumber(20)))
-		})
-	})
 })

ackhandler/received_packet_history.go

@@ -19,7 +19,10 @@ type receivedPacketHistory struct {
 	lowestInReceivedPacketNumbers protocol.PacketNumber
 }
 
-var errTooManyOutstandingReceivedAckRanges = qerr.Error(qerr.TooManyOutstandingReceivedPackets, "Too many outstanding received ACK ranges")
+var (
+	errTooManyOutstandingReceivedAckRanges = qerr.Error(qerr.TooManyOutstandingReceivedPackets, "Too many outstanding received ACK ranges")
+	errTooManyOutstandingReceivedPackets   = qerr.Error(qerr.TooManyOutstandingReceivedPackets, "Too many outstanding received packets")
+)
 
 // newReceivedPacketHistory creates a new received packet history
 func newReceivedPacketHistory() *receivedPacketHistory {