remove tracking of which packet is retransmission of which other packet

internal/ackhandler/interfaces.go

@@ -17,21 +17,14 @@ type Packet struct {
 	EncryptionLevel protocol.EncryptionLevel
 	SendTime        time.Time
 
-	// There are two reasons why a packet cannot be retransmitted:
-	// * it was already retransmitted
-	// * this packet is a retransmission, and we already received an ACK for the original packet
 	canBeRetransmitted      bool
 	includedInBytesInFlight bool
-	retransmittedAs         []protocol.PacketNumber
-	isRetransmission        bool // we need a separate bool here because 0 is a valid packet number
-	retransmissionOf        protocol.PacketNumber
 }
 
 // SentPacketHandler handles ACKs received for outgoing packets
 type SentPacketHandler interface {
 	// SentPacket may modify the packet
 	SentPacket(packet *Packet)
-	SentPacketsAsRetransmission(packets []*Packet, retransmissionOf protocol.PacketNumber)
 	ReceivedAck(ackFrame *wire.AckFrame, withPacketNumber protocol.PacketNumber, encLevel protocol.EncryptionLevel, recvTime time.Time) error
 	DropPackets(protocol.EncryptionLevel)
 	ResetForRetry() error

internal/ackhandler/sent_packet_handler.go

@@ -138,17 +138,6 @@ func (h *sentPacketHandler) SentPacket(packet *Packet) {
 	}
 }
 
