don't send application data probe packets before the handshake completes

internal/ackhandler/interfaces.go

@@ -27,6 +27,7 @@ type SentPacketHandler interface {
 	ReceivedAck(ackFrame *wire.AckFrame, withPacketNumber protocol.PacketNumber, encLevel protocol.EncryptionLevel, recvTime time.Time) error
 	DropPackets(protocol.EncryptionLevel)
 	ResetForRetry() error
+	SetHandshakeComplete()
 
 	// The SendMode determines if and what kind of packets can be sent.
 	SendMode() SendMode

internal/ackhandler/sent_packet_handler.go

@@ -48,6 +48,8 @@ type sentPacketHandler struct {
 	handshakePackets *packetNumberSpace
 	oneRTTPackets    *packetNumberSpace
 
+	handshakeComplete bool
+
 	// lowestNotConfirmedAcked is the lowest packet number that we sent an ACK for, but haven't received confirmation, that this ACK actually arrived
 	// example: we send an ACK for packets 90-100 with packet number 20
 	// once we receive an ACK from the peer for packet 20, the lowestNotConfirmedAcked is 101
@@ -290,7 +292,8 @@ func (h *sentPacketHandler) getEarliestLossTimeAndSpace() (time.Time, protocol.E
 		lossTime = h.handshakePackets.lossTime
 		encLevel = protocol.EncryptionHandshake
 	}
-	if lossTime.IsZero() || (!h.oneRTTPackets.lossTime.IsZero() && h.oneRTTPackets.lossTime.Before(lossTime)) {
+	if h.handshakeComplete &&
+		(lossTime.IsZero() || (!h.oneRTTPackets.lossTime.IsZero() && h.oneRTTPackets.lossTime.Before(lossTime))) {
 		lossTime = h.oneRTTPackets.lossTime
 		encLevel = protocol.Encryption1RTT
 	}
@@ -310,7 +313,8 @@ func (h *sentPacketHandler) getEarliestSentTimeAndSpace() (time.Time, protocol.E
 		sentTime = h.handshakePackets.lastSentAckElicitingPacketTime
 		encLevel = protocol.EncryptionHandshake
 	}
-	if sentTime.IsZero() || (!h.oneRTTPackets.lastSentAckElicitingPacketTime.IsZero() && h.oneRTTPackets.lastSentAckElicitingPacketTime.Before(sentTime)) {
+	if h.handshakeComplete &&
+		(sentTime.IsZero() || (!h.oneRTTPackets.lastSentAckElicitingPacketTime.IsZero() && h.oneRTTPackets.lastSentAckElicitingPacketTime.Before(sentTime))) {
 		sentTime = h.oneRTTPackets.lastSentAckElicitingPacketTime
 		encLevel = protocol.Encryption1RTT
 	}
@@ -329,7 +333,10 @@ func (h *sentPacketHandler) hasOutstandingCryptoPackets() bool {
 }
 
 func (h *sentPacketHandler) hasOutstandingPackets() bool {
-	return h.oneRTTPackets.history.HasOutstandingPackets() || h.hasOutstandingCryptoPackets()
+	// We only send application data probe packets once the handshake completes,
+	// because before that, we don't have the keys to decrypt ACKs sent in 1-RTT packets.
+	return (h.handshakeComplete && h.oneRTTPackets.history.HasOutstandingPackets()) ||
+		h.hasOutstandingCryptoPackets()
 }
 
 func (h *sentPacketHandler) setLossDetectionTimer() {
@@ -420,7 +427,7 @@ func (h *sentPacketHandler) detectLostPackets(
 
 func (h *sentPacketHandler) OnLossDetectionTimeout() error {
 	// When all outstanding are acknowledged, the alarm is canceled in
-	// updateLossDetectionAlarm. This doesn't reset the timer in the session though.
+	// setLossDetectionTimer. This doesn't reset the timer in the session though.
 	// When OnAlarm is called, we therefore need to make sure that there are
 	// actually packets outstanding.
 	if h.hasOutstandingPackets() {
@@ -591,6 +598,13 @@ func (h *sentPacketHandler) ResetForRetry() error {
 	return nil
 }
 
+func (h *sentPacketHandler) SetHandshakeComplete() {
+	h.handshakeComplete = true
+	// We don't send PTOs for application data packets before the handshake completes.
+	// Make sure the timer is armed now, if necessary.
+	h.setLossDetectionTimer()
+}
+
 func (h *sentPacketHandler) GetStats() *quictrace.TransportState {
 	return &quictrace.TransportState{
 		MinRTT:           h.rttStats.MinRTT(),

internal/ackhandler/sent_packet_handler_test.go

@@ -577,6 +577,7 @@ var _ = Describe("SentPacketHandler", func() {
 		})
 
 		It("implements exponential backoff", func() {
+			handler.SetHandshakeComplete()
 			sendTime := time.Now().Add(-time.Hour)
 			handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: 1, SendTime: sendTime}))
 			timeout := handler.GetLossDetectionTimeout().Sub(sendTime)
@@ -590,6 +591,7 @@ var _ = Describe("SentPacketHandler", func() {
 		})
 
 		It("allows two 1-RTT PTOs", func() {
+			handler.SetHandshakeComplete()
 			var lostPackets []protocol.PacketNumber
 			handler.SentPacket(ackElicitingPacket(&Packet{
 				PacketNumber: 1,
@@ -608,6 +610,7 @@ var _ = Describe("SentPacketHandler", func() {
 		})
 
 		It("only counts ack-eliciting packets as probe packets", func() {
+			handler.SetHandshakeComplete()
 			handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: 1, SendTime: time.Now().Add(-time.Hour)}))
 			Expect(handler.OnLossDetectionTimeout()).To(Succeed())
 			Expect(handler.SendMode()).To(Equal(SendPTOAppData))
@@ -623,6 +626,7 @@ var _ = Describe("SentPacketHandler", func() {
 		})
 
 		It("gets two probe packets if RTO expires", func() {
+			handler.SetHandshakeComplete()
 			handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: 1}))
 			handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: 2}))
 
@@ -662,7 +666,20 @@ var _ = Describe("SentPacketHandler", func() {
 			Expect(handler.SendMode()).To(Equal(SendAny))
 		})
 
+		It("doesn't send 1-RTT probe packets before the handshake completes", func() {
+			handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: 1}))
+			updateRTT(time.Hour)
+			Expect(handler.OnLossDetectionTimeout()).To(Succeed()) // TLP
+			Expect(handler.GetLossDetectionTimeout()).To(BeZero())
+			Expect(handler.SendMode()).To(Equal(SendAny))
+			handler.SetHandshakeComplete()
+			Expect(handler.GetLossDetectionTimeout()).ToNot(BeZero())
+			Expect(handler.OnLossDetectionTimeout()).To(Succeed())
+			Expect(handler.SendMode()).To(Equal(SendPTOAppData))
+		})
+
 		It("resets the send mode when it receives an acknowledgement after queueing probe packets", func() {
+			handler.SetHandshakeComplete()
 			handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: 1, SendTime: time.Now().Add(-time.Hour)}))
 			handler.rttStats.UpdateRTT(time.Second, 0, time.Now())
 			Expect(handler.OnLossDetectionTimeout()).To(Succeed())

internal/mocks/ackhandler/sent_packet_handler.go

@@ -203,6 +203,18 @@ func (mr *MockSentPacketHandlerMockRecorder) SentPacket(arg0 interface{}) *gomoc
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SentPacket", reflect.TypeOf((*MockSentPacketHandler)(nil).SentPacket), arg0)
 }
 
+// SetHandshakeComplete mocks base method
+func (m *MockSentPacketHandler) SetHandshakeComplete() {
+	m.ctrl.T.Helper()
+	m.ctrl.Call(m, "SetHandshakeComplete")
+}
+
+// SetHandshakeComplete indicates an expected call of SetHandshakeComplete
+func (mr *MockSentPacketHandlerMockRecorder) SetHandshakeComplete() *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetHandshakeComplete", reflect.TypeOf((*MockSentPacketHandler)(nil).SetHandshakeComplete))
+}
+
 // ShouldSendNumPackets mocks base method
 func (m *MockSentPacketHandler) ShouldSendNumPackets() int {
 	m.ctrl.T.Helper()

session.go

@@ -590,6 +590,7 @@ func (s *session) handleHandshakeComplete() {
 	s.handshakeCompleteChan = nil // prevent this case from ever being selected again
 	s.handshakeCtxCancel()
 
+	s.sentPacketHandler.SetHandshakeComplete()
 	// The client completes the handshake first (after sending the CFIN).
 	// We need to make sure it learns about the server completing the handshake,
 	// in order to stop retransmitting handshake packets.

session_test.go

@@ -1163,9 +1163,16 @@ var _ = Describe("Session", func() {
 		})
 	})
 
-	It("cancels the HandshakeComplete context when the handshake completes", func() {
+	It("cancels the HandshakeComplete context and informs the SentPacketHandler when the handshake completes", func() {
 		packer.EXPECT().PackPacket().AnyTimes()
 		finishHandshake := make(chan struct{})
+		sph := mockackhandler.NewMockSentPacketHandler(mockCtrl)
+		sess.sentPacketHandler = sph
+		sphNotified := make(chan struct{})
+		sph.EXPECT().SetHandshakeComplete().Do(func() { close(sphNotified) })
+		sph.EXPECT().GetLossDetectionTimeout().AnyTimes()
+		sph.EXPECT().TimeUntilSend().AnyTimes()
+		sph.EXPECT().SendMode().AnyTimes()
 		go func() {
 			defer GinkgoRecover()
 			<-finishHandshake
@@ -1177,6 +1184,7 @@ var _ = Describe("Session", func() {
 		Consistently(handshakeCtx.Done()).ShouldNot(BeClosed())
 		close(finishHandshake)
 		Eventually(handshakeCtx.Done()).Should(BeClosed())
+		Eventually(sphNotified).Should(BeClosed())
 		// make sure the go routine returns
 		streamManager.EXPECT().CloseWithError(gomock.Any())
 		expectReplaceWithClosed()