Skip to content

Commit

Permalink
discard DATAGRAM frames that don't fit into packets without an ACK (q…
Browse files Browse the repository at this point in the history 
…uic-go#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.
  • Loading branch information
@marten-seemann
marten-seemann authored and Constantine Shablia committed Feb 1, 2024
1 parent 1b77739 commit 1c961c8
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 14 deletions.
8 changes: 6 additions & 2 deletions interface.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
8 changes: 7 additions & 1 deletion packet_packer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}
}

Expand Down
39 changes: 28 additions & 11 deletions packet_packer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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))
})

Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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))
Expand All @@ -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())
Expand All @@ -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 {
Expand All @@ -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())
Expand Down Expand Up @@ -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{}))
})
})
Expand Down

0 comments on commit 1c961c8

Please sign in to comment.