-func (h *sentPacketHandler) SentPacketsAsRetransmission(packets []*Packet, retransmissionOf protocol.PacketNumber) {
-	var p []*Packet
-	for _, packet := range packets {
-		if isAckEliciting := h.sentPacketImpl(packet); isAckEliciting {
-			p = append(p, packet)
-		}
-	}
-	h.getPacketNumberSpace(p[0].EncryptionLevel).history.SentPacketsAsRetransmission(p, retransmissionOf)
-	h.setLossDetectionTimer()
-}
-
 func (h *sentPacketHandler) getPacketNumberSpace(encLevel protocol.EncryptionLevel) *packetNumberSpace {
 	switch encLevel {
 	case protocol.EncryptionInitial:
@@ -472,48 +461,14 @@ func (h *sentPacketHandler) onPacketAcked(p *Packet, rcvTime time.Time) error {
 		return nil
 	}
 
-	// only report the acking of this packet to the congestion controller if:
-	// * it is an ack-eliciting packet
-	// * this packet wasn't retransmitted yet
-	if p.isRetransmission {
-		// that the parent doesn't exist is expected to happen every time the original packet was already acked
-		if parent := pnSpace.history.GetPacket(p.retransmissionOf); parent != nil {
-			if len(parent.retransmittedAs) == 1 {
-				parent.retransmittedAs = nil
-			} else {
-				// remove this packet from the slice of retransmission
-				retransmittedAs := make([]protocol.PacketNumber, 0, len(parent.retransmittedAs)-1)
-				for _, pn := range parent.retransmittedAs {
-					if pn != p.PacketNumber {
-						retransmittedAs = append(retransmittedAs, pn)
-					}
-				}
-				parent.retransmittedAs = retransmittedAs
-			}
-		}
-	}
 	// this also applies to packets that have been retransmitted as probe packets
 	if p.includedInBytesInFlight {
 		h.bytesInFlight -= p.Length
 	}
-	if err := h.stopRetransmissionsFor(p, pnSpace); err != nil {
-		return err
-	}
-	return pnSpace.history.Remove(p.PacketNumber)
-}
-
-func (h *sentPacketHandler) stopRetransmissionsFor(p *Packet, pnSpace *packetNumberSpace) error {
 	if err := pnSpace.history.MarkCannotBeRetransmitted(p.PacketNumber); err != nil {
 		return err
 	}
-	for _, r := range p.retransmittedAs {
-		packet := pnSpace.history.GetPacket(r)
-		if packet == nil {
-			return fmt.Errorf("sent packet handler BUG: marking packet as not retransmittable %d (retransmission of %d) not found in history", r, p.PacketNumber)
-		}
-		h.stopRetransmissionsFor(packet, pnSpace)
-	}
-	return nil
+	return pnSpace.history.Remove(p.PacketNumber)
 }
 
 func (h *sentPacketHandler) DequeuePacketForRetransmission() *Packet {

internal/ackhandler/sent_packet_handler_test.go

@@ -63,19 +63,6 @@ var _ = Describe("SentPacketHandler", func() {
 		return nil
 	}
 
-	losePacket := func(pn protocol.PacketNumber, encLevel protocol.EncryptionLevel) {
-		p := getPacket(pn, encLevel)
-		ExpectWithOffset(1, p).ToNot(BeNil())
-		handler.queuePacketForRetransmission(p, handler.getPacketNumberSpace(encLevel))
-		if p.includedInBytesInFlight {
-			p.includedInBytesInFlight = false
-			handler.bytesInFlight -= p.Length
-		}
-		r := handler.DequeuePacketForRetransmission()
-		ExpectWithOffset(1, r).ToNot(BeNil())
-		ExpectWithOffset(1, r.PacketNumber).To(Equal(pn))
-	}
-
 	expectInPacketHistory := func(expected []protocol.PacketNumber, encLevel protocol.EncryptionLevel) {
 		pnSpace := handler.getPacketNumberSpace(encLevel)
 		ExpectWithOffset(1, pnSpace.history.Len()).To(Equal(len(expected)))
@@ -368,50 +355,6 @@ var _ = Describe("SentPacketHandler", func() {
 		})
 	})
 
-	Context("ACK processing, for retransmitted packets", func() {
-		It("sends a packet as retransmission", func() {
-			// packet 5 was retransmitted as packet 6
-			handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: 5, Length: 10, EncryptionLevel: protocol.Encryption1RTT}))
-			Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(10)))
-			losePacket(5, protocol.Encryption1RTT)
-			Expect(handler.bytesInFlight).To(BeZero())
-			handler.SentPacketsAsRetransmission([]*Packet{ackElicitingPacket(&Packet{PacketNumber: 6, Length: 11})}, 5)
-			Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(11)))
-		})
-
-		It("removes a packet when it is acked", func() {
-			// packet 5 was retransmitted as packet 6
-			handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: 5, Length: 10}))
-			losePacket(5, protocol.Encryption1RTT)
-			handler.SentPacketsAsRetransmission([]*Packet{ackElicitingPacket(&Packet{PacketNumber: 6, Length: 11})}, 5)
-			Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(11)))
-			// ack 5
-			ack := &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 5, Largest: 5}}}
-			err := handler.ReceivedAck(ack, 1, protocol.Encryption1RTT, time.Now())
-			Expect(err).ToNot(HaveOccurred())
-			expectInPacketHistory([]protocol.PacketNumber{6}, protocol.Encryption1RTT)
-			Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(11)))
-		})
-
-		It("handles ACKs that ack the original packet as well as the retransmission", func() {
-			// packet 5 was retransmitted as packet 7
-			handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: 5, Length: 10}))
-			losePacket(5, protocol.Encryption1RTT)
-			handler.SentPacketsAsRetransmission([]*Packet{ackElicitingPacket(&Packet{PacketNumber: 7, Length: 11})}, 5)
-			// ack 5 and 7
-			ack := &wire.AckFrame{
-				AckRanges: []wire.AckRange{
-					{Smallest: 7, Largest: 7},
-					{Smallest: 5, Largest: 5},
-				},
-			}
-			err := handler.ReceivedAck(ack, 1, protocol.Encryption1RTT, time.Now())
-			Expect(err).ToNot(HaveOccurred())
-			Expect(handler.oneRTTPackets.history.Len()).To(BeZero())
-			Expect(handler.bytesInFlight).To(BeZero())
-		})
-	})
-
 	It("does not dequeue a packet if no ACK has been received", func() {
 		handler.SentPacket(&Packet{PacketNumber: 1, EncryptionLevel: protocol.Encryption1RTT, SendTime: time.Now().Add(-time.Hour)})
 		Expect(handler.DequeuePacketForRetransmission()).To(BeNil())
@@ -751,20 +694,6 @@ var _ = Describe("SentPacketHandler", func() {
 			Expect(handler.retransmissionQueue).To(HaveLen(3)) // packets 3, 4, 5
 		})
 
-		It("handles ACKs for the original packet", func() {
-			handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: 5, SendTime: time.Now().Add(-time.Hour)}))
-			handler.rttStats.UpdateRTT(time.Second, 0, time.Now())
-			handler.OnLossDetectionTimeout() // TLP
-			handler.OnLossDetectionTimeout() // TLP
-			handler.OnLossDetectionTimeout() // RTO
-			handler.SentPacketsAsRetransmission([]*Packet{ackElicitingPacket(&Packet{PacketNumber: 6})}, 5)
-			ack := &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 5, Largest: 5}}}
-			err := handler.ReceivedAck(ack, 1, protocol.Encryption1RTT, time.Now())
-			Expect(err).ToNot(HaveOccurred())
-			err = handler.OnLossDetectionTimeout()
-			Expect(err).ToNot(HaveOccurred())
-		})
-
 		It("handles ACKs for the original packet", func() {
 			handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: 5, SendTime: time.Now().Add(-time.Hour)}))
 			handler.rttStats.UpdateRTT(time.Second, 0, time.Now())

