commit 40ec3acf2d13d105b12e99ff8e03e5d3fade9261
author fayang <fayang@google.com> Wed Jun 05 09:07:54 2019 -0700
committer Copybara-Service <copybara-worker@google.com> Wed Jun 05 21:19:06 2019 -0700
tree a5f3a76e2f59833b0a02e5d95fe0efbc29fb48e6
parent ecf9bd86d4931c515b57252707fb735dc248aaa0

gfe-relnote: In QUIC, add connected_ check in OnCanWrite and flusher's destructor. Protected by gfe2_reloadable_flag_quic_check_connected_before_flush.

PiperOrigin-RevId: 251653969
Change-Id: I93d9204391c7b900d0abb73aab9cab70f7ffabe9

quic/core/quic_connection.cc

diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index 1c589d6..83e6f04 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -2094,6 +2094,10 @@
 }
 
 void QuicConnection::OnCanWrite() {
+  if (GetQuicReloadableFlag(quic_check_connected_before_flush) && !connected_) {
+    QUIC_RELOADABLE_FLAG_COUNT_N(quic_check_connected_before_flush, 2, 2);
+    return;
+  }
   DCHECK(!writer_->IsWriteBlocked());
 
   // Add a flusher to ensure the connection is marked app-limited.
@@ -3459,6 +3463,11 @@
   if (connection_ == nullptr) {
     return;
   }
+  if (GetQuicReloadableFlag(quic_check_connected_before_flush) &&
+      !connection_->connected()) {
+    QUIC_RELOADABLE_FLAG_COUNT_N(quic_check_connected_before_flush, 1, 2);
+    return;
+  }
 
   if (flush_and_set_pending_retransmission_alarm_on_delete_) {
     if (connection_->packet_generator_.deprecate_ack_bundling_mode()) {

quic/core/quic_connection_test.cc

diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 9cca434..ba40e7a 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -1943,7 +1943,11 @@
 
   EXPECT_EQ(0u, connection_.GetStats().packets_discarded);
   connection_.OnCanWrite();
-  EXPECT_EQ(1u, connection_.GetStats().packets_discarded);
+  if (GetQuicReloadableFlag(quic_check_connected_before_flush)) {
+    EXPECT_EQ(0u, connection_.GetStats().packets_discarded);
+  } else {
+    EXPECT_EQ(1u, connection_.GetStats().packets_discarded);
+  }
 }
 
 TEST_P(QuicConnectionTest, ReceiveConnectivityProbingAtServer) {
@@ -8505,6 +8509,44 @@
   EXPECT_EQ(TestConnectionId(0x33), connection_.client_connection_id());
 }
 
+// Regression test for b/134416344.
+TEST_P(QuicConnectionTest, CheckConnectedBeforeFlush) {
+  // This test mimics a scenario where a connection processes 2 packets and the
+  // 2nd packet contains connection close frame. When the 2nd flusher goes out
+  // of scope, a delayed ACK is pending, and ACK alarm should not be scheduled
+  // because connection is disconnected.
+  EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
+  EXPECT_CALL(visitor_, OnConnectionClosed(_, _, _));
+  EXPECT_EQ(Perspective::IS_CLIENT, connection_.perspective());
+  std::unique_ptr<QuicConnectionCloseFrame> connection_close_frame(
+      new QuicConnectionCloseFrame(QUIC_INTERNAL_ERROR));
+  if (connection_.transport_version() == QUIC_VERSION_99) {
+    connection_close_frame->close_type = IETF_QUIC_TRANSPORT_CONNECTION_CLOSE;
+  }
+  // Received 2 packets.
+  QuicFrame frame;
+  if (QuicVersionUsesCryptoFrames(connection_.transport_version())) {
+    frame = QuicFrame(&crypto_frame_);
+    EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber());
+  } else {
+    frame = QuicFrame(QuicStreamFrame(
+        QuicUtils::GetCryptoStreamId(connection_.transport_version()), false,
+        0u, QuicStringPiece()));
+    EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber());
+  }
+  ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress);
+  QuicAlarm* ack_alarm = QuicConnectionPeer::GetAckAlarm(&connection_);
+  EXPECT_TRUE(ack_alarm->IsSet());
+  ProcessFramePacketWithAddresses(QuicFrame(connection_close_frame.get()),
+                                  kSelfAddress, kPeerAddress);
+  if (GetQuicReloadableFlag(quic_check_connected_before_flush)) {
+    // Verify ack alarm is not set.
+    EXPECT_FALSE(ack_alarm->IsSet());
+  } else {
+    EXPECT_TRUE(ack_alarm->IsSet());
+  }
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace quic