Remove QuicSession from LegacyQuicStreamIdManager and pass in transport_version and perspective instead. gfe-relnote: n/a (Refactor) PiperOrigin-RevId: 284747835 Change-Id: I5baf02f87cf72cdc02d6b8ddbd6befcccca199f3
diff --git a/quic/core/legacy_quic_stream_id_manager.cc b/quic/core/legacy_quic_stream_id_manager.cc index 157e375..b8498b3 100644 --- a/quic/core/legacy_quic_stream_id_manager.cc +++ b/quic/core/legacy_quic_stream_id_manager.cc
@@ -4,47 +4,33 @@ #include "net/third_party/quiche/src/quic/core/legacy_quic_stream_id_manager.h" #include "net/third_party/quiche/src/quic/core/quic_session.h" +#include "net/third_party/quiche/src/quic/core/quic_types.h" #include "net/third_party/quiche/src/quic/core/quic_utils.h" +#include "net/third_party/quiche/src/quic/core/quic_versions.h" #include "net/third_party/quiche/src/quic/platform/api/quic_map_util.h" namespace quic { -#define ENDPOINT \ - (session_->perspective() == Perspective::IS_SERVER ? "Server: " : "Client: ") - LegacyQuicStreamIdManager::LegacyQuicStreamIdManager( - QuicSession* session, + Perspective perspective, + QuicTransportVersion transport_version, size_t max_open_outgoing_streams, size_t max_open_incoming_streams) - : session_(session), + : perspective_(perspective), + transport_version_(transport_version), max_open_outgoing_streams_(max_open_outgoing_streams), max_open_incoming_streams_(max_open_incoming_streams), next_outgoing_stream_id_( - QuicUtils::GetFirstBidirectionalStreamId(session->transport_version(), - session->perspective())), + QuicUtils::GetFirstBidirectionalStreamId(transport_version_, + perspective_)), largest_peer_created_stream_id_( - session->perspective() == Perspective::IS_SERVER - ? (QuicVersionUsesCryptoFrames(session->transport_version()) - ? QuicUtils::GetInvalidStreamId( - session->transport_version()) - : QuicUtils::GetCryptoStreamId( - session->transport_version())) - : QuicUtils::GetInvalidStreamId(session->transport_version())) {} + perspective_ == Perspective::IS_SERVER + ? (QuicVersionUsesCryptoFrames(transport_version_) + ? QuicUtils::GetInvalidStreamId(transport_version_) + : QuicUtils::GetCryptoStreamId(transport_version_)) + : QuicUtils::GetInvalidStreamId(transport_version_)) {} -LegacyQuicStreamIdManager::~LegacyQuicStreamIdManager() { - QUIC_LOG_IF(WARNING, - session_->num_locally_closed_incoming_streams_highest_offset() > - max_open_incoming_streams_) - << "Surprisingly high number of locally closed peer initiated streams" - "still waiting for final byte offset: " - << session_->num_locally_closed_incoming_streams_highest_offset(); - QUIC_LOG_IF(WARNING, - session_->GetNumLocallyClosedOutgoingStreamsHighestOffset() > - max_open_outgoing_streams_) - << "Surprisingly high number of locally closed self initiated streams" - "still waiting for final byte offset: " - << session_->GetNumLocallyClosedOutgoingStreamsHighestOffset(); -} +LegacyQuicStreamIdManager::~LegacyQuicStreamIdManager() {} bool LegacyQuicStreamIdManager::CanOpenNextOutgoingStream( size_t current_num_open_outgoing_streams) const { @@ -69,7 +55,7 @@ available_streams_.erase(stream_id); if (largest_peer_created_stream_id_ != - QuicUtils::GetInvalidStreamId(session_->transport_version()) && + QuicUtils::GetInvalidStreamId(transport_version_) && stream_id <= largest_peer_created_stream_id_) { return true; } @@ -80,31 +66,26 @@ size_t additional_available_streams = (stream_id - largest_peer_created_stream_id_) / 2 - 1; if (largest_peer_created_stream_id_ == - QuicUtils::GetInvalidStreamId(session_->transport_version())) { + QuicUtils::GetInvalidStreamId(transport_version_)) { additional_available_streams = (stream_id + 1) / 2 - 1; } size_t new_num_available_streams = GetNumAvailableStreams() + additional_available_streams; if (new_num_available_streams > MaxAvailableStreams()) { - QUIC_DLOG(INFO) << ENDPOINT + QUIC_DLOG(INFO) << perspective_ << "Failed to create a new incoming stream with id:" << stream_id << ". There are already " << GetNumAvailableStreams() << " streams available, which would become " << new_num_available_streams << ", which exceeds the limit " << MaxAvailableStreams() << "."; - session_->connection()->CloseConnection( - QUIC_TOO_MANY_AVAILABLE_STREAMS, - QuicStrCat(new_num_available_streams, " above ", MaxAvailableStreams()), - ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); return false; } QuicStreamId first_available_stream = largest_peer_created_stream_id_ + 2; if (largest_peer_created_stream_id_ == - QuicUtils::GetInvalidStreamId(session_->transport_version())) { + QuicUtils::GetInvalidStreamId(transport_version_)) { first_available_stream = QuicUtils::GetFirstBidirectionalStreamId( - session_->transport_version(), - QuicUtils::InvertPerspective(session_->perspective())); + transport_version_, QuicUtils::InvertPerspective(perspective_)); } for (QuicStreamId id = first_available_stream; id < stream_id; id += 2) { available_streams_.insert(id); @@ -128,7 +109,7 @@ } // For peer created streams, we also need to consider available streams. return largest_peer_created_stream_id_ == - QuicUtils::GetInvalidStreamId(session_->transport_version()) || + QuicUtils::GetInvalidStreamId(transport_version_) || id > largest_peer_created_stream_id_ || QuicContainsKey(available_streams_, id); }
diff --git a/quic/core/legacy_quic_stream_id_manager.h b/quic/core/legacy_quic_stream_id_manager.h index 78d4545..1a94905 100644 --- a/quic/core/legacy_quic_stream_id_manager.h +++ b/quic/core/legacy_quic_stream_id_manager.h
@@ -5,6 +5,8 @@ #define QUICHE_QUIC_CORE_LEGACY_QUIC_STREAM_ID_MANAGER_H_ #include "net/third_party/quiche/src/quic/core/quic_stream_id_manager.h" +#include "net/third_party/quiche/src/quic/core/quic_types.h" +#include "net/third_party/quiche/src/quic/core/quic_versions.h" namespace quic { @@ -19,7 +21,8 @@ // next outgoing stream ID) and 2) can a new incoming stream be opened. class QUIC_EXPORT_PRIVATE LegacyQuicStreamIdManager { public: - LegacyQuicStreamIdManager(QuicSession* session, + LegacyQuicStreamIdManager(Perspective perspective, + QuicTransportVersion transport_version, size_t max_open_outgoing_streams, size_t max_open_incoming_streams); @@ -32,6 +35,8 @@ // Returns true if a new incoming stream can be opened. bool CanOpenIncomingStream(size_t current_num_open_incoming_streams) const; + // Returns false when increasing the largest created stream id to |id| would + // violate the limit, so the connection should be closed. bool MaybeIncreaseLargestPeerStreamId(const QuicStreamId id); // Returns true if |id| is still available. @@ -75,13 +80,13 @@ return largest_peer_created_stream_id_; } + size_t GetNumAvailableStreams() const; + private: friend class test::QuicSessionPeer; - size_t GetNumAvailableStreams() const; - - // Not owned. - QuicSession* session_; + const Perspective perspective_; + const QuicTransportVersion transport_version_; // The maximum number of outgoing streams this connection can open. size_t max_open_outgoing_streams_;
diff --git a/quic/core/legacy_quic_stream_id_manager_test.cc b/quic/core/legacy_quic_stream_id_manager_test.cc index 6a861a5..0270774 100644 --- a/quic/core/legacy_quic_stream_id_manager_test.cc +++ b/quic/core/legacy_quic_stream_id_manager_test.cc
@@ -7,6 +7,7 @@ #include <utility> #include "net/third_party/quiche/src/quic/core/quic_utils.h" +#include "net/third_party/quiche/src/quic/core/quic_versions.h" #include "net/third_party/quiche/src/quic/platform/api/quic_test.h" #include "net/third_party/quiche/src/quic/test_tools/quic_session_peer.h" #include "net/third_party/quiche/src/quic/test_tools/quic_test_utils.h" @@ -19,64 +20,64 @@ using testing::StrictMock; class LegacyQuicStreamIdManagerTest : public QuicTest { - protected: - void Initialize(Perspective perspective) { + public: + LegacyQuicStreamIdManagerTest(Perspective perspective, + QuicTransportVersion transport_version) + : transport_version_(transport_version), + manager_(perspective, + transport_version, + kDefaultMaxStreamsPerConnection, + kDefaultMaxStreamsPerConnection) { // LegacyQuicStreamIdManager is only used for versions < 99. // TODO(b/145768765) parameterize this test by version. SetQuicReloadableFlag(quic_enable_version_q099, false); SetQuicReloadableFlag(quic_enable_version_t099, false); - connection_ = new MockQuicConnection( - &helper_, &alarm_factory_, perspective, - ParsedVersionOfIndex(CurrentSupportedVersions(), 0)); - session_ = std::make_unique<StrictMock<MockQuicSession>>(connection_); - manager_ = QuicSessionPeer::GetStreamIdManager(session_.get()); - session_->Initialize(); } + protected: QuicStreamId GetNthClientInitiatedId(int n) { - return QuicUtils::GetFirstBidirectionalStreamId( - connection_->transport_version(), Perspective::IS_CLIENT) + + return QuicUtils::GetFirstBidirectionalStreamId(transport_version_, + Perspective::IS_CLIENT) + 2 * n; } QuicStreamId GetNthServerInitiatedId(int n) { return 2 + 2 * n; } - MockQuicConnectionHelper helper_; - MockAlarmFactory alarm_factory_; - MockQuicConnection* connection_; - std::unique_ptr<StrictMock<MockQuicSession>> session_; - LegacyQuicStreamIdManager* manager_; + QuicTransportVersion transport_version_; + LegacyQuicStreamIdManager manager_; }; class LegacyQuicStreamIdManagerTestServer : public LegacyQuicStreamIdManagerTest { protected: - LegacyQuicStreamIdManagerTestServer() { Initialize(Perspective::IS_SERVER); } + LegacyQuicStreamIdManagerTestServer() + : LegacyQuicStreamIdManagerTest(Perspective::IS_SERVER, + QuicTransportVersion::QUIC_VERSION_46) {} }; TEST_F(LegacyQuicStreamIdManagerTestServer, CanOpenNextOutgoingStream) { - EXPECT_TRUE(manager_->CanOpenNextOutgoingStream( - manager_->max_open_outgoing_streams() - 1)); - EXPECT_FALSE(manager_->CanOpenNextOutgoingStream( - manager_->max_open_outgoing_streams())); + EXPECT_TRUE(manager_.CanOpenNextOutgoingStream( + manager_.max_open_outgoing_streams() - 1)); + EXPECT_FALSE( + manager_.CanOpenNextOutgoingStream(manager_.max_open_outgoing_streams())); } TEST_F(LegacyQuicStreamIdManagerTestServer, CanOpenIncomingStream) { - EXPECT_TRUE(manager_->CanOpenIncomingStream( - manager_->max_open_incoming_streams() - 1)); + EXPECT_TRUE( + manager_.CanOpenIncomingStream(manager_.max_open_incoming_streams() - 1)); EXPECT_FALSE( - manager_->CanOpenIncomingStream(manager_->max_open_incoming_streams())); + manager_.CanOpenIncomingStream(manager_.max_open_incoming_streams())); } TEST_F(LegacyQuicStreamIdManagerTestServer, AvailableStreams) { ASSERT_TRUE( - manager_->MaybeIncreaseLargestPeerStreamId(GetNthClientInitiatedId(3))); - EXPECT_TRUE(manager_->IsAvailableStream(GetNthClientInitiatedId(1))); - EXPECT_TRUE(manager_->IsAvailableStream(GetNthClientInitiatedId(2))); + manager_.MaybeIncreaseLargestPeerStreamId(GetNthClientInitiatedId(3))); + EXPECT_TRUE(manager_.IsAvailableStream(GetNthClientInitiatedId(1))); + EXPECT_TRUE(manager_.IsAvailableStream(GetNthClientInitiatedId(2))); ASSERT_TRUE( - manager_->MaybeIncreaseLargestPeerStreamId(GetNthClientInitiatedId(2))); + manager_.MaybeIncreaseLargestPeerStreamId(GetNthClientInitiatedId(2))); ASSERT_TRUE( - manager_->MaybeIncreaseLargestPeerStreamId(GetNthClientInitiatedId(1))); + manager_.MaybeIncreaseLargestPeerStreamId(GetNthClientInitiatedId(1))); } TEST_F(LegacyQuicStreamIdManagerTestServer, MaxAvailableStreams) { @@ -84,17 +85,16 @@ // streams available. The server accepts slightly more than the negotiated // stream limit to deal with rare cases where a client FIN/RST is lost. const size_t kMaxStreamsForTest = 10; - session_->OnConfigNegotiated(); - const size_t kAvailableStreamLimit = manager_->MaxAvailableStreams(); + const size_t kAvailableStreamLimit = manager_.MaxAvailableStreams(); EXPECT_EQ( - manager_->max_open_incoming_streams() * kMaxAvailableStreamsMultiplier, - manager_->MaxAvailableStreams()); + manager_.max_open_incoming_streams() * kMaxAvailableStreamsMultiplier, + manager_.MaxAvailableStreams()); // The protocol specification requires that there can be at least 10 times // as many available streams as the connection's maximum open streams. EXPECT_LE(10 * kMaxStreamsForTest, kAvailableStreamLimit); EXPECT_TRUE( - manager_->MaybeIncreaseLargestPeerStreamId(GetNthClientInitiatedId(0))); + manager_.MaybeIncreaseLargestPeerStreamId(GetNthClientInitiatedId(0))); // Establish available streams up to the server's limit. const int kLimitingStreamId = @@ -102,85 +102,66 @@ // This exceeds the stream limit. In versions other than 99 // this is allowed. Version 99 hews to the IETF spec and does // not allow it. - EXPECT_TRUE(manager_->MaybeIncreaseLargestPeerStreamId(kLimitingStreamId)); - // A further available stream will result in connection close. - EXPECT_CALL(*connection_, - CloseConnection(QUIC_TOO_MANY_AVAILABLE_STREAMS, _, _)); + EXPECT_TRUE(manager_.MaybeIncreaseLargestPeerStreamId(kLimitingStreamId)); // This forces stream kLimitingStreamId + 2 to become available, which // violates the quota. EXPECT_FALSE( - manager_->MaybeIncreaseLargestPeerStreamId(kLimitingStreamId + 2 * 2)); + manager_.MaybeIncreaseLargestPeerStreamId(kLimitingStreamId + 2 * 2)); } TEST_F(LegacyQuicStreamIdManagerTestServer, MaximumAvailableOpenedStreams) { QuicStreamId stream_id = GetNthClientInitiatedId(0); - EXPECT_TRUE(manager_->MaybeIncreaseLargestPeerStreamId(stream_id)); + EXPECT_TRUE(manager_.MaybeIncreaseLargestPeerStreamId(stream_id)); - EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0); - EXPECT_TRUE(manager_->MaybeIncreaseLargestPeerStreamId( - stream_id + 2 * (manager_->max_open_incoming_streams() - 1))); + EXPECT_TRUE(manager_.MaybeIncreaseLargestPeerStreamId( + stream_id + 2 * (manager_.max_open_incoming_streams() - 1))); } TEST_F(LegacyQuicStreamIdManagerTestServer, TooManyAvailableStreams) { QuicStreamId stream_id = GetNthClientInitiatedId(0); - EXPECT_TRUE(manager_->MaybeIncreaseLargestPeerStreamId(stream_id)); + EXPECT_TRUE(manager_.MaybeIncreaseLargestPeerStreamId(stream_id)); // A stream ID which is too large to create. QuicStreamId stream_id2 = - GetNthClientInitiatedId(2 * manager_->MaxAvailableStreams() + 4); - EXPECT_CALL(*connection_, - CloseConnection(QUIC_TOO_MANY_AVAILABLE_STREAMS, _, _)); - EXPECT_FALSE(manager_->MaybeIncreaseLargestPeerStreamId(stream_id2)); + GetNthClientInitiatedId(2 * manager_.MaxAvailableStreams() + 4); + EXPECT_FALSE(manager_.MaybeIncreaseLargestPeerStreamId(stream_id2)); } TEST_F(LegacyQuicStreamIdManagerTestServer, ManyAvailableStreams) { // When max_open_streams_ is 200, should be able to create 200 streams // out-of-order, that is, creating the one with the largest stream ID first. - manager_->set_max_open_incoming_streams(200); + manager_.set_max_open_incoming_streams(200); QuicStreamId stream_id = GetNthClientInitiatedId(0); - EXPECT_TRUE(manager_->MaybeIncreaseLargestPeerStreamId(stream_id)); - EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0); + EXPECT_TRUE(manager_.MaybeIncreaseLargestPeerStreamId(stream_id)); // Create the largest stream ID of a threatened total of 200 streams. // GetNth... starts at 0, so for 200 streams, get the 199th. EXPECT_TRUE( - manager_->MaybeIncreaseLargestPeerStreamId(GetNthClientInitiatedId(199))); + manager_.MaybeIncreaseLargestPeerStreamId(GetNthClientInitiatedId(199))); } TEST_F(LegacyQuicStreamIdManagerTestServer, TestMaxIncomingAndOutgoingStreamsAllowed) { - // Tests that on server side, the value of max_open_incoming/outgoing - // streams are setup correctly during negotiation. The value for outgoing - // stream is limited to negotiated value and for incoming stream it is set to - // be larger than that. - session_->OnConfigNegotiated(); - // The max number of open outgoing streams is less than that of incoming - // streams, and it should be same as negotiated value. - EXPECT_LT(manager_->max_open_outgoing_streams(), - manager_->max_open_incoming_streams()); - EXPECT_EQ(manager_->max_open_outgoing_streams(), + EXPECT_EQ(manager_.max_open_incoming_streams(), kDefaultMaxStreamsPerConnection); - EXPECT_GT(manager_->max_open_incoming_streams(), + EXPECT_EQ(manager_.max_open_outgoing_streams(), kDefaultMaxStreamsPerConnection); } class LegacyQuicStreamIdManagerTestClient : public LegacyQuicStreamIdManagerTest { protected: - LegacyQuicStreamIdManagerTestClient() { Initialize(Perspective::IS_CLIENT); } + LegacyQuicStreamIdManagerTestClient() + : LegacyQuicStreamIdManagerTest(Perspective::IS_CLIENT, + QuicTransportVersion::QUIC_VERSION_46) {} }; TEST_F(LegacyQuicStreamIdManagerTestClient, TestMaxIncomingAndOutgoingStreamsAllowed) { - // Tests that on client side, the value of max_open_incoming/outgoing - // streams are setup correctly during negotiation. When flag is true, the - // value for outgoing stream is limited to negotiated value and for incoming - // stream it is set to be larger than that. - session_->OnConfigNegotiated(); - EXPECT_LT(manager_->max_open_outgoing_streams(), - manager_->max_open_incoming_streams()); - EXPECT_EQ(manager_->max_open_outgoing_streams(), + EXPECT_EQ(manager_.max_open_incoming_streams(), + kDefaultMaxStreamsPerConnection); + EXPECT_EQ(manager_.max_open_outgoing_streams(), kDefaultMaxStreamsPerConnection); }
diff --git a/quic/core/quic_session.cc b/quic/core/quic_session.cc index 6381bf2..c0f5992 100644 --- a/quic/core/quic_session.cc +++ b/quic/core/quic_session.cc
@@ -59,7 +59,8 @@ visitor_(owner), write_blocked_streams_(connection->transport_version()), config_(config), - stream_id_manager_(this, + stream_id_manager_(perspective(), + connection->transport_version(), kDefaultMaxStreamsPerConnection, config_.GetMaxIncomingBidirectionalStreamsToSend()), v99_streamid_manager_( @@ -131,6 +132,16 @@ 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) { @@ -1551,7 +1562,15 @@ if (VersionHasIetfQuicFrames(transport_version())) { return v99_streamid_manager_.MaybeIncreaseLargestPeerStreamId(stream_id); } - return stream_id_manager_.MaybeIncreaseLargestPeerStreamId(stream_id); + if (!stream_id_manager_.MaybeIncreaseLargestPeerStreamId(stream_id)) { + connection()->CloseConnection( + QUIC_TOO_MANY_AVAILABLE_STREAMS, + QuicStrCat(stream_id, " exceeds available streams ", + stream_id_manager_.MaxAvailableStreams()), + ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); + return false; + } + return true; } bool QuicSession::ShouldYield(QuicStreamId stream_id) {