From 7f080dd54bd94d7cb822dba722b3e59f3693d69c Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Mon, 1 Jan 2024 10:17:25 +0700 Subject: [PATCH] discard DATAGRAM frames that don't fit into packets without an ACK (#4221) If the packet doesn't contain an ACK (i.e. the payload is empty), there's no point retrying to pack it later. It's extremely unlikely that the available packet size will suddenly increase. --- interface.go | 8 ++++++-- packet_packer.go | 8 +++++++- packet_packer_test.go | 39 ++++++++++++++++++++++++++++----------- 3 files changed, 41 insertions(+), 14 deletions(-) diff --git a/interface.go b/interface.go index da0e5e2b..b269d790 100644 --- a/interface.go +++ b/interface.go @@ -187,8 +187,12 @@ type Connection interface { // Warning: This API should not be considered stable and might change soon. ConnectionState() ConnectionState - // SendDatagram sends a message as a datagram, as specified in RFC 9221. - SendDatagram([]byte) error + // SendDatagram sends a message using a QUIC datagram, as specified in RFC 9221. + // There is no delivery guarantee for DATAGRAM frames, they are not retransmitted if lost. + // The payload of the datagram needs to fit into a single QUIC packet. + // In addition, a datagram may be dropped before being sent out if the available packet size suddenly decreases. + // If the payload is too large to be sent at the current time, a DatagramTooLargeError is returned. + SendDatagram(payload []byte) error // ReceiveDatagram gets a message received in a datagram, as specified in RFC 9221. ReceiveDatagram(context.Context) ([]byte, error) } diff --git a/packet_packer.go b/packet_packer.go index a330632b..9a972952 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -606,11 +606,17 @@ func (p *packetPacker) composeNextPacket(maxFrameSize protocol.ByteCount, onlyAc if p.datagramQueue != nil { if f := p.datagramQueue.Peek(); f != nil { size := f.Length(v) - if size <= maxFrameSize-pl.length { + if size <= maxFrameSize-pl.length { // DATAGRAM frame fits pl.frames = append(pl.frames, ackhandler.Frame{Frame: f}) pl.length += size p.datagramQueue.Pop() + } else if !hasAck { + // The DATAGRAM frame doesn't fit, and the packet doesn't contain an ACK. + // Discard this frame. There's no point in retrying this in the next packet, + // as it's unlikely that the available packet size will increase. + p.datagramQueue.Pop() } + // If the DATAGRAM frame was too large and the packet contained an ACK, we'll try to send it out later. } } diff --git a/packet_packer_test.go b/packet_packer_test.go index fe746e63..677760ad 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -516,7 +516,6 @@ var _ = Describe("Packet packer", func() { buffer.Data = append(buffer.Data, []byte("foobar")...) p, err := packer.AppendPacket(buffer, maxPacketSize, protocol.Version1) Expect(err).ToNot(HaveOccurred()) - Expect(p).ToNot(BeNil()) b, err := f.Append(nil, protocol.Version1) Expect(err).ToNot(HaveOccurred()) Expect(p.Frames).To(BeEmpty()) @@ -535,7 +534,6 @@ var _ = Describe("Packet packer", func() { sealingManager.EXPECT().Get1RTTSealer().Return(getSealer(), nil) p, err := packer.AppendPacket(getPacketBuffer(), maxPacketSize, protocol.Version1) Expect(err).NotTo(HaveOccurred()) - Expect(p).ToNot(BeNil()) Expect(p.Ack).To(Equal(ack)) }) @@ -553,7 +551,6 @@ var _ = Describe("Packet packer", func() { expectAppendStreamFrames() buffer := getPacketBuffer() p, err := packer.AppendPacket(buffer, maxPacketSize, protocol.Version1) - Expect(p).ToNot(BeNil()) Expect(err).ToNot(HaveOccurred()) Expect(p.Frames).To(HaveLen(2)) for i, f := range p.Frames { @@ -577,7 +574,6 @@ var _ = Describe("Packet packer", func() { expectAppendStreamFrames() buffer := getPacketBuffer() p, err := packer.AppendPacket(buffer, maxPacketSize, protocol.Version1) - Expect(p).ToNot(BeNil()) Expect(err).ToNot(HaveOccurred()) Expect(p.Frames).To(HaveLen(3)) for i, f := range p.Frames { @@ -614,7 +610,6 @@ var _ = Describe("Packet packer", func() { framer.EXPECT().HasData() buffer := getPacketBuffer() p, err := packer.AppendPacket(buffer, maxPacketSize, protocol.Version1) - Expect(p).ToNot(BeNil()) Expect(err).ToNot(HaveOccurred()) Expect(p.Frames).To(HaveLen(1)) Expect(p.Frames[0].Frame).To(Equal(f)) @@ -643,15 +638,42 @@ var _ = Describe("Packet packer", func() { framer.EXPECT().HasData() buffer := getPacketBuffer() p, err := packer.AppendPacket(buffer, maxPacketSize, protocol.Version1) - Expect(p).ToNot(BeNil()) Expect(err).ToNot(HaveOccurred()) Expect(p.Ack).ToNot(BeNil()) Expect(p.Frames).To(BeEmpty()) Expect(buffer.Data).ToNot(BeEmpty()) + Expect(datagramQueue.Peek()).To(Equal(f)) // make sure the frame is still there datagramQueue.CloseWithError(nil) Eventually(done).Should(BeClosed()) }) + It("discards a DATAGRAM frame if it doesn't fit into a packet that doesn't contain an ACK", func() { + ackFramer.EXPECT().GetAckFrame(protocol.Encryption1RTT, true) + pnManager.EXPECT().PeekPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2) + sealingManager.EXPECT().Get1RTTSealer().Return(getSealer(), nil) + f := &wire.DatagramFrame{ + DataLenPresent: true, + Data: make([]byte, maxPacketSize+10), // won't fit + } + done := make(chan struct{}) + go func() { + defer GinkgoRecover() + defer close(done) + datagramQueue.AddAndWait(f) + }() + // make sure the DATAGRAM has actually been queued + time.Sleep(scaleDuration(20 * time.Millisecond)) + + framer.EXPECT().HasData() + buffer := getPacketBuffer() + p, err := packer.AppendPacket(buffer, maxPacketSize, protocol.Version1) + Expect(err).To(MatchError(errNothingToPack)) + Expect(p.Frames).To(BeEmpty()) + Expect(p.Ack).To(BeNil()) + Expect(datagramQueue.Peek()).To(BeNil()) + Eventually(done).Should(BeClosed()) + }) + It("accounts for the space consumed by control frames", func() { pnManager.EXPECT().PeekPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2) sealingManager.EXPECT().Get1RTTSealer().Return(getSealer(), nil) @@ -777,7 +799,6 @@ var _ = Describe("Packet packer", func() { expectAppendControlFrames() expectAppendStreamFrames(ackhandler.StreamFrame{Frame: f1}, ackhandler.StreamFrame{Frame: f2}, ackhandler.StreamFrame{Frame: f3}) p, err := packer.AppendPacket(getPacketBuffer(), maxPacketSize, protocol.Version1) - Expect(p).ToNot(BeNil()) Expect(err).ToNot(HaveOccurred()) Expect(p.Frames).To(BeEmpty()) Expect(p.StreamFrames).To(HaveLen(3)) @@ -797,7 +818,6 @@ var _ = Describe("Packet packer", func() { expectAppendControlFrames() expectAppendStreamFrames() p, err := packer.AppendPacket(getPacketBuffer(), maxPacketSize, protocol.Version1) - Expect(p).ToNot(BeNil()) Expect(err).ToNot(HaveOccurred()) Expect(p.Ack).ToNot(BeNil()) Expect(p.Frames).To(BeEmpty()) @@ -814,7 +834,6 @@ var _ = Describe("Packet packer", func() { expectAppendControlFrames() expectAppendStreamFrames() p, err := packer.AppendPacket(getPacketBuffer(), maxPacketSize, protocol.Version1) - Expect(p).ToNot(BeNil()) Expect(err).ToNot(HaveOccurred()) var hasPing bool for _, f := range p.Frames { @@ -833,7 +852,6 @@ var _ = Describe("Packet packer", func() { expectAppendControlFrames() expectAppendStreamFrames() p, err = packer.AppendPacket(getPacketBuffer(), maxPacketSize, protocol.Version1) - Expect(p).ToNot(BeNil()) Expect(err).ToNot(HaveOccurred()) Expect(p.Ack).ToNot(BeNil()) Expect(p.Frames).To(BeEmpty()) @@ -883,7 +901,6 @@ var _ = Describe("Packet packer", func() { expectAppendControlFrames(ackhandler.Frame{Frame: &wire.MaxDataFrame{}}) p, err := packer.AppendPacket(getPacketBuffer(), maxPacketSize, protocol.Version1) Expect(err).ToNot(HaveOccurred()) - Expect(p).ToNot(BeNil()) Expect(p.Frames).ToNot(ContainElement(&wire.PingFrame{})) }) })