If a QUIC stream is reset by peer and does not finish sending, it is closed...

If a QUIC stream is reset by peer and does not finish sending, it is closed instead of marked as zombie streams. Protected by ENABLED FLAGS_quic_reloadable_flag_quic_reset_stream_is_not_zombie.

Change-Id: I3d1e3368c97d62cf1f2b3c5a82db0229ad6e710f
Reviewed-on: https://chromium-review.googlesource.com/968662Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Fan Yang <fayang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544219}

net/quic/core/quic_flags_list.h

@@ -216,3 +216,9 @@ QUIC_FLAG(bool,
 
 // If true, stop sending a redundant PING every 20 acks.
 QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_remove_redundant_ping, false)
+
+// If true, when a stream is reset by peer with error, it should not be added to
+// zombie streams.
+QUIC_FLAG(bool,
+          FLAGS_quic_reloadable_flag_quic_reset_stream_is_not_zombie,
+          true)

net/quic/core/quic_session_test.cc

@@ -1250,14 +1250,63 @@ TEST_P(QuicSessionTestServer, ZombieStreams) {
   EXPECT_CALL(*connection_, SendControlFrame(_));
   EXPECT_CALL(*connection_, OnStreamReset(2, _));
   session_.CloseStream(2);
-  EXPECT_TRUE(QuicContainsKey(session_.zombie_streams(), 2));
-  EXPECT_TRUE(session_.closed_streams()->empty());
+  if (GetQuicReloadableFlag(quic_reset_stream_is_not_zombie)) {
+    EXPECT_FALSE(QuicContainsKey(session_.zombie_streams(), 2));
+    ASSERT_EQ(1u, session_.closed_streams()->size());
+    EXPECT_EQ(2u, session_.closed_streams()->front()->id());
+  } else {
+    EXPECT_TRUE(QuicContainsKey(session_.zombie_streams(), 2));
+    EXPECT_TRUE(session_.closed_streams()->empty());
+  }
   session_.OnStreamDoneWaitingForAcks(2);
   EXPECT_FALSE(QuicContainsKey(session_.zombie_streams(), 2));
   EXPECT_EQ(1u, session_.closed_streams()->size());
   EXPECT_EQ(2u, session_.closed_streams()->front()->id());
 }
 
