From 5d999f39277ddd1637a858e11b8b399fe13bef15 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sun, 21 Apr 2019 20:37:18 +0900 Subject: [PATCH] handle ACKs separately in the sent packet handler packet struct --- internal/ackhandler/ack_eliciting.go | 11 ----------- internal/ackhandler/ack_eliciting_test.go | 9 --------- internal/ackhandler/packet.go | 1 + internal/ackhandler/sent_packet_handler.go | 10 ++++------ .../ackhandler/sent_packet_handler_test.go | 9 ++++----- packet_packer.go | 18 ++++++++++++++++-- 6 files changed, 25 insertions(+), 33 deletions(-) diff --git a/internal/ackhandler/ack_eliciting.go b/internal/ackhandler/ack_eliciting.go index 0e8af8616..23940d982 100644 --- a/internal/ackhandler/ack_eliciting.go +++ b/internal/ackhandler/ack_eliciting.go @@ -2,17 +2,6 @@ package ackhandler import "github.com/lucas-clemente/quic-go/internal/wire" -// Returns a new slice with all non-ack-eliciting frames deleted. -func stripNonAckElicitingFrames(fs []wire.Frame) []wire.Frame { - res := make([]wire.Frame, 0, len(fs)) - for _, f := range fs { - if IsFrameAckEliciting(f) { - res = append(res, f) - } - } - return res -} - // IsFrameAckEliciting returns true if the frame is ack-eliciting. func IsFrameAckEliciting(f wire.Frame) bool { switch f.(type) { diff --git a/internal/ackhandler/ack_eliciting_test.go b/internal/ackhandler/ack_eliciting_test.go index 3236a496a..65263af68 100644 --- a/internal/ackhandler/ack_eliciting_test.go +++ b/internal/ackhandler/ack_eliciting_test.go @@ -27,15 +27,6 @@ var _ = Describe("ack-eliciting frames", func() { Expect(IsFrameAckEliciting(f)).To(Equal(e)) }) - It("stripping non-ack-elicinting frames works for "+fName, func() { - s := []wire.Frame{f} - if e { - Expect(stripNonAckElicitingFrames(s)).To(Equal([]wire.Frame{f})) - } else { - Expect(stripNonAckElicitingFrames(s)).To(BeEmpty()) - } - }) - It("HasAckElicitingFrames works for "+fName, func() { Expect(HasAckElicitingFrames([]wire.Frame{f})).To(Equal(e)) }) diff --git a/internal/ackhandler/packet.go b/internal/ackhandler/packet.go index 9673a85c7..5dfa608ba 100644 --- a/internal/ackhandler/packet.go +++ b/internal/ackhandler/packet.go @@ -11,6 +11,7 @@ import ( type Packet struct { PacketNumber protocol.PacketNumber PacketType protocol.PacketType + Ack *wire.AckFrame Frames []wire.Frame Length protocol.ByteCount EncryptionLevel protocol.EncryptionLevel diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index 9ca04c72e..7c3e21538 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -166,14 +166,12 @@ func (h *sentPacketHandler) sentPacketImpl(packet *Packet) bool /* is ack-elicit pnSpace.largestSent = packet.PacketNumber - if len(packet.Frames) > 0 { - if ackFrame, ok := packet.Frames[0].(*wire.AckFrame); ok { - packet.largestAcked = ackFrame.LargestAcked() - } + if packet.Ack != nil { + packet.largestAcked = packet.Ack.LargestAcked() } + packet.Ack = nil // no need to save the ACK - packet.Frames = stripNonAckElicitingFrames(packet.Frames) - isAckEliciting := len(packet.Frames) != 0 + isAckEliciting := len(packet.Frames) > 0 if isAckEliciting { if packet.EncryptionLevel != protocol.Encryption1RTT { diff --git a/internal/ackhandler/sent_packet_handler_test.go b/internal/ackhandler/sent_packet_handler_test.go index bb0c7fc83..1c2aeb5ad 100644 --- a/internal/ackhandler/sent_packet_handler_test.go +++ b/internal/ackhandler/sent_packet_handler_test.go @@ -29,9 +29,8 @@ func ackElicitingPacket(p *Packet) *Packet { func nonAckElicitingPacket(p *Packet) *Packet { p = ackElicitingPacket(p) - p.Frames = []wire.Frame{ - &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 1, Largest: 1}}}, - } + p.Frames = nil + p.Ack = &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 1, Largest: 1}}} return p } @@ -313,8 +312,8 @@ var _ = Describe("SentPacketHandler", 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, Frames: []wire.Frame{ack1, &streamFrame}, Length: 1, EncryptionLevel: protocol.Encryption1RTT}, - {PacketNumber: 14, Frames: []wire.Frame{ack2, &streamFrame}, Length: 1, EncryptionLevel: protocol.Encryption1RTT}, + {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: 15, Frames: []wire.Frame{&streamFrame}, Length: 1, EncryptionLevel: protocol.Encryption1RTT}, } for _, packet := range morePackets { diff --git a/packet_packer.go b/packet_packer.go index 71a515778..bc994b540 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -52,10 +52,24 @@ func (p *packedPacket) IsAckEliciting() bool { } func (p *packedPacket) ToAckHandlerPacket() *ackhandler.Packet { + var frames []wire.Frame + var ack *wire.AckFrame + if len(p.frames) > 0 { + var ok bool + ack, ok = p.frames[0].(*wire.AckFrame) + if ok { + // make a copy, so that the ACK can be garbage collected + frames = make([]wire.Frame, len(p.frames)-1) + copy(frames, p.frames[1:]) + } else { + frames = p.frames + } + } return &ackhandler.Packet{ PacketNumber: p.header.PacketNumber, PacketType: p.header.Type, - Frames: p.frames, + Ack: ack, + Frames: frames, Length: protocol.ByteCount(len(p.raw)), EncryptionLevel: p.EncryptionLevel(), SendTime: time.Now(), @@ -330,7 +344,7 @@ func (p *packetPacker) composeNextPacket(maxFrameSize protocol.ByteCount) ([]wir var length protocol.ByteCount var frames []wire.Frame - // ACKs need to go first, so that the sentPacketHandler will recognize them + // ACKs need to go first, so we recognize them in packedPacket.ToAckHandlerPacket() if ack := p.acks.GetAckFrame(protocol.Encryption1RTT); ack != nil { frames = append(frames, ack) length += ack.Length(p.version)