From 29fe5cac76f014f33e9c27e2a3611ba589641787 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sat, 29 Oct 2016 02:36:48 +0700 Subject: [PATCH] only save timestamp of largest observed packet in ReceivedPacketHandler fixes #336 --- ackhandler/received_packet_handler.go | 39 ++-------- ackhandler/received_packet_handler_test.go | 82 ++++++---------------- ackhandler/received_packet_history.go | 5 +- 3 files changed, 28 insertions(+), 98 deletions(-) diff --git a/ackhandler/received_packet_handler.go b/ackhandler/received_packet_handler.go index 94ca14d7..56ca3efd 100644 --- a/ackhandler/received_packet_handler.go +++ b/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 - } -} diff --git a/ackhandler/received_packet_handler_test.go b/ackhandler/received_packet_handler_test.go index 6109efd6..d1d644f8 100644 --- a/ackhandler/received_packet_handler_test.go +++ b/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))) - }) - }) }) diff --git a/ackhandler/received_packet_history.go b/ackhandler/received_packet_history.go index 9b2ee687..130fc4fc 100644 --- a/ackhandler/received_packet_history.go +++ b/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 {