handle ACK frames separately when packing packets

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,
+		Ack:             p.ack,
-		Frames:          frames,
+		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
+	// check if we have anything to send
-	if len(payload.frames) == 0 {
+	if len(payload.frames) == 0 && payload.ack == nil {
 		return nil, nil
 	}
-	// check if this packet only contains an ACK
+	if len(payload.frames) == 0 { // the packet only contains an ACK
-	if !ackhandler.HasAckElicitingFrames(payload.frames) {
 		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)
+		cf := s.PopCryptoFrame(p.maxPacketSize - hdrLen - protocol.ByteCount(sealer.Overhead()) - payload.length)
-		frames = append(frames, cf)
+		payload.frames = []wire.Frame{cf}
-		length += cf.Length(p.version)
+		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 payload payload
-	var frames []wire.Frame
 
 	// 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)
+		payload.ack = ack
-		length += ack.Length(p.version)
+		payload.length += ack.Length(p.version)
 	}
 
-	var lengthAdded protocol.ByteCount
+	frames, lengthAdded := p.framer.AppendControlFrames(payload.frames, maxFrameSize-payload.length)
-	frames, lengthAdded = p.framer.AppendControlFrames(frames, maxFrameSize-length)
+	payload.length += lengthAdded
-	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
 }

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.ack).To(Equal(ack))
-					Expect(p.frames).To(ContainElement(&wire.PingFrame{}))
+					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.ack).To(Equal(ack))
-				Expect(packet.frames[0]).To(Equal(ack))
+				Expect(packet.frames).To(HaveLen(1))
 			})
 
 			Context("retransmitions", func() {