From 4f3f1065cb1342d95cada3022797c1469708b174 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Tue, 16 Aug 2016 19:43:40 +0700 Subject: [PATCH] improve return values of (n)ackPacket in new SentPacketHandler --- ackhandler/sent_packet_handler.go | 42 +++++++++-------------- ackhandler/sent_packet_handler_test.go | 46 +++++++++++++++++--------- 2 files changed, 46 insertions(+), 42 deletions(-) diff --git a/ackhandler/sent_packet_handler.go b/ackhandler/sent_packet_handler.go index 4a9abec4..c617b5b2 100644 --- a/ackhandler/sent_packet_handler.go +++ b/ackhandler/sent_packet_handler.go @@ -66,7 +66,7 @@ func NewSentPacketHandler() SentPacketHandler { } } -func (h *sentPacketHandler) ackPacket(packetElement *ackhandlerlegacy.PacketElement) *ackhandlerlegacy.Packet { +func (h *sentPacketHandler) ackPacket(packetElement *ackhandlerlegacy.PacketElement) { packet := &packetElement.Value h.bytesInFlight -= packet.Length @@ -80,11 +80,11 @@ func (h *sentPacketHandler) ackPacket(packetElement *ackhandlerlegacy.PacketElem } h.packetHistory.Remove(packetElement) - - return packet } -func (h *sentPacketHandler) nackPacket(packetElement *ackhandlerlegacy.PacketElement) (*ackhandlerlegacy.Packet, error) { +// nackPacket NACKs a packet +// it returns true if a FastRetransmissions was triggered +func (h *sentPacketHandler) nackPacket(packetElement *ackhandlerlegacy.PacketElement) bool { packet := &packetElement.Value packet.MissingReports++ @@ -92,9 +92,9 @@ func (h *sentPacketHandler) nackPacket(packetElement *ackhandlerlegacy.PacketEle if packet.MissingReports > protocol.RetransmissionThreshold { utils.Debugf("\tQueueing packet 0x%x for retransmission (fast)", packet.PacketNumber) h.queuePacketForRetransmission(packetElement) - return packet, nil + return true } - return nil, nil + return false } // does NOT set packet.Retransmitted. This variable is not needed anymore @@ -195,12 +195,9 @@ func (h *sentPacketHandler) ReceivedAck(ackFrame *frames.AckFrame, withPacketNum // NACK packets below the LowestAcked if packetNumber < ackFrame.LowestAcked { - p, err := h.nackPacket(el) - if err != nil { - return err - } - if p != nil { - lostPackets = append(lostPackets, congestion.PacketInfo{Number: p.PacketNumber, Length: p.Length}) + retransmitted := h.nackPacket(el) + if retransmitted { + lostPackets = append(lostPackets, congestion.PacketInfo{Number: packetNumber, Length: packet.Length}) } continue } @@ -231,24 +228,17 @@ func (h *sentPacketHandler) ReceivedAck(ackFrame *frames.AckFrame, withPacketNum if packetNumber > ackRange.LastPacketNumber { return fmt.Errorf("BUG: ackhandler would have acked wrong packet 0x%x, while evaluating range 0x%x -> 0x%x", packetNumber, ackRange.FirstPacketNumber, ackRange.LastPacketNumber) } - p := h.ackPacket(el) - if p != nil { - ackedPackets = append(ackedPackets, congestion.PacketInfo{Number: p.PacketNumber, Length: p.Length}) - } + h.ackPacket(el) + ackedPackets = append(ackedPackets, congestion.PacketInfo{Number: packetNumber, Length: packet.Length}) } else { - p, err := h.nackPacket(el) - if err != nil { - return err - } - if p != nil { - lostPackets = append(lostPackets, congestion.PacketInfo{Number: p.PacketNumber, Length: p.Length}) + retransmitted := h.nackPacket(el) + if retransmitted { + lostPackets = append(lostPackets, congestion.PacketInfo{Number: packetNumber, Length: packet.Length}) } } } else { - p := h.ackPacket(el) - if p != nil { - ackedPackets = append(ackedPackets, congestion.PacketInfo{Number: p.PacketNumber, Length: p.Length}) - } + h.ackPacket(el) + ackedPackets = append(ackedPackets, congestion.PacketInfo{Number: packetNumber, Length: packet.Length}) } } diff --git a/ackhandler/sent_packet_handler_test.go b/ackhandler/sent_packet_handler_test.go index 5c75555a..79be2063 100644 --- a/ackhandler/sent_packet_handler_test.go +++ b/ackhandler/sent_packet_handler_test.go @@ -564,8 +564,9 @@ var _ = Describe("SentPacketHandler", func() { It("does not dequeue a packet if no packet has been nacked", func() { for i := uint8(0); i < protocol.RetransmissionThreshold; i++ { - _, err := handler.nackPacket(getPacketElement(2)) - Expect(err).ToNot(HaveOccurred()) + el := getPacketElement(2) + Expect(el).ToNot(BeNil()) + handler.nackPacket(el) } Expect(getPacketElement(2)).ToNot(BeNil()) handler.MaybeQueueRTOs() @@ -574,8 +575,9 @@ var _ = Describe("SentPacketHandler", func() { It("queues a packet for retransmission", func() { for i := uint8(0); i < protocol.RetransmissionThreshold+1; i++ { - _, err := handler.nackPacket(getPacketElement(2)) - Expect(err).ToNot(HaveOccurred()) + el := getPacketElement(2) + Expect(el).ToNot(BeNil()) + handler.nackPacket(el) } Expect(getPacketElement(2)).To(BeNil()) handler.MaybeQueueRTOs() @@ -585,8 +587,9 @@ var _ = Describe("SentPacketHandler", func() { It("dequeues a packet for retransmission", func() { for i := uint8(0); i < protocol.RetransmissionThreshold+1; i++ { - _, err := handler.nackPacket(getPacketElement(3)) - Expect(err).ToNot(HaveOccurred()) + el := getPacketElement(3) + Expect(el).ToNot(BeNil()) + handler.nackPacket(el) } packet := handler.DequeuePacketForRetransmission() Expect(packet).ToNot(BeNil()) @@ -596,12 +599,14 @@ var _ = Describe("SentPacketHandler", func() { It("keeps the packets in the right order", func() { for i := uint8(0); i < protocol.RetransmissionThreshold+1; i++ { - _, err := handler.nackPacket(getPacketElement(4)) - Expect(err).ToNot(HaveOccurred()) + el := getPacketElement(4) + Expect(el).ToNot(BeNil()) + handler.nackPacket(el) } for i := uint8(0); i < protocol.RetransmissionThreshold+1; i++ { - _, err := handler.nackPacket(getPacketElement(2)) - Expect(err).ToNot(HaveOccurred()) + el := getPacketElement(2) + Expect(el).ToNot(BeNil()) + handler.nackPacket(el) } packet := handler.DequeuePacketForRetransmission() Expect(packet).ToNot(BeNil()) @@ -626,7 +631,9 @@ var _ = Describe("SentPacketHandler", func() { Expect(handler.LargestInOrderAcked).To(Equal(protocol.PacketNumber(2))) // this will trigger a retransmission of packet 3 for i := uint8(0); i < protocol.RetransmissionThreshold; i++ { - handler.nackPacket(getPacketElement(3)) + el := getPacketElement(3) + Expect(el).ToNot(BeNil()) + handler.nackPacket(el) } Expect(handler.LargestInOrderAcked).To(Equal(protocol.PacketNumber(5))) }) @@ -646,7 +653,9 @@ var _ = Describe("SentPacketHandler", func() { Expect(handler.LargestInOrderAcked).To(Equal(protocol.PacketNumber(2))) // this will trigger a retransmission of packet 6 for i := uint8(0); i < protocol.RetransmissionThreshold; i++ { - handler.nackPacket(getPacketElement(6)) + el := getPacketElement(6) + Expect(el).ToNot(BeNil()) + handler.nackPacket(el) } Expect(handler.LargestInOrderAcked).To(Equal(protocol.PacketNumber(2))) }) @@ -665,12 +674,16 @@ var _ = Describe("SentPacketHandler", func() { Expect(err).ToNot(HaveOccurred()) // this will trigger a retransmission of packet 3 for i := uint8(0); i < protocol.RetransmissionThreshold; i++ { - handler.nackPacket(getPacketElement(3)) + el := getPacketElement(3) + Expect(el).ToNot(BeNil()) + handler.nackPacket(el) } Expect(handler.LargestInOrderAcked).To(Equal(protocol.PacketNumber(1))) // now, trigger retransmission of 2 ... for i := uint8(0); i < protocol.RetransmissionThreshold; i++ { - handler.nackPacket(getPacketElement(2)) + el := getPacketElement(2) + Expect(el).ToNot(BeNil()) + handler.nackPacket(el) } // and check that it increased LargestInOrderAcked past 3 Expect(handler.LargestInOrderAcked).To(Equal(protocol.PacketNumber(5))) @@ -719,8 +732,9 @@ var _ = Describe("SentPacketHandler", func() { // Simulate protocol.RetransmissionThreshold more NACKs for i := uint8(0); i < protocol.RetransmissionThreshold; i++ { - _, err = handler.nackPacket(getPacketElement(2)) - Expect(err).ToNot(HaveOccurred()) + el := getPacketElement(2) + Expect(el).ToNot(BeNil()) + handler.nackPacket(el) } Expect(handler.BytesInFlight()).To(Equal(protocol.ByteCount(0)))