deprecate gfe2_reloadable_flag_quic_remove_zombie_streams. PiperOrigin-RevId: 334253770 Change-Id: I52556e52b468948510cc10d62bf7c679ee4f8505
diff --git a/quic/core/http/end_to_end_test.cc b/quic/core/http/end_to_end_test.cc index 50b7904..7a0bd9a 100644 --- a/quic/core/http/end_to_end_test.cc +++ b/quic/core/http/end_to_end_test.cc
@@ -3162,7 +3162,6 @@ QuicSession* session = GetClientSession(); ASSERT_TRUE(session); // Verify canceled stream does not become zombie. - EXPECT_TRUE(QuicSessionPeer::zombie_streams(session).empty()); EXPECT_EQ(1u, QuicSessionPeer::closed_streams(session).size()); }
diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc index 1a6ecb0..0f806f7 100644 --- a/quic/core/http/quic_spdy_session.cc +++ b/quic/core/http/quic_spdy_session.cc
@@ -426,10 +426,6 @@ for (auto& stream : *closed_streams()) { static_cast<QuicSpdyStream*>(stream.get())->ClearSession(); } - DCHECK(!remove_zombie_streams() || zombie_streams().empty()); - for (auto const& kv : zombie_streams()) { - static_cast<QuicSpdyStream*>(kv.second.get())->ClearSession(); - } for (auto const& kv : stream_map()) { if (!kv.second->is_static()) { static_cast<QuicSpdyStream*>(kv.second.get())->ClearSession();
diff --git a/quic/core/quic_session.cc b/quic/core/quic_session.cc index 15b56f7..4737902 100644 --- a/quic/core/quic_session.cc +++ b/quic/core/quic_session.cc
@@ -100,9 +100,7 @@ is_configured_(false), enable_round_robin_scheduling_(false), was_zero_rtt_rejected_(false), - liveness_testing_in_progress_(false), - remove_zombie_streams_( - GetQuicReloadableFlag(quic_remove_zombie_streams)) { + liveness_testing_in_progress_(false) { closed_streams_clean_up_alarm_ = QuicWrapUnique<QuicAlarm>(connection_->alarm_factory()->CreateAlarm( new ClosedStreamsCleanUpDelegate(this))); @@ -147,12 +145,7 @@ GetMutableCryptoStream()->id()); } -QuicSession::~QuicSession() { - if (!remove_zombie_streams_) { - QUIC_LOG_IF(WARNING, !zombie_streams_.empty()) - << "Still have zombie streams"; - } -} +QuicSession::~QuicSession() {} void QuicSession::PendingStreamOnStreamFrame(const QuicStreamFrame& frame) { DCHECK(VersionUsesHttp3(transport_version())); @@ -395,30 +388,13 @@ stream->OnConnectionClosed(frame.quic_error_code, source); auto it = stream_map_.find(id); if (it != stream_map_.end()) { - if (!remove_zombie_streams_) { - QUIC_BUG << ENDPOINT << "Stream " << id - << " failed to close under OnConnectionClosed"; - } else { - QUIC_BUG_IF(!it->second->IsZombie()) - << ENDPOINT << "Non-zombie stream " << id - << " failed to close under OnConnectionClosed"; - } + QUIC_BUG_IF(!it->second->IsZombie()) + << ENDPOINT << "Non-zombie stream " << id + << " failed to close under OnConnectionClosed"; } return true; }); - if (!remove_zombie_streams_) { - // Cleanup zombie stream map on connection close. - while (!zombie_streams_.empty()) { - ZombieStreamMap::iterator it = zombie_streams_.begin(); - closed_streams_.push_back(std::move(it->second)); - zombie_streams_.erase(it); - } - } else { - QUIC_RELOADABLE_FLAG_COUNT_N(quic_remove_zombie_streams, 1, 4); - DCHECK(zombie_streams_.empty()); - } - closed_streams_clean_up_alarm_->Cancel(); if (visitor_) { @@ -918,20 +894,11 @@ const bool stream_waiting_for_acks = stream->IsWaitingForAcks(); if (stream_waiting_for_acks) { - if (remove_zombie_streams_) { - // The stream needs to be kept alive because it's waiting for acks. - QUIC_RELOADABLE_FLAG_COUNT_N(quic_remove_zombie_streams, 2, 4); - ++num_zombie_streams_; - } else { - zombie_streams_[stream_id] = std::move(it->second); - } + // The stream needs to be kept alive because it's waiting for acks. + ++num_zombie_streams_; } else { closed_streams_.push_back(std::move(it->second)); - if (remove_zombie_streams_) { - // When zombie_streams_ is removed, stream is only erased from stream map - // if it's not zombie. - stream_map_.erase(it); - } + stream_map_.erase(it); // Do not retransmit data of a closed stream. streams_with_pending_retransmission_.erase(stream_id); if (!closed_streams_clean_up_alarm_->IsSet()) { @@ -949,18 +916,12 @@ DCHECK(!stream->was_draining()); InsertLocallyClosedStreamsHighestOffset( stream_id, stream->highest_received_byte_offset()); - if (!remove_zombie_streams_) { - stream_map_.erase(it); - } return; } const bool stream_was_draining = stream->was_draining(); QUIC_DVLOG_IF(1, stream_was_draining) << ENDPOINT << "Stream " << stream_id << " was draining"; - if (!remove_zombie_streams_) { - stream_map_.erase(it); - } if (stream_was_draining) { QUIC_BUG_IF(num_draining_streams_ == 0); --num_draining_streams_; @@ -1804,11 +1765,7 @@ StreamMap::iterator it = stream_map_.find(stream_id); if (it != stream_map_.end()) { - if (remove_zombie_streams_ && it->second->IsZombie()) { - QUIC_RELOADABLE_FLAG_COUNT_N(quic_remove_zombie_streams, 3, 4); - return nullptr; - } - return it->second.get(); + return it->second->IsZombie() ? nullptr : it->second.get(); } if (IsClosedStream(stream_id)) { @@ -1954,7 +1911,7 @@ DCHECK_NE(QuicUtils::GetInvalidStreamId(transport_version()), id); const StreamMap::iterator it = stream_map_.find(id); if (it != stream_map_.end()) { - return remove_zombie_streams_ ? !it->second->IsZombie() : true; + return !it->second->IsZombie(); } if (QuicContainsKey(pending_stream_map_, id) || QuicUtils::IsCryptoStreamId(transport_version(), id)) { @@ -2046,24 +2003,13 @@ } void QuicSession::OnStreamDoneWaitingForAcks(QuicStreamId id) { - if (!remove_zombie_streams_) { - auto it = zombie_streams_.find(id); - if (it == zombie_streams_.end()) { - return; - } - - closed_streams_.push_back(std::move(it->second)); - zombie_streams_.erase(it); - } else { - QUIC_RELOADABLE_FLAG_COUNT_N(quic_remove_zombie_streams, 4, 4); - auto it = stream_map_.find(id); - if (it == stream_map_.end()) { - return; - } - --num_zombie_streams_; - closed_streams_.push_back(std::move(it->second)); - stream_map_.erase(it); + auto it = stream_map_.find(id); + if (it == stream_map_.end()) { + return; } + --num_zombie_streams_; + closed_streams_.push_back(std::move(it->second)); + stream_map_.erase(it); if (!closed_streams_clean_up_alarm_->IsSet()) { closed_streams_clean_up_alarm_->Set(connection_->clock()->ApproximateNow()); @@ -2078,12 +2024,6 @@ return active_stream->second.get(); } - DCHECK(!remove_zombie_streams_ || zombie_streams_.empty()); - auto zombie_stream = zombie_streams_.find(id); - if (zombie_stream != zombie_streams_.end()) { - return zombie_stream->second.get(); - } - if (QuicUtils::IsCryptoStreamId(transport_version(), id)) { return const_cast<QuicCryptoStream*>(GetCryptoStream()); } @@ -2224,10 +2164,6 @@ } bool QuicSession::HasUnackedStreamData() const { - DCHECK(!remove_zombie_streams_ || zombie_streams_.empty()); - if (!zombie_streams().empty()) { - return true; - } for (const auto& it : stream_map_) { if (it.second->IsWaitingForAcks()) { return true; @@ -2475,8 +2411,7 @@ std::function<bool(QuicStream*)> action) { std::vector<QuicStream*> active_streams; for (const auto& it : stream_map_) { - if (!it.second->is_static() && - (!remove_zombie_streams_ || !it.second->IsZombie())) { + if (!it.second->is_static() && !it.second->IsZombie()) { active_streams.push_back(it.second.get()); } } @@ -2491,8 +2426,7 @@ void QuicSession::PerformActionOnActiveStreams( std::function<bool(QuicStream*)> action) const { for (const auto& it : stream_map_) { - if (!it.second->is_static() && - (!remove_zombie_streams_ || !it.second->IsZombie()) && + if (!it.second->is_static() && !it.second->IsZombie() && !action(it.second.get())) { return; }
diff --git a/quic/core/quic_session.h b/quic/core/quic_session.h index 302a47a..f5e01f8 100644 --- a/quic/core/quic_session.h +++ b/quic/core/quic_session.h
@@ -491,8 +491,6 @@ return liveness_testing_in_progress_; } - bool remove_zombie_streams() const { return remove_zombie_streams_; } - protected: using StreamMap = QuicHashMap<QuicStreamId, std::unique_ptr<QuicStream>>; @@ -566,8 +564,6 @@ ClosedStreams* closed_streams() { return &closed_streams_; } - const ZombieStreamMap& zombie_streams() const { return zombie_streams_; } - void set_largest_peer_created_stream_id( QuicStreamId largest_peer_created_stream_id); @@ -749,9 +745,6 @@ QuicWriteBlockedList write_blocked_streams_; ClosedStreams closed_streams_; - // Streams which are closed, but need to be kept alive. Currently, the only - // reason is the stream's sent data (including FIN) does not get fully acked. - ZombieStreamMap zombie_streams_; QuicConfig config_; @@ -844,9 +837,6 @@ // This indicates a liveness testing is in progress, and push back the // creation of new outgoing bidirectional streams. bool liveness_testing_in_progress_; - - // Latched value of flag quic_remove_zombie_streams. - const bool remove_zombie_streams_; }; } // namespace quic
diff --git a/quic/core/quic_session_test.cc b/quic/core/quic_session_test.cc index a5d9b5f..e5b6cff 100644 --- a/quic/core/quic_session_test.cc +++ b/quic/core/quic_session_test.cc
@@ -367,7 +367,6 @@ using QuicSession::GetNextOutgoingBidirectionalStreamId; using QuicSession::GetNextOutgoingUnidirectionalStreamId; using QuicSession::stream_map; - using QuicSession::zombie_streams; private: StrictMock<TestCryptoStream> crypto_stream_; @@ -2261,11 +2260,9 @@ EXPECT_TRUE(stream2->IsWaitingForAcks()); CloseStream(stream2->id()); - EXPECT_FALSE(QuicContainsKey(session_.zombie_streams(), stream2->id())); ASSERT_EQ(1u, session_.closed_streams()->size()); EXPECT_EQ(stream2->id(), session_.closed_streams()->front()->id()); session_.OnStreamDoneWaitingForAcks(stream2->id()); - EXPECT_FALSE(QuicContainsKey(session_.zombie_streams(), stream2->id())); EXPECT_EQ(1u, session_.closed_streams()->size()); EXPECT_EQ(stream2->id(), session_.closed_streams()->front()->id()); } @@ -2320,7 +2317,6 @@ EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0); session_.OnStopSendingFrame(frame); } - EXPECT_FALSE(QuicContainsKey(session_.zombie_streams(), stream2->id())); ASSERT_EQ(1u, session_.closed_streams()->size()); EXPECT_EQ(stream2->id(), session_.closed_streams()->front()->id()); @@ -2342,7 +2338,6 @@ // QUIC it sends both a RST_STREAM and a STOP_SENDING (each of which // closes in only one direction). stream4->Reset(QUIC_STREAM_CANCELLED); - EXPECT_FALSE(QuicContainsKey(session_.zombie_streams(), stream4->id())); EXPECT_EQ(2u, session_.closed_streams()->size()); } @@ -2590,13 +2585,9 @@ stream2->WriteOrBufferData(body, true, nullptr); EXPECT_TRUE(stream2->IsWaitingForAcks()); // Verify stream2 is a zombie streams. - if (!session_.remove_zombie_streams()) { - EXPECT_TRUE(QuicContainsKey(session_.zombie_streams(), stream2->id())); - } else { - ASSERT_TRUE(QuicContainsKey(session_.stream_map(), stream2->id())); - auto* stream = session_.stream_map().find(stream2->id())->second.get(); - EXPECT_TRUE(stream->IsZombie()); - } + ASSERT_TRUE(QuicContainsKey(session_.stream_map(), stream2->id())); + auto* stream = session_.stream_map().find(stream2->id())->second.get(); + EXPECT_TRUE(stream->IsZombie()); QuicStreamFrame frame(stream2->id(), true, 0, 100); EXPECT_CALL(*stream2, HasPendingRetransmission()) @@ -2610,7 +2601,6 @@ stream2->Reset(QUIC_STREAM_CANCELLED); // Verify stream 2 gets closed. - EXPECT_FALSE(QuicContainsKey(session_.zombie_streams(), stream2->id())); EXPECT_TRUE(session_.IsClosedStream(stream2->id())); EXPECT_CALL(*stream2, OnCanWrite()).Times(0); session_.OnCanWrite(); @@ -2625,7 +2615,6 @@ EXPECT_FALSE(stream2->IsWaitingForAcks()); CloseStream(stream2->id()); - EXPECT_FALSE(QuicContainsKey(session_.zombie_streams(), stream2->id())); EXPECT_EQ(1u, session_.closed_streams()->size()); EXPECT_TRUE( QuicSessionPeer::GetCleanUpClosedStreamsAlarm(&session_)->IsSet()); @@ -2642,15 +2631,10 @@ session_.ActivateStream(QuicWrapUnique(stream4)); std::string body(100, '.'); stream4->WriteOrBufferData(body, false, nullptr); - EXPECT_FALSE(QuicContainsKey(session_.zombie_streams(), stream4->id())); stream4->WriteOrBufferData(body, true, nullptr); - if (!session_.remove_zombie_streams()) { - EXPECT_TRUE(QuicContainsKey(session_.zombie_streams(), stream4->id())); - } else { - ASSERT_TRUE(QuicContainsKey(session_.stream_map(), stream4->id())); - auto* stream = session_.stream_map().find(stream4->id())->second.get(); - EXPECT_TRUE(stream->IsZombie()); - } + ASSERT_TRUE(QuicContainsKey(session_.stream_map(), stream4->id())); + auto* stream = session_.stream_map().find(stream4->id())->second.get(); + EXPECT_TRUE(stream->IsZombie()); } TEST_P(QuicSessionTestServer, ReceivedDataOnWriteUnidirectionalStream) {
diff --git a/quic/core/quic_stream.cc b/quic/core/quic_stream.cc index 9849239..d973549 100644 --- a/quic/core/quic_stream.cc +++ b/quic/core/quic_stream.cc
@@ -1018,8 +1018,7 @@ fin_outstanding_ = false; fin_lost_ = false; } - if (!IsWaitingForAcks() && (!session()->remove_zombie_streams() || - (read_side_closed_ && write_side_closed_))) { + if (!IsWaitingForAcks() && read_side_closed_ && write_side_closed_) { session_->OnStreamDoneWaitingForAcks(id_); } return new_data_acked;
diff --git a/quic/quic_transport/quic_transport_client_session_test.cc b/quic/quic_transport/quic_transport_client_session_test.cc index 9c2f138..fe85576 100644 --- a/quic/quic_transport/quic_transport_client_session_test.cc +++ b/quic/quic_transport/quic_transport_client_session_test.cc
@@ -109,15 +109,9 @@ EXPECT_TRUE(session_->IsSessionReady()); QuicStream* client_indication_stream; - if (session_->remove_zombie_streams()) { - client_indication_stream = - QuicSessionPeer::stream_map(session_.get())[ClientIndicationStream()] - .get(); - } else { - client_indication_stream = QuicSessionPeer::zombie_streams( - session_.get())[ClientIndicationStream()] - .get(); - } + client_indication_stream = + QuicSessionPeer::stream_map(session_.get())[ClientIndicationStream()] + .get(); ASSERT_TRUE(client_indication_stream != nullptr); const std::string client_indication = DataInStream(client_indication_stream); const std::string expected_client_indication{ @@ -141,15 +135,9 @@ EXPECT_TRUE(session_->IsSessionReady()); QuicStream* client_indication_stream; - if (session_->remove_zombie_streams()) { - client_indication_stream = - QuicSessionPeer::stream_map(session_.get())[ClientIndicationStream()] - .get(); - } else { - client_indication_stream = QuicSessionPeer::zombie_streams( - session_.get())[ClientIndicationStream()] - .get(); - } + client_indication_stream = + QuicSessionPeer::stream_map(session_.get())[ClientIndicationStream()] + .get(); ASSERT_TRUE(client_indication_stream != nullptr); const std::string client_indication = DataInStream(client_indication_stream); const std::string expected_client_indication{
diff --git a/quic/test_tools/quic_session_peer.cc b/quic/test_tools/quic_session_peer.cc index 6823691..05d74d7 100644 --- a/quic/test_tools/quic_session_peer.cc +++ b/quic/test_tools/quic_session_peer.cc
@@ -137,12 +137,6 @@ } // static -QuicSession::ZombieStreamMap& QuicSessionPeer::zombie_streams( - QuicSession* session) { - return session->zombie_streams_; -} - -// static void QuicSessionPeer::ActivateStream(QuicSession* session, std::unique_ptr<QuicStream> stream) { return session->ActivateStream(std::move(stream));
diff --git a/quic/test_tools/quic_session_peer.h b/quic/test_tools/quic_session_peer.h index 1366ddb..ffd6c6f 100644 --- a/quic/test_tools/quic_session_peer.h +++ b/quic/test_tools/quic_session_peer.h
@@ -58,7 +58,6 @@ GetLocallyClosedStreamsHighestOffset(QuicSession* session); static QuicSession::StreamMap& stream_map(QuicSession* session); static const QuicSession::ClosedStreams& closed_streams(QuicSession* session); - static QuicSession::ZombieStreamMap& zombie_streams(QuicSession* session); static void ActivateStream(QuicSession* session, std::unique_ptr<QuicStream> stream);