+// Regression test of b/71548958.
+TEST_P(QuicSessionTestServer, TestZombieStreams) {
+  session_.set_writev_consumes_all_data(true);
+
+  TestStream* stream2 = session_.CreateOutgoingDynamicStream();
+  QuicString body(100, '.');
+  stream2->WriteOrBufferData(body, false, nullptr);
+  EXPECT_TRUE(stream2->IsWaitingForAcks());
+  EXPECT_EQ(1u, QuicStreamPeer::SendBuffer(stream2).size());
+
+  QuicRstStreamFrame rst_frame(kInvalidControlFrameId, stream2->id(),
+                               QUIC_STREAM_CANCELLED, 1234);
+  EXPECT_CALL(*connection_, SendControlFrame(_))
+      .WillOnce(Invoke(&session_, &TestSession::ClearControlFrame));
+  EXPECT_CALL(*connection_,
+              OnStreamReset(stream2->id(), QUIC_RST_ACKNOWLEDGEMENT));
+  stream2->OnStreamReset(rst_frame);
+  if (GetQuicReloadableFlag(quic_reset_stream_is_not_zombie)) {
+    EXPECT_FALSE(QuicContainsKey(session_.zombie_streams(), stream2->id()));
+    ASSERT_EQ(1u, session_.closed_streams()->size());
+    EXPECT_EQ(stream2->id(), session_.closed_streams()->front()->id());
+  } else {
+    // Stream reset by peer, and it becomes zombie stream and the data will be
+    // NEVER be acked because the frames in unacked packet map are removed.
+    EXPECT_TRUE(QuicContainsKey(session_.zombie_streams(), stream2->id()));
+    EXPECT_TRUE(session_.closed_streams()->empty());
+    EXPECT_EQ(1u, QuicStreamPeer::SendBuffer(stream2).size());
+  }
+
+  TestStream* stream4 = session_.CreateOutgoingDynamicStream();
+  EXPECT_CALL(*connection_, SendControlFrame(_)).Times(1);
+  EXPECT_CALL(*connection_,
+              OnStreamReset(stream4->id(), QUIC_STREAM_CANCELLED));
+  stream4->WriteOrBufferData(body, false, nullptr);
+  stream4->Reset(QUIC_STREAM_CANCELLED);
+  EXPECT_FALSE(QuicContainsKey(session_.zombie_streams(), stream4->id()));
+  if (GetQuicReloadableFlag(quic_reset_stream_is_not_zombie)) {
+    EXPECT_EQ(2u, session_.closed_streams()->size());
+  } else {
+    EXPECT_EQ(1u, session_.closed_streams()->size());
+  }
+}
+
 TEST_P(QuicSessionTestServer, OnStreamFrameLost) {
   QuicConnectionPeer::SetSessionDecidesWhatToWrite(connection_);
   InSequence s;

net/quic/core/quic_spdy_session_test.cc

@@ -1460,8 +1460,14 @@ TEST_P(QuicSpdySessionTestServer, ZombieStreams) {
   EXPECT_CALL(*connection_, SendControlFrame(_));
   EXPECT_CALL(*connection_, OnStreamReset(2, _));
   session_.CloseStream(2);
-  EXPECT_TRUE(QuicContainsKey(session_.zombie_streams(), 2));
-  EXPECT_TRUE(session_.closed_streams()->empty());
+  if (GetQuicReloadableFlag(quic_reset_stream_is_not_zombie)) {
+    EXPECT_FALSE(QuicContainsKey(session_.zombie_streams(), 2));
+    ASSERT_EQ(1u, session_.closed_streams()->size());
+    EXPECT_EQ(2u, session_.closed_streams()->front()->id());
+  } else {
+    EXPECT_TRUE(QuicContainsKey(session_.zombie_streams(), 2));
+    EXPECT_TRUE(session_.closed_streams()->empty());
+  }
   session_.OnStreamDoneWaitingForAcks(2);
   EXPECT_FALSE(QuicContainsKey(session_.zombie_streams(), 2));
   EXPECT_EQ(1u, session_.closed_streams()->size());

net/quic/core/quic_stream.cc

@@ -525,6 +525,10 @@ void QuicStream::OnClose() {
     // written on this stream before termination. Done here if needed, using a
     // RST_STREAM frame.
     QUIC_DLOG(INFO) << ENDPOINT << "Sending RST_STREAM in OnClose: " << id();
+    if (GetQuicReloadableFlag(quic_reset_stream_is_not_zombie)) {
+      QUIC_FLAG_COUNT(quic_reloadable_flag_quic_reset_stream_is_not_zombie);
+      session_->OnStreamDoneWaitingForAcks(id_);
+    }
     session_->SendRstStream(id(), QUIC_RST_ACKNOWLEDGEMENT,
                             stream_bytes_written());
     rst_sent_ = true;

net/quic/core/quic_stream_test.cc

@@ -819,7 +819,7 @@ TEST_F(QuicStreamTest, RstFrameReceivedStreamFinishSending) {
   QuicRstStreamFrame rst_frame(kInvalidControlFrameId, stream_->id(),
                                QUIC_STREAM_CANCELLED, 1234);
   stream_->OnStreamReset(rst_frame);
-  // Stream stops waiting for acks as it has unacked data.
+  // Stream still waits for acks as it finishes sending and has unacked data.
   EXPECT_TRUE(stream_->IsWaitingForAcks());
   EXPECT_EQ(1u, QuicStreamPeer::SendBuffer(stream_).size());
 }