From d8858d767d264c2905661df587a1b9b00e216e4a Mon Sep 17 00:00:00 2001
From: Marten Seemann <martenseemann@gmail.com>
Date: Thu, 17 Dec 2020 11:36:02 +0700
Subject: [PATCH] don't retransmit PING frames added to ACK-only packets

Every 20 non-ack-eliciting packets, we add a PING frame to make that
packet ack-eliciting. That way, we regularly receive acknowledgements,
even if we're not actually sending any data. This allows us to clean up
our sent packet history.
There's no need to retransmit this PING frame though. We'll just send a
new one if one of them is lost, as soon as we've sent another 20
non-ack-eliciting packets.
---
 packet_packer.go      |  3 ++-
 packet_packer_test.go | 18 ++++++++++++++++--
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/packet_packer.go b/packet_packer.go
index 6599a277c..c7fdeb3e2 100644
--- a/packet_packer.go
+++ b/packet_packer.go
@@ -565,7 +565,8 @@ func (p *packetPacker) maybeGetAppDataPacketWithEncLevel(maxPayloadSize protocol
 		// the packet only contains an ACK
 		if p.numNonAckElicitingAcks >= protocol.MaxNonAckElicitingAcks {
 			ping := &wire.PingFrame{}
-			payload.frames = append(payload.frames, ackhandler.Frame{Frame: ping})
+			// don't retransmit the PING frame when it is lost
+			payload.frames = append(payload.frames, ackhandler.Frame{Frame: ping, OnLost: func(wire.Frame) {}})
 			payload.length += ping.Length(p.version)
 			p.numNonAckElicitingAcks = 0
 		} else {
diff --git a/packet_packer_test.go b/packet_packer_test.go
index 9fbe557b5..633138251 100644
--- a/packet_packer_test.go
+++ b/packet_packer_test.go
@@ -728,7 +728,14 @@ var _ = Describe("Packet packer", func() {
 					p, err := packer.PackPacket()
 					Expect(p).ToNot(BeNil())
 					Expect(err).ToNot(HaveOccurred())
-					Expect(p.frames).To(ContainElement(ackhandler.Frame{Frame: &wire.PingFrame{}}))
+					var hasPing bool
+					for _, f := range p.frames {
+						if _, ok := f.Frame.(*wire.PingFrame); ok {
+							hasPing = true
+							Expect(f.OnLost).ToNot(BeNil()) // make sure the PING is not retransmitted if lost
+						}
+					}
+					Expect(hasPing).To(BeTrue())
 					// make sure the next packet doesn't contain another PING
 					pnManager.EXPECT().PeekPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2)
 					pnManager.EXPECT().PopPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x42))
@@ -768,7 +775,14 @@ var _ = Describe("Packet packer", func() {
 					p, err = packer.PackPacket()
 					Expect(err).ToNot(HaveOccurred())
 					Expect(p.ack).To(Equal(ack))
-					Expect(p.frames).To(Equal([]ackhandler.Frame{{Frame: &wire.PingFrame{}}}))
+					var hasPing bool
+					for _, f := range p.frames {
+						if _, ok := f.Frame.(*wire.PingFrame); ok {
+							hasPing = true
+							Expect(f.OnLost).ToNot(BeNil()) // make sure the PING is not retransmitted if lost
+						}
+					}
+					Expect(hasPing).To(BeTrue())
 				})
 
 				It("doesn't send a PING if it already sent another ack-eliciting frame", func() {