diff --git a/internal/ackhandler/interfaces.go b/internal/ackhandler/interfaces.go index 817f0d82..2b03c977 100644 --- a/internal/ackhandler/interfaces.go +++ b/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 diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index 1bcb4bd4..56600ef8 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/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 { diff --git a/internal/ackhandler/sent_packet_handler_test.go b/internal/ackhandler/sent_packet_handler_test.go index d3f88625..2bf944a0 100644 --- a/internal/ackhandler/sent_packet_handler_test.go +++ b/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()) diff --git a/internal/ackhandler/sent_packet_history.go b/internal/ackhandler/sent_packet_history.go index 393ebba6..857fd306 100644 --- a/internal/ackhandler/sent_packet_history.go +++ b/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 diff --git a/internal/ackhandler/sent_packet_history_test.go b/internal/ackhandler/sent_packet_history_test.go index e79db47a..eea18967 100644 --- a/internal/ackhandler/sent_packet_history_test.go +++ b/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()) diff --git a/internal/mocks/ackhandler/sent_packet_handler.go b/internal/mocks/ackhandler/sent_packet_handler.go index 85cfec19..2f231983 100644 --- a/internal/mocks/ackhandler/sent_packet_handler.go +++ b/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() diff --git a/session.go b/session.go index b01dc87d..6ea39965 100644 --- a/session.go +++ b/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 diff --git a/session_test.go b/session_test.go index 3540e605..f418da0d 100644 --- a/session_test.go +++ b/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())