Deprecate gfe2_reloadable_flag_quic_stream_id_manager_handles_accounting. PiperOrigin-RevId: 314205564 Change-Id: I10fc6c0c96255bb5bee3bdc8aa2e278bb705cf98
diff --git a/quic/core/http/end_to_end_test.cc b/quic/core/http/end_to_end_test.cc index f527271..8b350ee 100644 --- a/quic/core/http/end_to_end_test.cc +++ b/quic/core/http/end_to_end_test.cc
@@ -3307,7 +3307,9 @@ // though more resources need to be pushed. if (!version_.HasIetfQuicFrames()) { server_thread_->Pause(); - EXPECT_EQ(kNumMaxStreams, GetServerSession()->GetNumOpenOutgoingStreams()); + EXPECT_EQ(kNumMaxStreams, + QuicSessionPeer::GetStreamIdManager(GetServerSession()) + ->num_open_outgoing_streams()); server_thread_->Resume(); }
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc index ba22c81..f35e2fc 100644 --- a/quic/core/http/quic_spdy_session_test.cc +++ b/quic/core/http/quic_spdy_session_test.cc
@@ -240,7 +240,7 @@ TestStream* CreateIncomingStream(QuicStreamId id) override { // Enforce the limit on the number of open streams. if (!VersionHasIetfQuicFrames(connection()->transport_version()) && - GetNumOpenIncomingStreams() + 1 > + stream_id_manager().num_open_incoming_streams() + 1 > max_open_incoming_bidirectional_streams()) { connection()->CloseConnection( QUIC_TOO_MANY_OPEN_STREAMS, "Too many streams!",
diff --git a/quic/core/legacy_quic_stream_id_manager.cc b/quic/core/legacy_quic_stream_id_manager.cc index 6120f85..af4de8b 100644 --- a/quic/core/legacy_quic_stream_id_manager.cc +++ b/quic/core/legacy_quic_stream_id_manager.cc
@@ -31,42 +31,20 @@ : QuicUtils::GetCryptoStreamId(transport_version_)) : QuicUtils::GetInvalidStreamId(transport_version_)), num_open_incoming_streams_(0), - num_open_outgoing_streams_(0), - handles_accounting_( - GetQuicReloadableFlag(quic_stream_id_manager_handles_accounting)) { - if (handles_accounting_) { - QUIC_RELOADABLE_FLAG_COUNT(quic_stream_id_manager_handles_accounting); - } -} + num_open_outgoing_streams_(0) {} LegacyQuicStreamIdManager::~LegacyQuicStreamIdManager() {} -bool LegacyQuicStreamIdManager::CanOpenNextOutgoingStream( - size_t current_num_open_outgoing_streams) const { - if (handles_accounting_) { - DCHECK_LE(num_open_outgoing_streams_, max_open_outgoing_streams_); - QUIC_DLOG_IF(INFO, num_open_outgoing_streams_ == max_open_outgoing_streams_) - << "Failed to create a new outgoing stream. " - << "Already " << num_open_outgoing_streams_ << " open."; - return num_open_outgoing_streams_ < max_open_outgoing_streams_; - } - if (current_num_open_outgoing_streams >= max_open_outgoing_streams_) { - QUIC_DLOG(INFO) << "Failed to create a new outgoing stream. " - << "Already " << current_num_open_outgoing_streams - << " open."; - return false; - } - return true; +bool LegacyQuicStreamIdManager::CanOpenNextOutgoingStream() const { + DCHECK_LE(num_open_outgoing_streams_, max_open_outgoing_streams_); + QUIC_DLOG_IF(INFO, num_open_outgoing_streams_ == max_open_outgoing_streams_) + << "Failed to create a new outgoing stream. " + << "Already " << num_open_outgoing_streams_ << " open."; + return num_open_outgoing_streams_ < max_open_outgoing_streams_; } -bool LegacyQuicStreamIdManager::CanOpenIncomingStream( - size_t current_num_open_incoming_streams) const { - if (handles_accounting_) { - return num_open_incoming_streams_ < max_open_incoming_streams_; - } - // Check if the new number of open streams would cause the number of - // open streams to exceed the limit. - return current_num_open_incoming_streams < max_open_incoming_streams_; +bool LegacyQuicStreamIdManager::CanOpenIncomingStream() const { + return num_open_incoming_streams_ < max_open_incoming_streams_; } bool LegacyQuicStreamIdManager::MaybeIncreaseLargestPeerStreamId( @@ -121,7 +99,6 @@ } void LegacyQuicStreamIdManager::ActivateStream(bool is_incoming) { - DCHECK(handles_accounting_); if (is_incoming) { ++num_open_incoming_streams_; return; @@ -130,7 +107,6 @@ } void LegacyQuicStreamIdManager::OnStreamClosed(bool is_incoming) { - DCHECK(handles_accounting_); if (is_incoming) { QUIC_BUG_IF(num_open_incoming_streams_ == 0); --num_open_incoming_streams_;
diff --git a/quic/core/legacy_quic_stream_id_manager.h b/quic/core/legacy_quic_stream_id_manager.h index 6c1309e..0131587 100644 --- a/quic/core/legacy_quic_stream_id_manager.h +++ b/quic/core/legacy_quic_stream_id_manager.h
@@ -29,11 +29,10 @@ ~LegacyQuicStreamIdManager(); // Returns true if the next outgoing stream ID can be allocated. - bool CanOpenNextOutgoingStream( - size_t current_num_open_outgoing_streams) const; + bool CanOpenNextOutgoingStream() const; // Returns true if a new incoming stream can be opened. - bool CanOpenIncomingStream(size_t current_num_open_incoming_streams) const; + bool CanOpenIncomingStream() const; // Returns false when increasing the largest created stream id to |id| would // violate the limit, so the connection should be closed. @@ -95,8 +94,6 @@ return num_open_outgoing_streams_; } - bool handles_accounting() const { return handles_accounting_; } - private: friend class test::QuicSessionPeer; @@ -118,16 +115,11 @@ QuicStreamId largest_peer_created_stream_id_; - // A counter for peer initiated open streams. Used when handles_accounting_ is - // true. + // A counter for peer initiated open streams. size_t num_open_incoming_streams_; - // A counter for self initiated open streams. Used when handles_accounting_ is - // true. + // A counter for self initiated open streams. size_t num_open_outgoing_streams_; - - // Latched value of quic_stream_id_manager_handles_accounting. - const bool handles_accounting_; }; } // namespace quic
diff --git a/quic/core/legacy_quic_stream_id_manager_test.cc b/quic/core/legacy_quic_stream_id_manager_test.cc index 00654b4..b7ba1d2 100644 --- a/quic/core/legacy_quic_stream_id_manager_test.cc +++ b/quic/core/legacy_quic_stream_id_manager_test.cc
@@ -78,33 +78,21 @@ ::testing::PrintToStringParamName()); TEST_P(LegacyQuicStreamIdManagerTest, CanOpenNextOutgoingStream) { - if (GetQuicReloadableFlag(quic_stream_id_manager_handles_accounting)) { - for (size_t i = 0; i < manager_.max_open_outgoing_streams() - 1; ++i) { - manager_.ActivateStream(/*is_incoming=*/false); - } - } - EXPECT_TRUE(manager_.CanOpenNextOutgoingStream( - manager_.max_open_outgoing_streams() - 1)); - if (GetQuicReloadableFlag(quic_stream_id_manager_handles_accounting)) { + for (size_t i = 0; i < manager_.max_open_outgoing_streams() - 1; ++i) { manager_.ActivateStream(/*is_incoming=*/false); } - EXPECT_FALSE( - manager_.CanOpenNextOutgoingStream(manager_.max_open_outgoing_streams())); + EXPECT_TRUE(manager_.CanOpenNextOutgoingStream()); + manager_.ActivateStream(/*is_incoming=*/false); + EXPECT_FALSE(manager_.CanOpenNextOutgoingStream()); } TEST_P(LegacyQuicStreamIdManagerTest, CanOpenIncomingStream) { - if (GetQuicReloadableFlag(quic_stream_id_manager_handles_accounting)) { - for (size_t i = 0; i < manager_.max_open_incoming_streams() - 1; ++i) { - manager_.ActivateStream(/*is_incoming=*/true); - } - } - EXPECT_TRUE( - manager_.CanOpenIncomingStream(manager_.max_open_incoming_streams() - 1)); - if (GetQuicReloadableFlag(quic_stream_id_manager_handles_accounting)) { + for (size_t i = 0; i < manager_.max_open_incoming_streams() - 1; ++i) { manager_.ActivateStream(/*is_incoming=*/true); } - EXPECT_FALSE( - manager_.CanOpenIncomingStream(manager_.max_open_incoming_streams())); + EXPECT_TRUE(manager_.CanOpenIncomingStream()); + manager_.ActivateStream(/*is_incoming=*/true); + EXPECT_FALSE(manager_.CanOpenIncomingStream()); } TEST_P(LegacyQuicStreamIdManagerTest, AvailableStreams) {
diff --git a/quic/core/quic_session.cc b/quic/core/quic_session.cc index 7fb52dd..218384f 100644 --- a/quic/core/quic_session.cc +++ b/quic/core/quic_session.cc
@@ -73,12 +73,10 @@ config_.GetMaxBidirectionalStreamsToSend(), config_.GetMaxUnidirectionalStreamsToSend() + num_expected_unidirectional_static_streams), - num_dynamic_incoming_streams_(0), num_draining_incoming_streams_(0), num_draining_outgoing_streams_(0), num_outgoing_static_streams_(0), num_incoming_static_streams_(0), - num_locally_closed_incoming_streams_highest_offset_(0), flow_controller_( this, QuicUtils::GetInvalidStreamId(connection->transport_version()), @@ -137,16 +135,6 @@ QuicSession::~QuicSession() { QUIC_LOG_IF(WARNING, !zombie_streams_.empty()) << "Still have zombie streams"; - QUIC_LOG_IF(WARNING, num_locally_closed_incoming_streams_highest_offset() > - stream_id_manager_.max_open_incoming_streams()) - << "Surprisingly high number of locally closed peer initiated streams" - "still waiting for final byte offset: " - << num_locally_closed_incoming_streams_highest_offset(); - QUIC_LOG_IF(WARNING, GetNumLocallyClosedOutgoingStreamsHighestOffset() > - stream_id_manager_.max_open_outgoing_streams()) - << "Surprisingly high number of locally closed self initiated streams" - "still waiting for final byte offset: " - << GetNumLocallyClosedOutgoingStreamsHighestOffset(); } void QuicSession::PendingStreamOnStreamFrame(const QuicStreamFrame& frame) { @@ -879,9 +867,6 @@ const QuicStreamId id, QuicStreamOffset offset) { locally_closed_streams_highest_offset_[id] = offset; - if (IsIncomingStream(id)) { - ++num_locally_closed_incoming_streams_highest_offset_; - } } void QuicSession::OnStreamClosed(QuicStreamId stream_id) { @@ -918,9 +903,6 @@ InsertLocallyClosedStreamsHighestOffset( stream_id, stream->flow_controller()->highest_received_byte_offset()); stream_map_.erase(it); - if (IsIncomingStream(stream_id)) { - --num_dynamic_incoming_streams_; - } return; } @@ -928,9 +910,6 @@ QUIC_DVLOG_IF(1, stream_was_draining) << ENDPOINT << "Stream " << stream_id << " was draining"; stream_map_.erase(it); - if (IsIncomingStream(stream_id)) { - --num_dynamic_incoming_streams_; - } if (stream_was_draining) { if (IsIncomingStream(stream_id)) { QUIC_BUG_IF(num_draining_incoming_streams_ == 0); @@ -948,8 +927,7 @@ // disconnected. return; } - if (stream_id_manager_.handles_accounting() && - !VersionHasIetfQuicFrames(transport_version())) { + if (!VersionHasIetfQuicFrames(transport_version())) { stream_id_manager_.OnStreamClosed( /*is_incoming=*/IsIncomingStream(stream_id)); } @@ -1004,13 +982,11 @@ flow_controller_.AddBytesConsumed(offset_diff); locally_closed_streams_highest_offset_.erase(it); - if (stream_id_manager_.handles_accounting() && - !VersionHasIetfQuicFrames(transport_version())) { + if (!VersionHasIetfQuicFrames(transport_version())) { stream_id_manager_.OnStreamClosed( /*is_incoming=*/IsIncomingStream(stream_id)); } if (IsIncomingStream(stream_id)) { - --num_locally_closed_incoming_streams_highest_offset_; if (VersionHasIetfQuicFrames(transport_version())) { v99_streamid_manager_.OnStreamClosed(stream_id); } @@ -1588,14 +1564,12 @@ << ". activating stream " << stream_id; DCHECK(!QuicContainsKey(stream_map_, stream_id)); stream_map_[stream_id] = std::move(stream); - if (IsIncomingStream(stream_id)) { - is_static ? ++num_incoming_static_streams_ - : ++num_dynamic_incoming_streams_; - } else if (is_static) { - ++num_outgoing_static_streams_; + if (is_static) { + IsIncomingStream(stream_id) ? ++num_incoming_static_streams_ + : ++num_outgoing_static_streams_; + return; } - if (stream_id_manager_.handles_accounting() && !is_static && - !VersionHasIetfQuicFrames(transport_version())) { + if (!VersionHasIetfQuicFrames(transport_version())) { // Do not inform stream ID manager of static streams. stream_id_manager_.ActivateStream( /*is_incoming=*/IsIncomingStream(stream_id)); @@ -1618,8 +1592,7 @@ bool QuicSession::CanOpenNextOutgoingBidirectionalStream() { if (!VersionHasIetfQuicFrames(transport_version())) { - return stream_id_manager_.CanOpenNextOutgoingStream( - GetNumOpenOutgoingStreams()); + return stream_id_manager_.CanOpenNextOutgoingStream(); } if (v99_streamid_manager_.CanOpenNextOutgoingBidirectionalStream()) { return true; @@ -1635,8 +1608,7 @@ bool QuicSession::CanOpenNextOutgoingUnidirectionalStream() { if (!VersionHasIetfQuicFrames(transport_version())) { - return stream_id_manager_.CanOpenNextOutgoingStream( - GetNumOpenOutgoingStreams()); + return stream_id_manager_.CanOpenNextOutgoingStream(); } if (v99_streamid_manager_.CanOpenNextOutgoingUnidirectionalStream()) { return true; @@ -1685,15 +1657,11 @@ return nullptr; } - if (!VersionHasIetfQuicFrames(transport_version())) { - // TODO(fayang): Let LegacyQuicStreamIdManager count open streams and make - // CanOpenIncomingStream interface consistent with that of v99. - if (!stream_id_manager_.CanOpenIncomingStream( - GetNumOpenIncomingStreams())) { - // Refuse to open the stream. - ResetStream(stream_id, QUIC_REFUSED_STREAM, 0); - return nullptr; - } + if (!VersionHasIetfQuicFrames(transport_version()) && + !stream_id_manager_.CanOpenIncomingStream()) { + // Refuse to open the stream. + ResetStream(stream_id, QUIC_REFUSED_STREAM, 0); + return nullptr; } return CreateIncomingStream(stream_id); @@ -1704,7 +1672,7 @@ QUIC_DVLOG(1) << ENDPOINT << "Stream " << stream_id << " is draining"; if (VersionHasIetfQuicFrames(transport_version())) { v99_streamid_manager_.OnStreamClosed(stream_id); - } else if (stream_id_manager_.handles_accounting()) { + } else { stream_id_manager_.OnStreamClosed( /*is_incoming=*/IsIncomingStream(stream_id)); } @@ -1830,31 +1798,8 @@ return it->second->is_static(); } -size_t QuicSession::GetNumOpenIncomingStreams() const { - DCHECK(!VersionHasIetfQuicFrames(transport_version())); - if (stream_id_manager_.handles_accounting()) { - return stream_id_manager_.num_open_incoming_streams(); - } - return num_dynamic_incoming_streams_ - num_draining_incoming_streams_ + - num_locally_closed_incoming_streams_highest_offset_; -} - -size_t QuicSession::GetNumOpenOutgoingStreams() const { - DCHECK(!VersionHasIetfQuicFrames(transport_version())); - if (stream_id_manager_.handles_accounting()) { - return stream_id_manager_.num_open_outgoing_streams(); - } - DCHECK_GE(GetNumDynamicOutgoingStreams() + - GetNumLocallyClosedOutgoingStreamsHighestOffset(), - GetNumDrainingOutgoingStreams()); - return GetNumDynamicOutgoingStreams() + - GetNumLocallyClosedOutgoingStreamsHighestOffset() - - GetNumDrainingOutgoingStreams(); -} - size_t QuicSession::GetNumActiveStreams() const { - if (!VersionHasIetfQuicFrames(transport_version()) && - stream_id_manager_.handles_accounting()) { + if (!VersionHasIetfQuicFrames(transport_version())) { // Exclude locally_closed_streams when determine whether to keep connection // alive. return stream_id_manager_.num_open_incoming_streams() + @@ -1897,27 +1842,10 @@ control_frame_manager_.WritePing(); } -size_t QuicSession::GetNumDynamicOutgoingStreams() const { - DCHECK_GE( - static_cast<size_t>(stream_map_.size() + pending_stream_map_.size()), - num_dynamic_incoming_streams_ + num_outgoing_static_streams_ + - num_incoming_static_streams_); - return stream_map_.size() + pending_stream_map_.size() - - num_dynamic_incoming_streams_ - num_outgoing_static_streams_ - - num_incoming_static_streams_; -} - size_t QuicSession::GetNumDrainingOutgoingStreams() const { return num_draining_outgoing_streams_; } -size_t QuicSession::GetNumLocallyClosedOutgoingStreamsHighestOffset() const { - DCHECK_GE(locally_closed_streams_highest_offset_.size(), - num_locally_closed_incoming_streams_highest_offset_); - return locally_closed_streams_highest_offset_.size() - - num_locally_closed_incoming_streams_highest_offset_; -} - bool QuicSession::IsConnectionFlowControlBlocked() const { return flow_controller_.IsBlocked(); }
diff --git a/quic/core/quic_session.h b/quic/core/quic_session.h index d7400d5..c156a6e 100644 --- a/quic/core/quic_session.h +++ b/quic/core/quic_session.h
@@ -334,20 +334,6 @@ // Returns the number of currently draining streams. size_t GetNumDrainingStreams() const; - // Returns the number of currently open peer initiated streams, excluding - // static streams. - // TODO(fayang): remove this and instead use - // LegacyStreamIdManager::num_open_incoming_streams() in tests when - // deprecating quic_stream_id_manager_handles_accounting. - size_t GetNumOpenIncomingStreams() const; - - // Returns the number of currently open self initiated streams, excluding - // static streams. - // TODO(fayang): remove this and instead use - // LegacyStreamIdManager::num_open_outgoing_streams() in tests when - // deprecating quic_stream_id_manager_handles_accounting. - size_t GetNumOpenOutgoingStreams() const; - // Returns the number of open peer initiated static streams. size_t num_incoming_static_streams() const { return num_incoming_static_streams_; @@ -448,12 +434,6 @@ // Return true if given stream is peer initiated. bool IsIncomingStream(QuicStreamId id) const; - size_t GetNumLocallyClosedOutgoingStreamsHighestOffset() const; - - size_t num_locally_closed_incoming_streams_highest_offset() const { - return num_locally_closed_incoming_streams_highest_offset_; - } - // Record errors when a connection is closed at the server side, should only // be called from server's perspective. // Noop if |error| is QUIC_NO_ERROR. @@ -591,8 +571,6 @@ return &write_blocked_streams_; } - size_t GetNumDynamicOutgoingStreams() const; - size_t GetNumDrainingOutgoingStreams() const; // Returns true if the stream is still active. @@ -772,37 +750,26 @@ // Manages stream IDs for version99/IETF QUIC UberQuicStreamIdManager v99_streamid_manager_; - // A counter for peer initiated dynamic streams which are in the stream_map_. - // TODO(fayang): Remove this when deprecating - // quic_stream_id_manager_handles_accounting. - size_t num_dynamic_incoming_streams_; - // A counter for peer initiated streams which have sent and received FIN but // waiting for application to consume data. - // TODO(fayang): Remove this when deprecating - // quic_stream_id_manager_handles_accounting. + // TODO(fayang): Merge num_draining_incoming_streams_ and + // num_draining_outgoing_streams_. size_t num_draining_incoming_streams_; // A counter for self initiated streams which have sent and received FIN but // waiting for application to consume data. - // TODO(fayang): Remove this when deprecating - // quic_stream_id_manager_handles_accounting. size_t num_draining_outgoing_streams_; // A counter for self initiated static streams which are in // stream_map_. + // TODO(fayang): Merge num_outgoing_static_streams_ and + // num_incoming_static_streams_. size_t num_outgoing_static_streams_; // A counter for peer initiated static streams which are in // stream_map_. size_t num_incoming_static_streams_; - // A counter for peer initiated streams which are in the - // locally_closed_streams_highest_offset_. - // TODO(fayang): Remove this when deprecating - // quic_stream_id_manager_handles_accounting. - size_t num_locally_closed_incoming_streams_highest_offset_; - // Received information for a connection close. QuicConnectionCloseFrame on_closed_frame_;
diff --git a/quic/core/quic_session_test.cc b/quic/core/quic_session_test.cc index ad10f43..d2ff9f8 100644 --- a/quic/core/quic_session_test.cc +++ b/quic/core/quic_session_test.cc
@@ -230,7 +230,7 @@ TestStream* CreateIncomingStream(QuicStreamId id) override { // Enforce the limit on the number of open streams. if (!VersionHasIetfQuicFrames(connection()->transport_version()) && - GetNumOpenIncomingStreams() + 1 > + stream_id_manager().num_open_incoming_streams() + 1 > max_open_incoming_bidirectional_streams()) { // No need to do this test for version 99; it's done by // QuicSession::GetOrCreateStream.