From 9221149194f89174a13b75c97e1dd5db74b03a04 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 21 Aug 2019 14:40:38 +0700 Subject: [PATCH] simplify how the largest acked is passed to the ackhandler --- internal/ackhandler/packet.go | 4 +-- internal/ackhandler/sent_packet_handler.go | 11 ++------ .../ackhandler/sent_packet_handler_test.go | 8 +++--- internal/protocol/packet_number.go | 2 +- packet_packer.go | 6 ++++- packet_packer_test.go | 27 +++++++++++++++++++ 6 files changed, 39 insertions(+), 19 deletions(-) diff --git a/internal/ackhandler/packet.go b/internal/ackhandler/packet.go index 5dfa608ba..ed917e49b 100644 --- a/internal/ackhandler/packet.go +++ b/internal/ackhandler/packet.go @@ -11,14 +11,12 @@ import ( type Packet struct { PacketNumber protocol.PacketNumber PacketType protocol.PacketType - Ack *wire.AckFrame Frames []wire.Frame + LargestAcked protocol.PacketNumber // InvalidPacketNumber if the packet doesn't contain an ACK Length protocol.ByteCount EncryptionLevel protocol.EncryptionLevel SendTime time.Time - largestAcked protocol.PacketNumber // if the packet contains an ACK, the LargestAcked value of that ACK - // 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 diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index 2747ea987..1bcb4bd4a 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -172,13 +172,6 @@ func (h *sentPacketHandler) sentPacketImpl(packet *Packet) bool /* is ack-elicit } pnSpace.largestSent = packet.PacketNumber - - packet.largestAcked = protocol.InvalidPacketNumber - if packet.Ack != nil { - packet.largestAcked = packet.Ack.LargestAcked() - } - packet.Ack = nil // no need to save the ACK - isAckEliciting := len(packet.Frames) > 0 if isAckEliciting { @@ -237,8 +230,8 @@ func (h *sentPacketHandler) ReceivedAck(ackFrame *wire.AckFrame, withPacketNumbe priorInFlight := h.bytesInFlight for _, p := range ackedPackets { - if p.largestAcked != protocol.InvalidPacketNumber && encLevel == protocol.Encryption1RTT { - h.lowestNotConfirmedAcked = utils.MaxPacketNumber(h.lowestNotConfirmedAcked, p.largestAcked+1) + if p.LargestAcked != protocol.InvalidPacketNumber && encLevel == protocol.Encryption1RTT { + h.lowestNotConfirmedAcked = utils.MaxPacketNumber(h.lowestNotConfirmedAcked, p.LargestAcked+1) } if err := h.onPacketAcked(p, rcvTime); err != nil { return err diff --git a/internal/ackhandler/sent_packet_handler_test.go b/internal/ackhandler/sent_packet_handler_test.go index 83040f8ec..d3f886258 100644 --- a/internal/ackhandler/sent_packet_handler_test.go +++ b/internal/ackhandler/sent_packet_handler_test.go @@ -31,7 +31,7 @@ func ackElicitingPacket(p *Packet) *Packet { func nonAckElicitingPacket(p *Packet) *Packet { p = ackElicitingPacket(p) p.Frames = nil - p.Ack = &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 1, Largest: 1}}} + p.LargestAcked = 1 return p } @@ -332,11 +332,9 @@ var _ = Describe("SentPacketHandler", func() { Context("determining which ACKs we have received an ACK for", func() { BeforeEach(func() { - ack1 := &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 80, Largest: 100}}} - ack2 := &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 50, Largest: 200}}} morePackets := []*Packet{ - {PacketNumber: 13, Ack: ack1, Frames: []wire.Frame{&streamFrame}, Length: 1, EncryptionLevel: protocol.Encryption1RTT}, - {PacketNumber: 14, Ack: ack2, Frames: []wire.Frame{&streamFrame}, Length: 1, EncryptionLevel: protocol.Encryption1RTT}, + {PacketNumber: 13, LargestAcked: 100, Frames: []wire.Frame{&streamFrame}, Length: 1, EncryptionLevel: protocol.Encryption1RTT}, + {PacketNumber: 14, LargestAcked: 200, Frames: []wire.Frame{&streamFrame}, Length: 1, EncryptionLevel: protocol.Encryption1RTT}, {PacketNumber: 15, Frames: []wire.Frame{&streamFrame}, Length: 1, EncryptionLevel: protocol.Encryption1RTT}, } for _, packet := range morePackets { diff --git a/internal/protocol/packet_number.go b/internal/protocol/packet_number.go index 0fa62ff3d..82ac88f28 100644 --- a/internal/protocol/packet_number.go +++ b/internal/protocol/packet_number.go @@ -5,7 +5,7 @@ type PacketNumber int64 // InvalidPacketNumber is a packet number that is never sent. // In QUIC, 0 is a valid packet number. -const InvalidPacketNumber = -1 +const InvalidPacketNumber PacketNumber = -1 // PacketNumberLen is the length of the packet number in bytes type PacketNumberLen uint8 diff --git a/packet_packer.go b/packet_packer.go index ed9589c87..91130c3ce 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -63,10 +63,14 @@ func (p *packedPacket) IsAckEliciting() bool { } func (p *packedPacket) ToAckHandlerPacket() *ackhandler.Packet { + largestAcked := protocol.InvalidPacketNumber + if p.ack != nil { + largestAcked = p.ack.LargestAcked() + } return &ackhandler.Packet{ PacketNumber: p.header.PacketNumber, PacketType: p.header.Type, - Ack: p.ack, + LargestAcked: largestAcked, Frames: p.frames, Length: protocol.ByteCount(len(p.raw)), EncryptionLevel: p.EncryptionLevel(), diff --git a/packet_packer_test.go b/packet_packer_test.go index e016870a8..11fdaaa9e 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -5,6 +5,7 @@ import ( "errors" "math/rand" "net" + "time" "github.com/golang/mock/gomock" "github.com/lucas-clemente/quic-go/internal/ackhandler" @@ -969,3 +970,29 @@ var _ = Describe("Packet packer", func() { }) }) }) + +var _ = Describe("Converting to AckHandler packets", func() { + It("convert a packet", func() { + packet := &packedPacket{ + header: &wire.ExtendedHeader{Header: wire.Header{}}, + frames: []wire.Frame{&wire.MaxDataFrame{}, &wire.PingFrame{}}, + ack: &wire.AckFrame{AckRanges: []wire.AckRange{{Largest: 100, Smallest: 80}}}, + raw: []byte("foobar"), + } + p := packet.ToAckHandlerPacket() + Expect(p.Length).To(Equal(protocol.ByteCount(6))) + Expect(p.Frames).To(Equal(packet.frames)) + Expect(p.LargestAcked).To(Equal(protocol.PacketNumber(100))) + Expect(p.SendTime).To(BeTemporally("~", time.Now(), 50*time.Millisecond)) + }) + + It("sets the LargestAcked to invalid, if the packet doesn't have an ACK frame", func() { + packet := &packedPacket{ + header: &wire.ExtendedHeader{Header: wire.Header{}}, + frames: []wire.Frame{&wire.MaxDataFrame{}, &wire.PingFrame{}}, + raw: []byte("foobar"), + } + p := packet.ToAckHandlerPacket() + Expect(p.LargestAcked).To(Equal(protocol.InvalidPacketNumber)) + }) +})