ackhandler: use the receive time of the Retry packet for RTT estimation (#4070)

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
 	}

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"))

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.

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())

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())
 		})
 	})

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.