From 765d26f132cdaa945ba90275c2f3377abddb851d Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sat, 13 Feb 2021 13:27:29 +0800 Subject: [PATCH] don't reduce the congestion window when a path MTU probe packet is lost --- internal/ackhandler/interfaces.go | 2 ++ internal/ackhandler/sent_packet_handler.go | 8 ++++--- .../ackhandler/sent_packet_handler_test.go | 21 +++++++++++++++++++ packet_packer.go | 16 ++++++++------ packet_packer_test.go | 9 ++++++++ 5 files changed, 47 insertions(+), 9 deletions(-) diff --git a/internal/ackhandler/interfaces.go b/internal/ackhandler/interfaces.go index 22c5e6c1e..ac74f8550 100644 --- a/internal/ackhandler/interfaces.go +++ b/internal/ackhandler/interfaces.go @@ -16,6 +16,8 @@ type Packet struct { EncryptionLevel protocol.EncryptionLevel SendTime time.Time + IsPathMTUProbePacket bool // We don't report the loss of Path MTU probe packets to the congestion controller. + includedInBytesInFlight bool declaredLost bool skippedPacket bool diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index 199f560d1..cfec0c1ed 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -557,11 +557,13 @@ func (h *sentPacketHandler) detectLostPackets(now time.Time, encLevel protocol.E pnSpace.lossTime = lossTime } if packetLost { - h.congestion.OnPacketLost(p.PacketNumber, p.Length, priorInFlight) p.declaredLost = true - h.queueFramesForRetransmission(p) - // the bytes in flight need to be reduced no matter if this packet will be retransmitted + // the bytes in flight need to be reduced no matter if the frames in this packet will be retransmitted h.removeFromBytesInFlight(p) + h.queueFramesForRetransmission(p) + if !p.IsPathMTUProbePacket { + h.congestion.OnPacketLost(p.PacketNumber, p.Length, priorInFlight) + } } return true, nil }) diff --git a/internal/ackhandler/sent_packet_handler_test.go b/internal/ackhandler/sent_packet_handler_test.go index 891a99525..db093584e 100644 --- a/internal/ackhandler/sent_packet_handler_test.go +++ b/internal/ackhandler/sent_packet_handler_test.go @@ -482,6 +482,27 @@ var _ = Describe("SentPacketHandler", func() { Expect(handler.ReceivedAck(ack, protocol.Encryption1RTT, time.Now())).To(Succeed()) }) + It("doesn't call OnPacketLost when a Path MTU probe packet is lost", func() { + cong.EXPECT().OnPacketSent(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(2) + var mtuPacketDeclaredLost bool + handler.SentPacket(ackElicitingPacket(&Packet{ + PacketNumber: 1, + SendTime: time.Now().Add(-time.Hour), + IsPathMTUProbePacket: true, + Frames: []Frame{{Frame: &wire.PingFrame{}, OnLost: func(wire.Frame) { mtuPacketDeclaredLost = true }}}, + })) + handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: 2})) + // lose packet 1, but don't EXPECT any calls to OnPacketLost() + gomock.InOrder( + cong.EXPECT().MaybeExitSlowStart(), + cong.EXPECT().OnPacketAcked(protocol.PacketNumber(2), protocol.ByteCount(1), protocol.ByteCount(2), gomock.Any()), + ) + ack := &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 2, Largest: 2}}} + Expect(handler.ReceivedAck(ack, protocol.Encryption1RTT, time.Now())).To(Succeed()) + Expect(mtuPacketDeclaredLost).To(BeTrue()) + Expect(handler.bytesInFlight).To(BeZero()) + }) + It("calls OnPacketAcked and OnPacketLost with the right bytes_in_flight value", func() { cong.EXPECT().OnPacketSent(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(4) handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: 1, SendTime: time.Now().Add(-time.Hour)})) diff --git a/packet_packer.go b/packet_packer.go index c9b94d4f8..20f58ba1f 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -50,6 +50,8 @@ type packetContents struct { frames []ackhandler.Frame length protocol.ByteCount + + isMTUProbePacket bool } type coalescedPacket struct { @@ -98,12 +100,13 @@ func (p *packetContents) ToAckHandlerPacket(now time.Time, q *retransmissionQueu } } return &ackhandler.Packet{ - PacketNumber: p.header.PacketNumber, - LargestAcked: largestAcked, - Frames: p.frames, - Length: p.length, - EncryptionLevel: encLevel, - SendTime: now, + PacketNumber: p.header.PacketNumber, + LargestAcked: largestAcked, + Frames: p.frames, + Length: p.length, + EncryptionLevel: encLevel, + SendTime: now, + IsPathMTUProbePacket: p.isMTUProbePacket, } } @@ -701,6 +704,7 @@ func (p *packetPacker) PackMTUProbePacket(ping ackhandler.Frame, size protocol.B if err != nil { return nil, err } + contents.isMTUProbePacket = true return &packedPacket{ buffer: buffer, packetContents: contents, diff --git a/packet_packer_test.go b/packet_packer_test.go index a8341ac87..3911d8aa5 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -1480,6 +1480,7 @@ var _ = Describe("Packet packer", func() { Expect(p.header.PacketNumber).To(Equal(protocol.PacketNumber(0x43))) Expect(p.EncryptionLevel()).To(Equal(protocol.Encryption1RTT)) Expect(p.buffer.Data).To(HaveLen(int(probePacketSize))) + Expect(p.packetContents.isMTUProbePacket).To(BeTrue()) }) }) }) @@ -1510,6 +1511,14 @@ var _ = Describe("Converting to AckHandler packets", func() { Expect(p.LargestAcked).To(Equal(protocol.InvalidPacketNumber)) }) + It("marks MTU probe packets", func() { + packet := &packetContents{ + header: &wire.ExtendedHeader{Header: wire.Header{}}, + isMTUProbePacket: true, + } + Expect(packet.ToAckHandlerPacket(time.Now(), nil).IsPathMTUProbePacket).To(BeTrue()) + }) + DescribeTable( "doesn't overwrite the OnLost callback, if it is set", func(hdr wire.Header) {