internal/ackhandler/sent_packet_history.go

@@ -38,27 +38,6 @@ func (h *sentPacketHistory) sentPacketImpl(p *Packet) *PacketElement {
 	return el
 }
 
-func (h *sentPacketHistory) SentPacketsAsRetransmission(packets []*Packet, retransmissionOf protocol.PacketNumber) {
-	retransmission, ok := h.packetMap[retransmissionOf]
-	// The retransmitted packet is not present anymore.
-	// This can happen if it was acked in between dequeueing of the retransmission and sending.
-	// Just treat the retransmissions as normal packets.
-	// TODO: This won't happen if we clear packets queued for retransmission on new ACKs.
-	if !ok {
-		for _, packet := range packets {
-			h.sentPacketImpl(packet)
-		}
-		return
-	}
-	retransmission.Value.retransmittedAs = make([]protocol.PacketNumber, len(packets))
-	for i, packet := range packets {
-		retransmission.Value.retransmittedAs[i] = packet.PacketNumber
-		el := h.sentPacketImpl(packet)
-		el.Value.isRetransmission = true
-		el.Value.retransmissionOf = retransmissionOf
-	}
-}
-
 func (h *sentPacketHistory) GetPacket(p protocol.PacketNumber) *Packet {
 	if el, ok := h.packetMap[p]; ok {
 		return &el.Value

internal/ackhandler/sent_packet_history_test.go

@@ -161,43 +161,6 @@ var _ = Describe("SentPacketHistory", func() {
 		})
 	})
 
