From f112edd894a3244cd94a4c132bc2780e3a67caf8 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Mon, 22 Apr 2019 12:45:53 +0900 Subject: [PATCH] handle ACK frames separately when packing packets --- packet_packer.go | 77 +++++++++++++++++++------------------------ packet_packer_test.go | 29 ++++++++-------- 2 files changed, 50 insertions(+), 56 deletions(-) diff --git a/packet_packer.go b/packet_packer.go index 6f0dca02b..b52a29c4f 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -27,12 +27,14 @@ type packer interface { type payload struct { frames []wire.Frame + ack *wire.AckFrame length protocol.ByteCount } type packedPacket struct { header *wire.ExtendedHeader raw []byte + ack *wire.AckFrame frames []wire.Frame buffer *packetBuffer @@ -57,24 +59,11 @@ 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, - Ack: ack, - Frames: frames, + Ack: p.ack, + Frames: p.frames, Length: protocol.ByteCount(len(p.raw)), EncryptionLevel: p.EncryptionLevel(), SendTime: time.Now(), @@ -185,7 +174,7 @@ func (p *packetPacker) MaybePackAckPacket() (*packedPacket, error) { return nil, nil } payload := payload{ - frames: []wire.Frame{ack}, + ack: ack, length: ack.Length(p.version), } // TODO(#1534): only pack ACKs with the right encryption level @@ -291,12 +280,11 @@ func (p *packetPacker) PackPacket() (*packedPacket, error) { return nil, err } - // Check if we have enough frames to send - if len(payload.frames) == 0 { + // check if we have anything to send + if len(payload.frames) == 0 && payload.ack == nil { return nil, nil } - // check if this packet only contains an ACK - if !ackhandler.HasAckElicitingFrames(payload.frames) { + if len(payload.frames) == 0 { // the packet only contains an ACK if p.numNonAckElicitingAcks >= protocol.MaxNonAckElicitingAcks { ping := &wire.PingFrame{} payload.frames = append(payload.frames, ping) @@ -338,35 +326,32 @@ func (p *packetPacker) maybePackCryptoPacket() (*packedPacket, error) { return nil, err } + var payload payload + if ack != nil { + payload.ack = ack + payload.length = ack.Length(p.version) + } hdr := p.getHeader(encLevel) hdrLen := hdr.GetLength(p.version) - var length protocol.ByteCount - frames := make([]wire.Frame, 0, 2) - if ack != nil { - frames = append(frames, ack) - length += ack.Length(p.version) - } if hasData { - cf := s.PopCryptoFrame(p.maxPacketSize - hdrLen - protocol.ByteCount(sealer.Overhead()) - length) - frames = append(frames, cf) - length += cf.Length(p.version) + cf := s.PopCryptoFrame(p.maxPacketSize - hdrLen - protocol.ByteCount(sealer.Overhead()) - payload.length) + payload.frames = []wire.Frame{cf} + payload.length += cf.Length(p.version) } - return p.writeAndSealPacket(hdr, payload{frames: frames, length: length}, encLevel, sealer) + return p.writeAndSealPacket(hdr, payload, encLevel, sealer) } func (p *packetPacker) composeNextPacket(maxFrameSize protocol.ByteCount) (payload, error) { - var length protocol.ByteCount - var frames []wire.Frame + var payload payload // 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) + payload.ack = ack + payload.length += ack.Length(p.version) } - var lengthAdded protocol.ByteCount - frames, lengthAdded = p.framer.AppendControlFrames(frames, maxFrameSize-length) - length += lengthAdded + frames, lengthAdded := p.framer.AppendControlFrames(payload.frames, maxFrameSize-payload.length) + payload.length += lengthAdded // temporarily increase the maxFrameSize by the (minimum) length of the DataLen field // this leads to a properly sized packet in all cases, since we do all the packet length calculations with STREAM frames that have the DataLen set @@ -374,15 +359,16 @@ func (p *packetPacker) composeNextPacket(maxFrameSize protocol.ByteCount) (paylo // the length is encoded to either 1 or 2 bytes maxFrameSize++ - frames, lengthAdded = p.framer.AppendStreamFrames(frames, maxFrameSize-length) + frames, lengthAdded = p.framer.AppendStreamFrames(frames, maxFrameSize-payload.length) if len(frames) > 0 { lastFrame := frames[len(frames)-1] if sf, ok := lastFrame.(*wire.StreamFrame); ok { sf.DataLenPresent = false } - length += lengthAdded + payload.frames = append(payload.frames, frames...) + payload.length += lengthAdded } - return payload{frames: frames, length: length}, nil + return payload, nil } func (p *packetPacker) getHeader(encLevel protocol.EncryptionLevel) *wire.ExtendedHeader { @@ -447,17 +433,21 @@ func (p *packetPacker) writeAndSealPacketWithPadding( ) (*packedPacket, error) { packetBuffer := getPacketBuffer() buffer := bytes.NewBuffer(packetBuffer.Slice[:0]) - frames := payload.frames if err := header.Write(buffer, p.version); err != nil { return nil, err } payloadOffset := buffer.Len() + if payload.ack != nil { + if err := payload.ack.Write(buffer, p.version); err != nil { + return nil, err + } + } if paddingLen > 0 { buffer.Write(bytes.Repeat([]byte{0}, int(paddingLen))) } - for _, frame := range frames { + for _, frame := range payload.frames { if err := frame.Write(buffer, p.version); err != nil { return nil, err } @@ -485,7 +475,8 @@ func (p *packetPacker) writeAndSealPacketWithPadding( return &packedPacket{ header: header, raw: raw, - frames: frames, + ack: payload.ack, + frames: payload.frames, buffer: packetBuffer, }, nil } diff --git a/packet_packer_test.go b/packet_packer_test.go index 59fa2fb8e..b5a9f6152 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -272,7 +272,7 @@ var _ = Describe("Packet packer", func() { p, err := packer.PackPacket() Expect(err).NotTo(HaveOccurred()) Expect(p).ToNot(BeNil()) - Expect(p.frames[0]).To(Equal(ack)) + Expect(p.ack).To(Equal(ack)) }) It("packs a CONNECTION_CLOSE", func() { @@ -340,7 +340,7 @@ var _ = Describe("Packet packer", func() { ackFramer.EXPECT().GetAckFrame(protocol.Encryption1RTT).Return(ack) p, err := packer.MaybePackAckPacket() Expect(err).NotTo(HaveOccurred()) - Expect(p.frames).To(Equal([]wire.Frame{ack})) + Expect(p.ack).To(Equal(ack)) }) }) @@ -356,7 +356,8 @@ var _ = Describe("Packet packer", func() { p, err := packer.PackPacket() Expect(p).ToNot(BeNil()) Expect(err).ToNot(HaveOccurred()) - Expect(p.frames).To(HaveLen(1)) + Expect(p.ack).ToNot(BeNil()) + Expect(p.frames).To(BeEmpty()) } } @@ -382,7 +383,8 @@ var _ = Describe("Packet packer", func() { p, err = packer.PackPacket() Expect(p).ToNot(BeNil()) Expect(err).ToNot(HaveOccurred()) - Expect(p.frames).To(HaveLen(1)) + Expect(p.ack).ToNot(BeNil()) + Expect(p.frames).To(BeEmpty()) }) It("waits until there's something to send before adding a PING frame", func() { @@ -402,14 +404,15 @@ var _ = Describe("Packet packer", func() { pnManager.EXPECT().PeekPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2) pnManager.EXPECT().PopPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x42)) sealingManager.EXPECT().GetSealer().Return(protocol.Encryption1RTT, sealer) - ackFramer.EXPECT().GetAckFrame(protocol.Encryption1RTT).Return(&wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 1, Largest: 1}}}) + ack := &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 1, Largest: 1}}} + ackFramer.EXPECT().GetAckFrame(protocol.Encryption1RTT).Return(ack) p, err = packer.PackPacket() Expect(err).ToNot(HaveOccurred()) - Expect(p.frames).To(HaveLen(2)) - Expect(p.frames).To(ContainElement(&wire.PingFrame{})) + Expect(p.ack).To(Equal(ack)) + Expect(p.frames).To(Equal([]wire.Frame{&wire.PingFrame{}})) }) - It("doesn't send a PING if it already sent another ack-elicitng frame", func() { + It("doesn't send a PING if it already sent another ack-eliciting frame", func() { sendMaxNumNonAckElicitingAcks() pnManager.EXPECT().PeekPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2) pnManager.EXPECT().PopPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x42)) @@ -746,7 +749,7 @@ var _ = Describe("Packet packer", func() { checkLength(p.raw) }) - It("sends a Initial packet containing only an ACK", func() { + It("sends an Initial packet containing only an ACK", func() { ack := &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 10, Largest: 20}}} ackFramer.EXPECT().GetAckFrame(protocol.EncryptionInitial).Return(ack) initialStream.EXPECT().HasData() @@ -755,7 +758,7 @@ var _ = Describe("Packet packer", func() { pnManager.EXPECT().PopPacketNumber(protocol.EncryptionInitial).Return(protocol.PacketNumber(0x42)) p, err := packer.PackPacket() Expect(err).ToNot(HaveOccurred()) - Expect(p.frames).To(Equal([]wire.Frame{ack})) + Expect(p.ack).To(Equal(ack)) }) It("sends a Handshake packet containing only an ACK", func() { @@ -769,7 +772,7 @@ var _ = Describe("Packet packer", func() { pnManager.EXPECT().PopPacketNumber(protocol.EncryptionHandshake).Return(protocol.PacketNumber(0x42)) p, err := packer.PackPacket() Expect(err).ToNot(HaveOccurred()) - Expect(p.frames).To(Equal([]wire.Frame{ack})) + Expect(p.ack).To(Equal(ack)) }) It("pads Initial packets to the required minimum packet size", func() { @@ -860,8 +863,8 @@ var _ = Describe("Packet packer", func() { packet, err := packer.PackPacket() Expect(err).ToNot(HaveOccurred()) Expect(packet.raw).To(HaveLen(protocol.MinInitialPacketSize)) - Expect(packet.frames).To(HaveLen(2)) - Expect(packet.frames[0]).To(Equal(ack)) + Expect(packet.ack).To(Equal(ack)) + Expect(packet.frames).To(HaveLen(1)) }) Context("retransmitions", func() {