From 54b76ceb3e91bd1f153c47d30e90891c75d83cb8 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sat, 9 Sep 2023 20:12:19 +0700 Subject: [PATCH] ackhandler: use the receive time of the Retry packet for RTT estimation (#4070) --- connection.go | 6 ++-- connection_test.go | 9 ++++-- internal/ackhandler/interfaces.go | 4 +-- internal/ackhandler/sent_packet_handler.go | 3 +- .../ackhandler/sent_packet_handler_test.go | 28 +++++++++++-------- .../mocks/ackhandler/sent_packet_handler.go | 8 +++--- 6 files changed, 32 insertions(+), 26 deletions(-) diff --git a/connection.go b/connection.go index 42b6c122..1f3dadfd 100644 --- a/connection.go +++ b/connection.go @@ -927,7 +927,7 @@ func (s *connection) handleLongHeaderPacket(p receivedPacket, hdr *wire.Header) }() if hdr.Type == protocol.PacketTypeRetry { - return s.handleRetryPacket(hdr, p.data) + return s.handleRetryPacket(hdr, p.data, p.rcvTime) } // The server can change the source connection ID with the first Handshake packet. @@ -1013,7 +1013,7 @@ func (s *connection) handleUnpackError(err error, p receivedPacket, pt logging.P return false } -func (s *connection) handleRetryPacket(hdr *wire.Header, data []byte) bool /* was this a valid Retry */ { +func (s *connection) handleRetryPacket(hdr *wire.Header, data []byte, rcvTime time.Time) bool /* was this a valid Retry */ { if s.perspective == protocol.PerspectiveServer { if s.tracer != nil { s.tracer.DroppedPacket(logging.PacketTypeRetry, protocol.ByteCount(len(data)), logging.PacketDropUnexpectedPacket) @@ -1062,7 +1062,7 @@ func (s *connection) handleRetryPacket(hdr *wire.Header, data []byte) bool /* wa } newDestConnID := hdr.SrcConnectionID s.receivedRetry = true - if err := s.sentPacketHandler.ResetForRetry(); err != nil { + if err := s.sentPacketHandler.ResetForRetry(rcvTime); err != nil { s.closeLocal(err) return false } diff --git a/connection_test.go b/connection_test.go index eb512513..8b7c763f 100644 --- a/connection_test.go +++ b/connection_test.go @@ -2767,9 +2767,10 @@ var _ = Describe("Client Connection", func() { } It("handles Retry packets", func() { + now := time.Now() sph := mockackhandler.NewMockSentPacketHandler(mockCtrl) conn.sentPacketHandler = sph - sph.EXPECT().ResetForRetry() + sph.EXPECT().ResetForRetry(now) sph.EXPECT().ReceivedBytes(gomock.Any()) cryptoSetup.EXPECT().ChangeConnectionID(protocol.ParseConnectionID([]byte{0xde, 0xad, 0xbe, 0xef})) packer.EXPECT().SetToken([]byte("foobar")) @@ -2778,7 +2779,9 @@ var _ = Describe("Client Connection", func() { Expect(hdr.SrcConnectionID).To(Equal(retryHdr.SrcConnectionID)) Expect(hdr.Token).To(Equal(retryHdr.Token)) }) - Expect(conn.handlePacketImpl(getPacket(retryHdr, getRetryTag(retryHdr)))).To(BeTrue()) + p := getPacket(retryHdr, getRetryTag(retryHdr)) + p.rcvTime = now + Expect(conn.handlePacketImpl(p)).To(BeTrue()) }) It("ignores Retry packets after receiving a regular packet", func() { @@ -3143,7 +3146,7 @@ var _ = Describe("Client Connection", func() { sph := mockackhandler.NewMockSentPacketHandler(mockCtrl) conn.sentPacketHandler = sph sph.EXPECT().ReceivedBytes(gomock.Any()).Times(2) - sph.EXPECT().ResetForRetry() + sph.EXPECT().ResetForRetry(gomock.Any()) newSrcConnID := protocol.ParseConnectionID([]byte{0xde, 0xad, 0xbe, 0xef}) cryptoSetup.EXPECT().ChangeConnectionID(newSrcConnID) packer.EXPECT().SetToken([]byte("foobar")) diff --git a/internal/ackhandler/interfaces.go b/internal/ackhandler/interfaces.go index 9c4a5b80..7b4eaa31 100644 --- a/internal/ackhandler/interfaces.go +++ b/internal/ackhandler/interfaces.go @@ -13,10 +13,10 @@ type SentPacketHandler interface { SentPacket(t time.Time, pn, largestAcked protocol.PacketNumber, streamFrames []StreamFrame, frames []Frame, encLevel protocol.EncryptionLevel, size protocol.ByteCount, isPathMTUProbePacket bool) // ReceivedAck processes an ACK frame. // It does not store a copy of the frame. - ReceivedAck(f *wire.AckFrame, encLevel protocol.EncryptionLevel, recvTime time.Time) (bool /* 1-RTT packet acked */, error) + ReceivedAck(f *wire.AckFrame, encLevel protocol.EncryptionLevel, rcvTime time.Time) (bool /* 1-RTT packet acked */, error) ReceivedBytes(protocol.ByteCount) DropPackets(protocol.EncryptionLevel) - ResetForRetry() error + ResetForRetry(rcvTime time.Time) error SetHandshakeConfirmed() // The SendMode determines if and what kind of packets can be sent. diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index c955da6e..a03d9f53 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -824,7 +824,7 @@ func (h *sentPacketHandler) queueFramesForRetransmission(p *packet) { p.Frames = nil } -func (h *sentPacketHandler) ResetForRetry() error { +func (h *sentPacketHandler) ResetForRetry(now time.Time) error { h.bytesInFlight = 0 var firstPacketSendTime time.Time h.initialPackets.history.Iterate(func(p *packet) (bool, error) { @@ -850,7 +850,6 @@ func (h *sentPacketHandler) ResetForRetry() error { // Otherwise, we don't know which Initial the Retry was sent in response to. if h.ptoCount == 0 { // Don't set the RTT to a value lower than 5ms here. - now := time.Now() h.rttStats.UpdateRTT(utils.Max(minRTTAfterRetry, now.Sub(firstPacketSendTime)), 0, now) if h.logger.Debug() { h.logger.Debugf("\tupdated RTT: %s (σ: %s)", h.rttStats.SmoothedRTT(), h.rttStats.MeanDeviation()) diff --git a/internal/ackhandler/sent_packet_handler_test.go b/internal/ackhandler/sent_packet_handler_test.go index fc120171..396014d7 100644 --- a/internal/ackhandler/sent_packet_handler_test.go +++ b/internal/ackhandler/sent_packet_handler_test.go @@ -1334,7 +1334,7 @@ var _ = Describe("SentPacketHandler", func() { Expect(handler.bytesInFlight).ToNot(BeZero()) Expect(handler.SendMode(time.Now())).To(Equal(SendAny)) // now receive a Retry - Expect(handler.ResetForRetry()).To(Succeed()) + Expect(handler.ResetForRetry(time.Now())).To(Succeed()) Expect(lostPackets).To(Equal([]protocol.PacketNumber{42})) Expect(handler.bytesInFlight).To(BeZero()) Expect(handler.GetLossDetectionTimeout()).To(BeZero()) @@ -1369,7 +1369,7 @@ var _ = Describe("SentPacketHandler", func() { }) Expect(handler.bytesInFlight).ToNot(BeZero()) // now receive a Retry - Expect(handler.ResetForRetry()).To(Succeed()) + Expect(handler.ResetForRetry(time.Now())).To(Succeed()) Expect(handler.bytesInFlight).To(BeZero()) Expect(lostInitial).To(BeTrue()) Expect(lost0RTT).To(BeTrue()) @@ -1379,49 +1379,53 @@ var _ = Describe("SentPacketHandler", func() { }) It("uses a Retry for an RTT estimate, if it was not retransmitted", func() { + now := time.Now() sentPacket(ackElicitingPacket(&packet{ PacketNumber: 42, EncryptionLevel: protocol.EncryptionInitial, - SendTime: time.Now().Add(-500 * time.Millisecond), + SendTime: now, })) sentPacket(ackElicitingPacket(&packet{ PacketNumber: 43, EncryptionLevel: protocol.EncryptionInitial, - SendTime: time.Now().Add(-10 * time.Millisecond), + SendTime: now.Add(500 * time.Millisecond), })) - Expect(handler.ResetForRetry()).To(Succeed()) - Expect(handler.rttStats.SmoothedRTT()).To(BeNumerically("~", 500*time.Millisecond, 100*time.Millisecond)) + Expect(handler.ResetForRetry(now.Add(time.Second))).To(Succeed()) + Expect(handler.rttStats.SmoothedRTT()).To(Equal(time.Second)) }) It("uses a Retry for an RTT estimate, but doesn't set the RTT to a value lower than 5ms", func() { + now := time.Now() sentPacket(ackElicitingPacket(&packet{ PacketNumber: 42, EncryptionLevel: protocol.EncryptionInitial, - SendTime: time.Now().Add(-500 * time.Microsecond), + SendTime: now, })) sentPacket(ackElicitingPacket(&packet{ PacketNumber: 43, EncryptionLevel: protocol.EncryptionInitial, - SendTime: time.Now().Add(-10 * time.Microsecond), + SendTime: now.Add(2 * time.Millisecond), })) - Expect(handler.ResetForRetry()).To(Succeed()) + Expect(handler.ResetForRetry(now.Add(4 * time.Millisecond))).To(Succeed()) + Expect(minRTTAfterRetry).To(BeNumerically(">", 4*time.Millisecond)) Expect(handler.rttStats.SmoothedRTT()).To(Equal(minRTTAfterRetry)) }) It("doesn't use a Retry for an RTT estimate, if it was not retransmitted", func() { + now := time.Now() sentPacket(ackElicitingPacket(&packet{ PacketNumber: 42, EncryptionLevel: protocol.EncryptionInitial, - SendTime: time.Now().Add(-800 * time.Millisecond), + SendTime: now, })) Expect(handler.OnLossDetectionTimeout()).To(Succeed()) Expect(handler.SendMode(time.Now())).To(Equal(SendPTOInitial)) sentPacket(ackElicitingPacket(&packet{ PacketNumber: 43, EncryptionLevel: protocol.EncryptionInitial, - SendTime: time.Now().Add(-100 * time.Millisecond), + SendTime: now.Add(500 * time.Millisecond), })) - Expect(handler.ResetForRetry()).To(Succeed()) + Expect(handler.ResetForRetry(now.Add(time.Second))).To(Succeed()) Expect(handler.rttStats.SmoothedRTT()).To(BeZero()) }) }) diff --git a/internal/mocks/ackhandler/sent_packet_handler.go b/internal/mocks/ackhandler/sent_packet_handler.go index 80aaae0f..24f5a157 100644 --- a/internal/mocks/ackhandler/sent_packet_handler.go +++ b/internal/mocks/ackhandler/sent_packet_handler.go @@ -148,17 +148,17 @@ func (mr *MockSentPacketHandlerMockRecorder) ReceivedBytes(arg0 interface{}) *go } // ResetForRetry mocks base method. -func (m *MockSentPacketHandler) ResetForRetry() error { +func (m *MockSentPacketHandler) ResetForRetry(arg0 time.Time) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ResetForRetry") + ret := m.ctrl.Call(m, "ResetForRetry", arg0) ret0, _ := ret[0].(error) return ret0 } // ResetForRetry indicates an expected call of ResetForRetry. -func (mr *MockSentPacketHandlerMockRecorder) ResetForRetry() *gomock.Call { +func (mr *MockSentPacketHandlerMockRecorder) ResetForRetry(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResetForRetry", reflect.TypeOf((*MockSentPacketHandler)(nil).ResetForRetry)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResetForRetry", reflect.TypeOf((*MockSentPacketHandler)(nil).ResetForRetry), arg0) } // SendMode mocks base method.