-	Context("retransmissions", func() {
-		BeforeEach(func() {
-			for i := protocol.PacketNumber(1); i <= 5; i++ {
-				hist.SentPacket(&Packet{PacketNumber: i})
-			}
-		})
-
-		It("errors if the packet doesn't exist", func() {
-			err := hist.MarkCannotBeRetransmitted(100)
-			Expect(err).To(MatchError("sent packet history: packet 100 not found"))
-		})
-
-		It("adds a sent packets as a retransmission", func() {
-			hist.SentPacketsAsRetransmission([]*Packet{{PacketNumber: 13}}, 2)
-			expectInHistory([]protocol.PacketNumber{1, 2, 3, 4, 5, 13})
-			Expect(hist.GetPacket(13).isRetransmission).To(BeTrue())
-			Expect(hist.GetPacket(13).retransmissionOf).To(Equal(protocol.PacketNumber(2)))
-			Expect(hist.GetPacket(2).retransmittedAs).To(Equal([]protocol.PacketNumber{13}))
-		})
-
-		It("adds multiple packets sent as a retransmission", func() {
-			hist.SentPacketsAsRetransmission([]*Packet{{PacketNumber: 13}, {PacketNumber: 15}}, 2)
-			expectInHistory([]protocol.PacketNumber{1, 2, 3, 4, 5, 13, 15})
-			Expect(hist.GetPacket(13).isRetransmission).To(BeTrue())
-			Expect(hist.GetPacket(13).retransmissionOf).To(Equal(protocol.PacketNumber(2)))
-			Expect(hist.GetPacket(15).retransmissionOf).To(Equal(protocol.PacketNumber(2)))
-			Expect(hist.GetPacket(2).retransmittedAs).To(Equal([]protocol.PacketNumber{13, 15}))
-		})
-
-		It("adds a packet as a normal packet if the retransmitted packet doesn't exist", func() {
-			hist.SentPacketsAsRetransmission([]*Packet{{PacketNumber: 13}}, 7)
-			expectInHistory([]protocol.PacketNumber{1, 2, 3, 4, 5, 13})
-			Expect(hist.GetPacket(13).isRetransmission).To(BeFalse())
-			Expect(hist.GetPacket(13).retransmissionOf).To(BeZero())
-		})
-	})
-
 	Context("outstanding packets", func() {
 		It("says if it has outstanding packets", func() {
 			Expect(hist.HasOutstandingPackets()).To(BeFalse())

internal/mocks/ackhandler/sent_packet_handler.go

@@ -218,18 +218,6 @@ func (mr *MockSentPacketHandlerMockRecorder) SentPacket(arg0 interface{}) *gomoc
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SentPacket", reflect.TypeOf((*MockSentPacketHandler)(nil).SentPacket), arg0)
 }
 
-// SentPacketsAsRetransmission mocks base method
-func (m *MockSentPacketHandler) SentPacketsAsRetransmission(arg0 []*ackhandler.Packet, arg1 protocol.PacketNumber) {
-	m.ctrl.T.Helper()
-	m.ctrl.Call(m, "SentPacketsAsRetransmission", arg0, arg1)
-}
-
-// SentPacketsAsRetransmission indicates an expected call of SentPacketsAsRetransmission
-func (mr *MockSentPacketHandlerMockRecorder) SentPacketsAsRetransmission(arg0, arg1 interface{}) *gomock.Call {
-	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SentPacketsAsRetransmission", reflect.TypeOf((*MockSentPacketHandler)(nil).SentPacketsAsRetransmission), arg0, arg1)
-}
-
 // ShouldSendNumPackets mocks base method
 func (m *MockSentPacketHandler) ShouldSendNumPackets() int {
 	m.ctrl.T.Helper()

session.go

@@ -1170,12 +1170,8 @@ func (s *session) maybeSendRetransmission() (bool, error) {
 	if err != nil {
 		return false, err
 	}
-	ackhandlerPackets := make([]*ackhandler.Packet, len(packets))
-	for i, packet := range packets {
-		ackhandlerPackets[i] = packet.ToAckHandlerPacket()
-	}
-	s.sentPacketHandler.SentPacketsAsRetransmission(ackhandlerPackets, retransmitPacket.PacketNumber)
 	for _, packet := range packets {
+		s.sentPacketHandler.SentPacket(packet.ToAckHandlerPacket())
 		s.sendPackedPacket(packet)
 	}
 	return true, nil
@@ -1192,12 +1188,8 @@ func (s *session) sendProbePacket() error {
 	if err != nil {
 		return err
 	}
-	ackhandlerPackets := make([]*ackhandler.Packet, len(packets))
-	for i, packet := range packets {
-		ackhandlerPackets[i] = packet.ToAckHandlerPacket()
-	}
-	s.sentPacketHandler.SentPacketsAsRetransmission(ackhandlerPackets, p.PacketNumber)
 	for _, packet := range packets {
+		s.sentPacketHandler.SentPacket(packet.ToAckHandlerPacket())
 		s.sendPackedPacket(packet)
 	}
 	return nil

session_test.go

@@ -906,9 +906,8 @@ var _ = Describe("Session", func() {
 			sph.EXPECT().TimeUntilSend()
 			gomock.InOrder(
 				packer.EXPECT().PackRetransmission(packetToRetransmit).Return([]*packedPacket{retransmittedPacket}, nil),
-				sph.EXPECT().SentPacketsAsRetransmission(gomock.Any(), protocol.PacketNumber(10)).Do(func(packets []*ackhandler.Packet, _ protocol.PacketNumber) {
-					Expect(packets).To(HaveLen(1))
-					Expect(packets[0].PacketNumber).To(Equal(protocol.PacketNumber(123)))
+				sph.EXPECT().SentPacket(gomock.Any()).Do(func(packet *ackhandler.Packet) {
+					Expect(packet.PacketNumber).To(Equal(protocol.PacketNumber(123)))
 				}),
 				packer.EXPECT().PackPacket().Return(newPacket, nil),
 				sph.EXPECT().SentPacket(gomock.Any()).Do(func(p *ackhandler.Packet) {
@@ -934,11 +933,14 @@ var _ = Describe("Session", func() {
 			sph.EXPECT().GetLossDetectionTimeout().AnyTimes()
 			sph.EXPECT().DequeuePacketForRetransmission().Return(packet)
 			packer.EXPECT().PackRetransmission(packet).Return(retransmissions, nil)
-			sph.EXPECT().SentPacketsAsRetransmission(gomock.Any(), protocol.PacketNumber(42)).Do(func(packets []*ackhandler.Packet, _ protocol.PacketNumber) {
-				Expect(packets).To(HaveLen(2))
-				Expect(packets[0].PacketNumber).To(Equal(protocol.PacketNumber(1337)))
-				Expect(packets[1].PacketNumber).To(Equal(protocol.PacketNumber(1338)))
-			})
+			gomock.InOrder(
+				sph.EXPECT().SentPacket(gomock.Any()).Do(func(packet *ackhandler.Packet) {
+					Expect(packet.PacketNumber).To(Equal(protocol.PacketNumber(1337)))
+				}),
+				sph.EXPECT().SentPacket(gomock.Any()).Do(func(packet *ackhandler.Packet) {
+					Expect(packet.PacketNumber).To(Equal(protocol.PacketNumber(1338)))
+				}),
+			)
 			sess.sentPacketHandler = sph
 			sent, err := sess.maybeSendRetransmission()
 			Expect(err).NotTo(HaveOccurred())
@@ -956,9 +958,8 @@ var _ = Describe("Session", func() {
 			sph.EXPECT().ShouldSendNumPackets().Return(1)
 			sph.EXPECT().DequeueProbePacket().Return(packetToRetransmit, nil)
 			packer.EXPECT().PackRetransmission(packetToRetransmit).Return([]*packedPacket{retransmittedPacket}, nil)
-			sph.EXPECT().SentPacketsAsRetransmission(gomock.Any(), protocol.PacketNumber(0x42)).Do(func(packets []*ackhandler.Packet, _ protocol.PacketNumber) {
-				Expect(packets).To(HaveLen(1))
-				Expect(packets[0].PacketNumber).To(Equal(protocol.PacketNumber(123)))
+			sph.EXPECT().SentPacket(gomock.Any()).Do(func(packet *ackhandler.Packet) {
+				Expect(packet.PacketNumber).To(Equal(protocol.PacketNumber(123)))
 			})
 			sess.sentPacketHandler = sph
 			Expect(sess.sendPackets()).To(Succeed())