From 5225a104d4837467d7c385bc43a98a64051cf0ab Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sun, 4 Jun 2023 15:28:16 +0300 Subject: [PATCH] retransmission queue: simplify queueing of PING frames --- connection.go | 12 +------- packet_packer_test.go | 28 +++++++++--------- retransmission_queue.go | 28 ++++++++++++++---- retransmission_queue_test.go | 56 ++++++++++++++++++++++++------------ 4 files changed, 74 insertions(+), 50 deletions(-) diff --git a/connection.go b/connection.go index 54bc61b3..5aa8d97f 100644 --- a/connection.go +++ b/connection.go @@ -1971,17 +1971,7 @@ func (s *connection) sendProbePacket(encLevel protocol.EncryptionLevel, now time } } if packet == nil { - //nolint:exhaustive // Cannot send probe packets for 0-RTT. - switch encLevel { - case protocol.EncryptionInitial: - s.retransmissionQueue.AddInitial(&wire.PingFrame{}) - case protocol.EncryptionHandshake: - s.retransmissionQueue.AddHandshake(&wire.PingFrame{}) - case protocol.Encryption1RTT: - s.retransmissionQueue.AddAppData(&wire.PingFrame{}) - default: - panic("unexpected encryption level") - } + s.retransmissionQueue.AddPing(encLevel) var err error packet, err = s.packer.MaybePackProbePacket(encLevel, s.mtuDiscoverer.CurrentSize(), s.version) if err != nil { diff --git a/packet_packer_test.go b/packet_packer_test.go index 74953dbb..cc277e0c 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -647,7 +647,7 @@ var _ = Describe("Packet packer", func() { sealingManager.EXPECT().GetInitialSealer().Return(nil, handshake.ErrKeysDropped) sealingManager.EXPECT().GetHandshakeSealer().Return(sealer, nil) sealingManager.EXPECT().Get1RTTSealer().Return(nil, handshake.ErrKeysNotYetAvailable) - packer.retransmissionQueue.AddHandshake(&wire.PingFrame{}) + packer.retransmissionQueue.addHandshake(&wire.PingFrame{}) handshakeStream.EXPECT().HasData() ackFramer.EXPECT().GetAckFrame(protocol.EncryptionHandshake, false) packet, err := packer.PackCoalescedPacket(false, maxPacketSize, protocol.Version1) @@ -978,7 +978,7 @@ var _ = Describe("Packet packer", func() { return &wire.CryptoFrame{Offset: 0x42, Data: []byte("initial")} }) handshakeStream.EXPECT().HasData() - packer.retransmissionQueue.AddHandshake(&wire.PingFrame{}) + packer.retransmissionQueue.addHandshake(&wire.PingFrame{}) p, err := packer.PackCoalescedPacket(false, maxPacketSize, protocol.Version1) Expect(err).ToNot(HaveOccurred()) Expect(p.buffer.Len()).To(BeEquivalentTo(maxPacketSize)) @@ -1008,8 +1008,8 @@ var _ = Describe("Packet packer", func() { ackFramer.EXPECT().GetAckFrame(protocol.EncryptionInitial, gomock.Any()) initialStream.EXPECT().HasData() handshakeStream.EXPECT().HasData() - packer.retransmissionQueue.AddInitial(&wire.PingFrame{}) - packer.retransmissionQueue.AddHandshake(&wire.PingFrame{}) + packer.retransmissionQueue.addInitial(&wire.PingFrame{}) + packer.retransmissionQueue.addHandshake(&wire.PingFrame{}) p, err := packer.PackCoalescedPacket(false, maxPacketSize, protocol.Version1) Expect(err).ToNot(HaveOccurred()) Expect(p.buffer.Len()).To(BeEquivalentTo(maxPacketSize)) @@ -1043,7 +1043,7 @@ var _ = Describe("Packet packer", func() { expectAppendControlFrames() expectAppendStreamFrames() framer.EXPECT().HasData().Return(true) - packer.retransmissionQueue.AddAppData(&wire.PingFrame{}) + packer.retransmissionQueue.addAppData(&wire.PingFrame{}) p, err := packer.PackCoalescedPacket(false, maxPacketSize, protocol.Version1) Expect(err).ToNot(HaveOccurred()) Expect(p.buffer.Len()).To(BeEquivalentTo(maxPacketSize)) @@ -1166,7 +1166,7 @@ var _ = Describe("Packet packer", func() { sealingManager.EXPECT().GetInitialSealer().Return(nil, handshake.ErrKeysDropped) sealingManager.EXPECT().GetHandshakeSealer().Return(sealer, nil) sealingManager.EXPECT().Get1RTTSealer().Return(nil, handshake.ErrKeysNotYetAvailable) - packer.retransmissionQueue.AddHandshake(&wire.PingFrame{}) + packer.retransmissionQueue.addHandshake(&wire.PingFrame{}) handshakeStream.EXPECT().HasData() ackFramer.EXPECT().GetAckFrame(protocol.EncryptionHandshake, false) packet, err := packer.PackCoalescedPacket(false, maxPacketSize, protocol.Version1) @@ -1201,8 +1201,8 @@ var _ = Describe("Packet packer", func() { It("adds retransmissions", func() { f := &wire.CryptoFrame{Data: []byte("Initial")} - retransmissionQueue.AddInitial(f) - retransmissionQueue.AddHandshake(&wire.CryptoFrame{Data: []byte("Handshake")}) + retransmissionQueue.addInitial(f) + retransmissionQueue.addHandshake(&wire.CryptoFrame{Data: []byte("Handshake")}) pnManager.EXPECT().PeekPacketNumber(protocol.EncryptionInitial).Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2) pnManager.EXPECT().PopPacketNumber(protocol.EncryptionInitial).Return(protocol.PacketNumber(0x42)) sealingManager.EXPECT().GetInitialSealer().Return(getSealer(), nil) @@ -1320,7 +1320,7 @@ var _ = Describe("Packet packer", func() { It(fmt.Sprintf("packs an Initial probe packet and pads it, for the %s", perspective), func() { packer.perspective = perspective f := &wire.CryptoFrame{Data: []byte("Initial")} - retransmissionQueue.AddInitial(f) + retransmissionQueue.addInitial(f) sealingManager.EXPECT().GetInitialSealer().Return(getSealer(), nil) ackFramer.EXPECT().GetAckFrame(protocol.EncryptionInitial, false) initialStream.EXPECT().HasData() @@ -1342,7 +1342,7 @@ var _ = Describe("Packet packer", func() { It(fmt.Sprintf("packs an Initial probe packet with 1 byte payload, for the %s", perspective), func() { packer.perspective = perspective - retransmissionQueue.AddInitial(&wire.PingFrame{}) + retransmissionQueue.addInitial(&wire.PingFrame{}) sealingManager.EXPECT().GetInitialSealer().Return(getSealer(), nil) ackFramer.EXPECT().GetAckFrame(protocol.EncryptionInitial, false) initialStream.EXPECT().HasData() @@ -1366,7 +1366,7 @@ var _ = Describe("Packet packer", func() { It("packs a Handshake probe packet", func() { f := &wire.CryptoFrame{Data: []byte("Handshake")} - retransmissionQueue.AddHandshake(f) + retransmissionQueue.addHandshake(f) sealingManager.EXPECT().GetHandshakeSealer().Return(getSealer(), nil) ackFramer.EXPECT().GetAckFrame(protocol.EncryptionHandshake, false) handshakeStream.EXPECT().HasData() @@ -1387,7 +1387,7 @@ var _ = Describe("Packet packer", func() { It("packs a full size Handshake probe packet", func() { f := &wire.CryptoFrame{Data: make([]byte, 2000)} - retransmissionQueue.AddHandshake(f) + retransmissionQueue.addHandshake(f) sealingManager.EXPECT().GetHandshakeSealer().Return(getSealer(), nil) ackFramer.EXPECT().GetAckFrame(protocol.EncryptionHandshake, false) handshakeStream.EXPECT().HasData() @@ -1409,7 +1409,7 @@ var _ = Describe("Packet packer", func() { It("packs a 1-RTT probe packet", func() { f := &wire.StreamFrame{Data: []byte("1-RTT")} - retransmissionQueue.AddInitial(f) + retransmissionQueue.addInitial(f) sealingManager.EXPECT().Get1RTTSealer().Return(getSealer(), nil) ackFramer.EXPECT().GetAckFrame(protocol.Encryption1RTT, false) pnManager.EXPECT().PeekPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2) @@ -1431,7 +1431,7 @@ var _ = Describe("Packet packer", func() { It("packs a full size 1-RTT probe packet", func() { f := &wire.StreamFrame{Data: make([]byte, 2000)} - retransmissionQueue.AddInitial(f) + retransmissionQueue.addInitial(f) sealingManager.EXPECT().Get1RTTSealer().Return(getSealer(), nil) ackFramer.EXPECT().GetAckFrame(protocol.Encryption1RTT, false) pnManager.EXPECT().PeekPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2) diff --git a/retransmission_queue.go b/retransmission_queue.go index d5e844d0..909ad622 100644 --- a/retransmission_queue.go +++ b/retransmission_queue.go @@ -23,7 +23,23 @@ func newRetransmissionQueue() *retransmissionQueue { return &retransmissionQueue{} } -func (q *retransmissionQueue) AddInitial(f wire.Frame) { +// AddPing queues a ping. +// It is used when a probe packet needs to be sent +func (q *retransmissionQueue) AddPing(encLevel protocol.EncryptionLevel) { + //nolint:exhaustive // Cannot send probe packets for 0-RTT. + switch encLevel { + case protocol.EncryptionInitial: + q.addInitial(&wire.PingFrame{}) + case protocol.EncryptionHandshake: + q.addHandshake(&wire.PingFrame{}) + case protocol.Encryption1RTT: + q.addAppData(&wire.PingFrame{}) + default: + panic("unexpected encryption level") + } +} + +func (q *retransmissionQueue) addInitial(f wire.Frame) { if cf, ok := f.(*wire.CryptoFrame); ok { q.initialCryptoData = append(q.initialCryptoData, cf) return @@ -31,7 +47,7 @@ func (q *retransmissionQueue) AddInitial(f wire.Frame) { q.initial = append(q.initial, f) } -func (q *retransmissionQueue) AddHandshake(f wire.Frame) { +func (q *retransmissionQueue) addHandshake(f wire.Frame) { if cf, ok := f.(*wire.CryptoFrame); ok { q.handshakeCryptoData = append(q.handshakeCryptoData, cf) return @@ -51,7 +67,7 @@ func (q *retransmissionQueue) HasAppData() bool { return len(q.appData) > 0 } -func (q *retransmissionQueue) AddAppData(f wire.Frame) { +func (q *retransmissionQueue) addAppData(f wire.Frame) { if _, ok := f.(*wire.StreamFrame); ok { panic("STREAM frames are handled with their respective streams.") } @@ -146,19 +162,19 @@ type retransmissionQueueInitialAckHandler retransmissionQueue func (q *retransmissionQueueInitialAckHandler) OnAcked(wire.Frame) {} func (q *retransmissionQueueInitialAckHandler) OnLost(f wire.Frame) { - (*retransmissionQueue)(q).AddInitial(f) + (*retransmissionQueue)(q).addInitial(f) } type retransmissionQueueHandshakeAckHandler retransmissionQueue func (q *retransmissionQueueHandshakeAckHandler) OnAcked(wire.Frame) {} func (q *retransmissionQueueHandshakeAckHandler) OnLost(f wire.Frame) { - (*retransmissionQueue)(q).AddHandshake(f) + (*retransmissionQueue)(q).addHandshake(f) } type retransmissionQueueAppDataAckHandler retransmissionQueue func (q *retransmissionQueueAppDataAckHandler) OnAcked(wire.Frame) {} func (q *retransmissionQueueAppDataAckHandler) OnLost(f wire.Frame) { - (*retransmissionQueue)(q).AddAppData(f) + (*retransmissionQueue)(q).addAppData(f) } diff --git a/retransmission_queue_test.go b/retransmission_queue_test.go index 0eaad18c..c0132848 100644 --- a/retransmission_queue_test.go +++ b/retransmission_queue_test.go @@ -23,7 +23,7 @@ var _ = Describe("Retransmission queue", func() { It("queues and retrieves a control frame", func() { f := &wire.MaxDataFrame{MaximumData: 0x42} - q.AddInitial(f) + q.addInitial(f) Expect(q.HasInitialData()).To(BeTrue()) Expect(q.GetInitialFrame(f.Length(protocol.Version1)-1, protocol.Version1)).To(BeNil()) Expect(q.GetInitialFrame(f.Length(protocol.Version1), protocol.Version1)).To(Equal(f)) @@ -32,7 +32,7 @@ var _ = Describe("Retransmission queue", func() { It("queues and retrieves a CRYPTO frame", func() { f := &wire.CryptoFrame{Data: []byte("foobar")} - q.AddInitial(f) + q.addInitial(f) Expect(q.HasInitialData()).To(BeTrue()) Expect(q.GetInitialFrame(f.Length(protocol.Version1), protocol.Version1)).To(Equal(f)) Expect(q.HasInitialData()).To(BeFalse()) @@ -43,7 +43,7 @@ var _ = Describe("Retransmission queue", func() { Offset: 100, Data: []byte("foobar"), } - q.AddInitial(f) + q.addInitial(f) Expect(q.HasInitialData()).To(BeTrue()) f1 := q.GetInitialFrame(f.Length(protocol.Version1)-3, protocol.Version1) Expect(f1).ToNot(BeNil()) @@ -61,8 +61,8 @@ var _ = Describe("Retransmission queue", func() { It("returns other frames when a CRYPTO frame wouldn't fit", func() { f := &wire.CryptoFrame{Data: []byte("foobar")} - q.AddInitial(f) - q.AddInitial(&wire.PingFrame{}) + q.addInitial(f) + q.addInitial(&wire.PingFrame{}) f1 := q.GetInitialFrame(2, protocol.Version1) // too small for a CRYPTO frame Expect(f1).ToNot(BeNil()) Expect(f1).To(BeAssignableToTypeOf(&wire.PingFrame{})) @@ -74,8 +74,8 @@ var _ = Describe("Retransmission queue", func() { It("retrieves both a CRYPTO frame and a control frame", func() { cf := &wire.MaxDataFrame{MaximumData: 0x42} f := &wire.CryptoFrame{Data: []byte("foobar")} - q.AddInitial(f) - q.AddInitial(cf) + q.addInitial(f) + q.addInitial(cf) Expect(q.HasInitialData()).To(BeTrue()) Expect(q.GetInitialFrame(protocol.MaxByteCount, protocol.Version1)).To(Equal(f)) Expect(q.GetInitialFrame(protocol.MaxByteCount, protocol.Version1)).To(Equal(cf)) @@ -83,8 +83,8 @@ var _ = Describe("Retransmission queue", func() { }) It("drops all Initial frames", func() { - q.AddInitial(&wire.CryptoFrame{Data: []byte("foobar")}) - q.AddInitial(&wire.MaxDataFrame{MaximumData: 0x42}) + q.addInitial(&wire.CryptoFrame{Data: []byte("foobar")}) + q.addInitial(&wire.MaxDataFrame{MaximumData: 0x42}) q.DropPackets(protocol.EncryptionInitial) Expect(q.HasInitialData()).To(BeFalse()) Expect(q.GetInitialFrame(protocol.MaxByteCount, protocol.Version1)).To(BeNil()) @@ -96,6 +96,12 @@ var _ = Describe("Retransmission queue", func() { Expect(q.HasInitialData()).To(BeTrue()) Expect(q.GetInitialFrame(protocol.MaxByteCount, protocol.Version1)).To(Equal(f)) }) + + It("adds a PING", func() { + q.AddPing(protocol.EncryptionInitial) + Expect(q.HasInitialData()).To(BeTrue()) + Expect(q.GetInitialFrame(protocol.MaxByteCount, protocol.Version1)).To(Equal(&wire.PingFrame{})) + }) }) Context("Handshake data", func() { @@ -106,7 +112,7 @@ var _ = Describe("Retransmission queue", func() { It("queues and retrieves a control frame", func() { f := &wire.MaxDataFrame{MaximumData: 0x42} - q.AddHandshake(f) + q.addHandshake(f) Expect(q.HasHandshakeData()).To(BeTrue()) Expect(q.GetHandshakeFrame(f.Length(protocol.Version1)-1, protocol.Version1)).To(BeNil()) Expect(q.GetHandshakeFrame(f.Length(protocol.Version1), protocol.Version1)).To(Equal(f)) @@ -115,7 +121,7 @@ var _ = Describe("Retransmission queue", func() { It("queues and retrieves a CRYPTO frame", func() { f := &wire.CryptoFrame{Data: []byte("foobar")} - q.AddHandshake(f) + q.addHandshake(f) Expect(q.HasHandshakeData()).To(BeTrue()) Expect(q.GetHandshakeFrame(f.Length(protocol.Version1), protocol.Version1)).To(Equal(f)) Expect(q.HasHandshakeData()).To(BeFalse()) @@ -126,7 +132,7 @@ var _ = Describe("Retransmission queue", func() { Offset: 100, Data: []byte("foobar"), } - q.AddHandshake(f) + q.addHandshake(f) Expect(q.HasHandshakeData()).To(BeTrue()) f1 := q.GetHandshakeFrame(f.Length(protocol.Version1)-3, protocol.Version1) Expect(f1).ToNot(BeNil()) @@ -144,8 +150,8 @@ var _ = Describe("Retransmission queue", func() { It("returns other frames when a CRYPTO frame wouldn't fit", func() { f := &wire.CryptoFrame{Data: []byte("foobar")} - q.AddHandshake(f) - q.AddHandshake(&wire.PingFrame{}) + q.addHandshake(f) + q.addHandshake(&wire.PingFrame{}) f1 := q.GetHandshakeFrame(2, protocol.Version1) // too small for a CRYPTO frame Expect(f1).ToNot(BeNil()) Expect(f1).To(BeAssignableToTypeOf(&wire.PingFrame{})) @@ -157,8 +163,8 @@ var _ = Describe("Retransmission queue", func() { It("retrieves both a CRYPTO frame and a control frame", func() { cf := &wire.MaxDataFrame{MaximumData: 0x42} f := &wire.CryptoFrame{Data: []byte("foobar")} - q.AddHandshake(f) - q.AddHandshake(cf) + q.addHandshake(f) + q.addHandshake(cf) Expect(q.HasHandshakeData()).To(BeTrue()) Expect(q.GetHandshakeFrame(protocol.MaxByteCount, protocol.Version1)).To(Equal(f)) Expect(q.GetHandshakeFrame(protocol.MaxByteCount, protocol.Version1)).To(Equal(cf)) @@ -166,8 +172,8 @@ var _ = Describe("Retransmission queue", func() { }) It("drops all Handshake frames", func() { - q.AddHandshake(&wire.CryptoFrame{Data: []byte("foobar")}) - q.AddHandshake(&wire.MaxDataFrame{MaximumData: 0x42}) + q.addHandshake(&wire.CryptoFrame{Data: []byte("foobar")}) + q.addHandshake(&wire.MaxDataFrame{MaximumData: 0x42}) q.DropPackets(protocol.EncryptionHandshake) Expect(q.HasHandshakeData()).To(BeFalse()) Expect(q.GetHandshakeFrame(protocol.MaxByteCount, protocol.Version1)).To(BeNil()) @@ -179,6 +185,12 @@ var _ = Describe("Retransmission queue", func() { Expect(q.HasHandshakeData()).To(BeTrue()) Expect(q.GetHandshakeFrame(protocol.MaxByteCount, protocol.Version1)).To(Equal(f)) }) + + It("adds a PING", func() { + q.AddPing(protocol.EncryptionHandshake) + Expect(q.HasHandshakeData()).To(BeTrue()) + Expect(q.GetHandshakeFrame(protocol.MaxByteCount, protocol.Version1)).To(Equal(&wire.PingFrame{})) + }) }) Context("Application data", func() { @@ -189,7 +201,7 @@ var _ = Describe("Retransmission queue", func() { It("queues and retrieves a control frame", func() { f := &wire.MaxDataFrame{MaximumData: 0x42} Expect(q.HasAppData()).To(BeFalse()) - q.AddAppData(f) + q.addAppData(f) Expect(q.HasAppData()).To(BeTrue()) Expect(q.GetAppDataFrame(f.Length(protocol.Version1)-1, protocol.Version1)).To(BeNil()) Expect(q.GetAppDataFrame(f.Length(protocol.Version1), protocol.Version1)).To(Equal(f)) @@ -202,5 +214,11 @@ var _ = Describe("Retransmission queue", func() { Expect(q.HasAppData()).To(BeTrue()) Expect(q.GetAppDataFrame(protocol.MaxByteCount, protocol.Version1)).To(Equal(f)) }) + + It("adds a PING", func() { + q.AddPing(protocol.Encryption1RTT) + Expect(q.HasAppData()).To(BeTrue()) + Expect(q.GetAppDataFrame(protocol.MaxByteCount, protocol.Version1)).To(Equal(&wire.PingFrame{})) + }) }) })