when forcing a retransmittable packet, bundle the PING with other frames

We're sending a retransmittable packet every 20 packets (if there are no
other frames to send). To make a packet retransmittable, we add a PING
frame. We should bundle this PING with an ACK.

packet_packer.go

@@ -33,6 +33,7 @@ type packetPacker struct {
 	leastUnacked                  protocol.PacketNumber
 	omitConnectionID              bool
 	hasSentPacket                 bool // has the packetPacker already sent a packet
+	makeNextPacketRetransmittable bool
 }
 
 func newPacketPacker(connectionID protocol.ConnectionID,
@@ -153,6 +154,15 @@ func (p *packetPacker) PackPacket() (*packedPacket, error) {
 	if len(payloadFrames) == 1 && p.stopWaiting != nil {
 		return nil, nil
 	}
+	// check if this packet only contains an ACK and / or STOP_WAITING
+	if !ackhandler.HasRetransmittableFrames(payloadFrames) {
+		if p.makeNextPacketRetransmittable {
+			payloadFrames = append(payloadFrames, &wire.PingFrame{})
+			p.makeNextPacketRetransmittable = false
+		}
+	} else { // this packet already contains a retransmittable frame. No need to send a PING
+		p.makeNextPacketRetransmittable = false
+	}
 	p.stopWaiting = nil
 	p.ackFrame = nil
 
@@ -369,3 +379,7 @@ func (p *packetPacker) SetLeastUnacked(leastUnacked protocol.PacketNumber) {
 func (p *packetPacker) SetOmitConnectionID() {
 	p.omitConnectionID = true
 }
+
+func (p *packetPacker) MakeNextPacketRetransmittable() {
+	p.makeNextPacketRetransmittable = true
+}

packet_packer_test.go

@@ -361,6 +361,49 @@ var _ = Describe("Packet packer", func() {
 		Expect(packer.packetNumberGenerator.Peek()).To(Equal(protocol.PacketNumber(2)))
 	})
 
+	It("adds a PING frame when it's supposed to send a retransmittable packet", func() {
+		packer.QueueControlFrame(&wire.AckFrame{})
+		packer.QueueControlFrame(&wire.StopWaitingFrame{})
+		packer.MakeNextPacketRetransmittable()
+		p, err := packer.PackPacket()
+		Expect(p).ToNot(BeNil())
+		Expect(err).ToNot(HaveOccurred())
+		Expect(p.frames).To(HaveLen(3))
+		Expect(p.frames).To(ContainElement(&wire.PingFrame{}))
+		// make sure the next packet doesn't contain another PING
+		packer.QueueControlFrame(&wire.AckFrame{})
+		p, err = packer.PackPacket()
+		Expect(p).ToNot(BeNil())
+		Expect(err).ToNot(HaveOccurred())
+		Expect(p.frames).To(HaveLen(1))
+	})
+
+	It("waits until there's something to send before adding a PING frame", func() {
+		packer.MakeNextPacketRetransmittable()
+		p, err := packer.PackPacket()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(p).To(BeNil())
+		packer.QueueControlFrame(&wire.AckFrame{})
+		p, err = packer.PackPacket()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(p.frames).To(HaveLen(2))
+		Expect(p.frames).To(ContainElement(&wire.PingFrame{}))
+	})
+
+	It("doesn't send a PING if it already sent another retransmittable frame", func() {
+		packer.MakeNextPacketRetransmittable()
+		packer.QueueControlFrame(&wire.MaxDataFrame{})
+		p, err := packer.PackPacket()
+		Expect(p).ToNot(BeNil())
+		Expect(err).ToNot(HaveOccurred())
+		Expect(p.frames).To(HaveLen(1))
+		packer.QueueControlFrame(&wire.AckFrame{})
+		p, err = packer.PackPacket()
+		Expect(p).ToNot(BeNil())
+		Expect(err).ToNot(HaveOccurred())
+		Expect(p.frames).To(HaveLen(1))
+	})
+
 	Context("STREAM Frame handling", func() {
 		It("does not splits a STREAM frame with maximum size, for gQUIC frames", func() {
 			f := &wire.StreamFrame{

session.go

@@ -766,7 +766,7 @@ func (s *session) sendPacket() error {
 		}
 		// add a retransmittable frame
 		if s.sentPacketHandler.ShouldSendRetransmittablePacket() {
-			s.packer.QueueControlFrame(&wire.PingFrame{})
+			s.packer.MakeNextPacketRetransmittable()
 		}
 		packet, err := s.packer.PackPacket()
 		if err != nil || packet == nil {

session_test.go

@@ -824,6 +824,7 @@ var _ = Describe("Session", func() {
 
 		It("sends a retransmittable packet when required by the SentPacketHandler", func() {
 			sess.sentPacketHandler = &mockSentPacketHandler{shouldSendRetransmittablePacket: true}
+			sess.packer.QueueControlFrame(&wire.AckFrame{LargestAcked: 1000})
 			err := sess.sendPacket()
 			Expect(err).ToNot(HaveOccurred())
 			Expect(mconn.written).To(HaveLen(1))