diff --git a/ackhandlerlegacy/sent_packet_handler.go b/ackhandlerlegacy/sent_packet_handler.go index ccc5a9e1..c28409aa 100644 --- a/ackhandlerlegacy/sent_packet_handler.go +++ b/ackhandlerlegacy/sent_packet_handler.go @@ -106,8 +106,6 @@ func (h *sentPacketHandler) queuePacketForRetransmission(packet *Packet) { h.bytesInFlight -= packet.Length h.retransmissionQueue = append(h.retransmissionQueue, packet) packet.Retransmitted = true - - // TODO: delete from packetHistory once we drop support for version smaller than QUIC 33 } func (h *sentPacketHandler) SentPacket(packet *Packet) error { diff --git a/ackhandlerlegacy/sent_packet_handler_test.go b/ackhandlerlegacy/sent_packet_handler_test.go index a0d644a3..50b6f9f1 100644 --- a/ackhandlerlegacy/sent_packet_handler_test.go +++ b/ackhandlerlegacy/sent_packet_handler_test.go @@ -459,6 +459,7 @@ var _ = Describe("SentPacketHandler", func() { Expect(err).ToNot(HaveOccurred()) } packet := handler.DequeuePacketForRetransmission() + Expect(packet).ToNot(BeNil()) Expect(packet.PacketNumber).To(Equal(protocol.PacketNumber(3))) Expect(handler.DequeuePacketForRetransmission()).To(BeNil()) }) @@ -473,6 +474,7 @@ var _ = Describe("SentPacketHandler", func() { Expect(err).ToNot(HaveOccurred()) } packet := handler.DequeuePacketForRetransmission() + Expect(packet).ToNot(BeNil()) Expect(packet.PacketNumber).To(Equal(protocol.PacketNumber(2))) packet = handler.DequeuePacketForRetransmission() Expect(packet.PacketNumber).To(Equal(protocol.PacketNumber(4))) @@ -491,8 +493,10 @@ var _ = Describe("SentPacketHandler", func() { _, err := handler.nackPacket(4) Expect(err).ToNot(HaveOccurred()) } - _ = handler.DequeuePacketForRetransmission() - _ = handler.DequeuePacketForRetransmission() + packet := handler.DequeuePacketForRetransmission() + Expect(packet).ToNot(BeNil()) + packet = handler.DequeuePacketForRetransmission() + Expect(packet).ToNot(BeNil()) Expect(handler.DequeuePacketForRetransmission()).To(BeNil()) }) @@ -508,7 +512,7 @@ var _ = Describe("SentPacketHandler", func() { Expect(handler.highestInOrderAckedPacketNumber).To(Equal(protocol.PacketNumber(2))) }) - It("does not retransmit a packet if a belated was received", func() { + It("does not retransmit a packet if a belated ACK was received", func() { // lose packet by NACKing it often enough for i := uint8(0); i < protocol.RetransmissionThreshold+1; i++ { _, err := handler.nackPacket(2) diff --git a/ackhandlernew/sent_packet_handler.go b/ackhandlernew/sent_packet_handler.go index aceeb9e3..4adf9715 100644 --- a/ackhandlernew/sent_packet_handler.go +++ b/ackhandlernew/sent_packet_handler.go @@ -85,14 +85,10 @@ func (h *sentPacketHandler) ackPacket(packetNumber protocol.PacketNumber) *Packe func (h *sentPacketHandler) nackPacket(packetNumber protocol.PacketNumber) (*Packet, error) { packet, ok := h.packetHistory[packetNumber] - if !ok { - return nil, ErrMapAccess - } - - // If the packet has already been retransmitted, do nothing. + // This means that the packet has already been retransmitted, do nothing. // We're probably only receiving another NACK for this packet because the // retransmission has not yet arrived at the client. - if packet.Retransmitted { + if !ok { return nil, nil } @@ -109,8 +105,6 @@ func (h *sentPacketHandler) queuePacketForRetransmission(packet *Packet) { h.bytesInFlight -= packet.Length h.retransmissionQueue = append(h.retransmissionQueue, packet) packet.Retransmitted = true - - // TODO: delete from packetHistory once we drop support for version smaller than QUIC 33 } func (h *sentPacketHandler) SentPacket(packet *Packet) error { @@ -231,12 +225,14 @@ func (h *sentPacketHandler) DequeuePacketForRetransmission() (packet *Packet) { packet = h.retransmissionQueue[queueLen-1] h.retransmissionQueue = h.retransmissionQueue[:queueLen-1] - // check if the packet was ACKed after it was already queued for retransmission - // if so, it doesn't exist in the packetHistory anymore. Skip it then + // this happens if a belated ACK arrives for this packet + // no need to retransmit it _, ok := h.packetHistory[packet.PacketNumber] if !ok { continue } + + delete(h.packetHistory, packet.PacketNumber) return packet } diff --git a/ackhandlernew/sent_packet_handler_test.go b/ackhandlernew/sent_packet_handler_test.go index 2fe800af..50bfee3f 100644 --- a/ackhandlernew/sent_packet_handler_test.go +++ b/ackhandlernew/sent_packet_handler_test.go @@ -390,10 +390,21 @@ var _ = Describe("SentPacketHandler", func() { Expect(err).ToNot(HaveOccurred()) } packet := handler.DequeuePacketForRetransmission() + Expect(packet).ToNot(BeNil()) Expect(packet.PacketNumber).To(Equal(protocol.PacketNumber(3))) Expect(handler.DequeuePacketForRetransmission()).To(BeNil()) }) + It("deletes a packet from the packetHistory map when sending out the retransmission", func() { + for i := uint8(0); i < protocol.RetransmissionThreshold+1; i++ { + _, err := handler.nackPacket(3) + Expect(err).ToNot(HaveOccurred()) + } + packet := handler.DequeuePacketForRetransmission() + Expect(packet).ToNot(BeNil()) + Expect(handler.packetHistory).ToNot(HaveKey(protocol.PacketNumber(3))) + }) + It("keeps the packets in the right order", func() { for i := uint8(0); i < protocol.RetransmissionThreshold+1; i++ { _, err := handler.nackPacket(4) @@ -404,8 +415,10 @@ var _ = Describe("SentPacketHandler", func() { Expect(err).ToNot(HaveOccurred()) } packet := handler.DequeuePacketForRetransmission() + Expect(packet).ToNot(BeNil()) Expect(packet.PacketNumber).To(Equal(protocol.PacketNumber(2))) packet = handler.DequeuePacketForRetransmission() + Expect(packet).ToNot(BeNil()) Expect(packet.PacketNumber).To(Equal(protocol.PacketNumber(4))) }) @@ -422,8 +435,10 @@ var _ = Describe("SentPacketHandler", func() { _, err := handler.nackPacket(4) Expect(err).ToNot(HaveOccurred()) } - _ = handler.DequeuePacketForRetransmission() - _ = handler.DequeuePacketForRetransmission() + packet := handler.DequeuePacketForRetransmission() + Expect(packet).ToNot(BeNil()) + packet = handler.DequeuePacketForRetransmission() + Expect(packet).ToNot(BeNil()) Expect(handler.DequeuePacketForRetransmission()).To(BeNil()) }) @@ -443,7 +458,7 @@ var _ = Describe("SentPacketHandler", func() { Expect(handler.highestInOrderAckedPacketNumber).To(Equal(protocol.PacketNumber(2))) }) - It("does not retransmit a packet if a belated was received", func() { + It("does not retransmit a packet if a belated ACK was received", func() { // lose packet by NACKing it often enough for i := uint8(0); i < protocol.RetransmissionThreshold+1; i++ { _, err := handler.nackPacket(2)