commit 7bc93b76ebfaee26c65e9ffe3213337bdf9fadea
author fayang <fayang@google.com> Tue Aug 11 10:45:57 2020 -0700
committer Copybara-Service <copybara-worker@google.com> Tue Aug 11 10:46:36 2020 -0700
tree 15141dc573ab3ad207dbd2a12ac2d6a19a318a93
parent 07b2e8df6cc8aeda04e989102f4c97a11297ffb2

In QUIC, do not send PING when PING alarm fires but ShouldKeepConnectionAlive returns false. Client side only, protected by ENABLED gfe2_reloadable_flag_quic_fix_on_ping_timeout.

This is used to fix a bug where PING alarm is not cancelled when application finish reading data (draining streams get closed).

PiperOrigin-RevId: 326053367
Change-Id: I06aea85dad633c40e1d8486f0cb790c644b0e0e9

quic/core/quic_connection.cc

diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index f442b1b..4bd2195 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -3091,9 +3091,14 @@
 }
 
 void QuicConnection::OnPingTimeout() {
-  if (!retransmission_alarm_->IsSet()) {
-    visitor_->SendPing();
+  if (retransmission_alarm_->IsSet()) {
+    return;
   }
+  if (GetQuicReloadableFlag(quic_fix_on_ping_timeout) &&
+      !visitor_->ShouldKeepConnectionAlive()) {
+    return;
+  }
+  visitor_->SendPing();
 }
 
 void QuicConnection::SendAck() {

quic/core/quic_connection_test.cc

diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 49c5c4d..5340233 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -11646,6 +11646,58 @@
   }
 }
 
+TEST_P(QuicConnectionTest, DonotSendPing) {
+  connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
+  connection_.OnHandshakeComplete();
+  EXPECT_TRUE(connection_.connected());
+  EXPECT_CALL(visitor_, ShouldKeepConnectionAlive())
+      .WillRepeatedly(Return(true));
+  EXPECT_FALSE(connection_.GetPingAlarm()->IsSet());
+  EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet());
+
+  SendStreamDataToPeer(
+      GetNthClientInitiatedStreamId(0, connection_.transport_version()),
+      "GET /", 0, FIN, nullptr);
+  EXPECT_TRUE(connection_.GetPingAlarm()->IsSet());
+  EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet());
+  EXPECT_EQ(QuicTime::Delta::FromSeconds(15),
+            connection_.GetPingAlarm()->deadline() - clock_.ApproximateNow());
+
+  // Now recevie an ACK and response of the previous packet, which will move the
+  // ping alarm forward.
+  clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(5));
+  QuicFrames frames;
+  QuicAckFrame ack_frame = InitAckFrame(1);
+  frames.push_back(QuicFrame(&ack_frame));
+  frames.push_back(QuicFrame(QuicStreamFrame(
+      GetNthClientInitiatedStreamId(0, connection_.transport_version()), true,
+      0u, quiche::QuicheStringPiece())));
+  EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
+  EXPECT_CALL(*send_algorithm_, OnCongestionEvent(true, _, _, _, _));
+  EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(1);
+  ProcessFramesPacketAtLevel(1, frames, ENCRYPTION_FORWARD_SECURE);
+  EXPECT_TRUE(connection_.GetPingAlarm()->IsSet());
+  EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet());
+  // The ping timer is set slightly less than 15 seconds in the future, because
+  // of the 1s ping timer alarm granularity.
+  EXPECT_EQ(
+      QuicTime::Delta::FromSeconds(15) - QuicTime::Delta::FromMilliseconds(5),
+      connection_.GetPingAlarm()->deadline() - clock_.ApproximateNow());
+
+  clock_.AdvanceTime(QuicTime::Delta::FromSeconds(15));
+  // Suppose now ShouldKeepConnectionAlive returns false.
+  EXPECT_CALL(visitor_, ShouldKeepConnectionAlive())
+      .WillRepeatedly(Return(false));
+  if (GetQuicReloadableFlag(quic_fix_on_ping_timeout)) {
+    // Verify PING does not get sent.
+    EXPECT_CALL(visitor_, SendPing()).Times(0);
+  } else {
+    // PING get sent even ShouldKeepConnectionAlive returns false.
+    EXPECT_CALL(visitor_, SendPing()).Times(1);
+  }
+  connection_.GetPingAlarm()->Fire();
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace quic