Remove `is_static` argument from QuicWriteBlockedList::UnregisterStream(). This argument is unnecessary, because QuicWriteBlockedList can easily determine whether the stream has been registered as static or not. PiperOrigin-RevId: 503440382
diff --git a/quiche/quic/core/http/quic_server_session_base_test.cc b/quiche/quic/core/http/quic_server_session_base_test.cc index 133dcbf..807f05f 100644 --- a/quiche/quic/core/http/quic_server_session_base_test.cc +++ b/quiche/quic/core/http/quic_server_session_base_test.cc
@@ -525,8 +525,7 @@ if (!VersionUsesHttp3(transport_version())) { session_->UnregisterStreamPriority( - QuicUtils::GetHeadersStreamId(transport_version()), - /*is_static=*/true); + QuicUtils::GetHeadersStreamId(transport_version())); } QuicServerSessionBasePeer::SetCryptoStream(session_.get(), nullptr); MockQuicCryptoServerStream* quic_crypto_stream = nullptr;
diff --git a/quiche/quic/core/quic_session.cc b/quiche/quic/core/quic_session.cc index a58e572..4b37c76 100644 --- a/quiche/quic/core/quic_session.cc +++ b/quiche/quic/core/quic_session.cc
@@ -1816,8 +1816,8 @@ write_blocked_streams()->RegisterStream(id, is_static, priority); } -void QuicSession::UnregisterStreamPriority(QuicStreamId id, bool is_static) { - write_blocked_streams()->UnregisterStream(id, is_static); +void QuicSession::UnregisterStreamPriority(QuicStreamId id) { + write_blocked_streams()->UnregisterStream(id); } void QuicSession::UpdateStreamPriority(QuicStreamId id,
diff --git a/quiche/quic/core/quic_session.h b/quiche/quic/core/quic_session.h index 5c97f1a..f59d184 100644 --- a/quiche/quic/core/quic_session.h +++ b/quiche/quic/core/quic_session.h
@@ -334,7 +334,7 @@ void RegisterStreamPriority(QuicStreamId id, bool is_static, const QuicStreamPriority& priority) override; // Clears priority from the write blocked list. - void UnregisterStreamPriority(QuicStreamId id, bool is_static) override; + void UnregisterStreamPriority(QuicStreamId id) override; // Updates priority on the write blocked list. void UpdateStreamPriority(QuicStreamId id, const QuicStreamPriority& new_priority) override;
diff --git a/quiche/quic/core/quic_stream.cc b/quiche/quic/core/quic_stream.cc index 3fa0d01..6a436ff 100644 --- a/quiche/quic/core/quic_stream.cc +++ b/quiche/quic/core/quic_stream.cc
@@ -390,7 +390,7 @@ << ", fin_outstanding: " << fin_outstanding_; } if (stream_delegate_ != nullptr && type_ != CRYPTO) { - stream_delegate_->UnregisterStreamPriority(id(), is_static_); + stream_delegate_->UnregisterStreamPriority(id()); } }
diff --git a/quiche/quic/core/quic_write_blocked_list.cc b/quiche/quic/core/quic_write_blocked_list.cc index 2497907..f6d00f2 100644 --- a/quiche/quic/core/quic_write_blocked_list.cc +++ b/quiche/quic/core/quic_write_blocked_list.cc
@@ -68,10 +68,8 @@ priority_write_scheduler_.RegisterStream(stream_id, priority); } -void QuicWriteBlockedList::UnregisterStream(QuicStreamId stream_id, - bool is_static) { - if (is_static) { - static_stream_collection_.Unregister(stream_id); +void QuicWriteBlockedList::UnregisterStream(QuicStreamId stream_id) { + if (static_stream_collection_.Unregister(stream_id)) { return; } priority_write_scheduler_.UnregisterStream(stream_id); @@ -129,17 +127,17 @@ return false; } -void QuicWriteBlockedList::StaticStreamCollection::Unregister(QuicStreamId id) { +bool QuicWriteBlockedList::StaticStreamCollection::Unregister(QuicStreamId id) { for (auto it = streams_.begin(); it != streams_.end(); ++it) { if (it->id == id) { if (it->is_blocked) { --num_blocked_; } streams_.erase(it); - return; + return true; } } - QUICHE_DCHECK(false) << "Erasing a non-exist stream with id " << id; + return false; } bool QuicWriteBlockedList::StaticStreamCollection::SetBlocked(QuicStreamId id) {
diff --git a/quiche/quic/core/quic_write_blocked_list.h b/quiche/quic/core/quic_write_blocked_list.h index 0ad2c2d..b366eaf 100644 --- a/quiche/quic/core/quic_write_blocked_list.h +++ b/quiche/quic/core/quic_write_blocked_list.h
@@ -63,7 +63,9 @@ void RegisterStream(QuicStreamId stream_id, bool is_static_stream, const QuicStreamPriority& priority); - void UnregisterStream(QuicStreamId stream_id, bool is_static); + // Unregister a stream. `stream_id` must be registered, either as a static + // stream or as a non-static stream. + void UnregisterStream(QuicStreamId stream_id); // Updates the stored priority of a stream. Must not be called for static // streams. @@ -124,9 +126,9 @@ // True if |id| is in the collection, regardless of its state. bool IsRegistered(QuicStreamId id) const; - // Remove |id| from the collection, if it is in the blocked state, reduce - // |num_blocked_| by 1. - void Unregister(QuicStreamId id); + // Remove |id| from the collection. If it is in the blocked state, reduce + // |num_blocked_| by 1. Returns true if |id| was in the collection. + bool Unregister(QuicStreamId id); // Set |id| to be blocked. If |id| is not already blocked, increase // |num_blocked_| by 1.
diff --git a/quiche/quic/core/quic_write_blocked_list_test.cc b/quiche/quic/core/quic_write_blocked_list_test.cc index 1847993..e69e071 100644 --- a/quiche/quic/core/quic_write_blocked_list_test.cc +++ b/quiche/quic/core/quic_write_blocked_list_test.cc
@@ -6,6 +6,7 @@ #include "quiche/quic/platform/api/quic_test.h" #include "quiche/quic/test_tools/quic_test_utils.h" +#include "quiche/common/platform/api/quiche_expect_bug.h" using spdy::kV3HighestPriority; using spdy::kV3LowestPriority; @@ -53,8 +54,8 @@ write_blocked_list_.RegisterStream(stream_id, is_static_stream, priority); } - void UnregisterStream(QuicStreamId stream_id, bool is_static) { - write_blocked_list_.UnregisterStream(stream_id, is_static); + void UnregisterStream(QuicStreamId stream_id) { + write_blocked_list_.UnregisterStream(stream_id); } void UpdateStreamPriority(QuicStreamId stream_id, @@ -295,8 +296,8 @@ AddStream(1); AddStream(3); - UnregisterStream(23, kNotStatic); - UnregisterStream(1, kStatic); + UnregisterStream(23); + UnregisterStream(1); EXPECT_EQ(3u, PopFront()); EXPECT_EQ(17u, PopFront()); @@ -304,6 +305,14 @@ EXPECT_EQ(40, PopFront()); } +TEST_F(QuicWriteBlockedListTest, UnregisterNotRegisteredStream) { + EXPECT_QUICHE_BUG(UnregisterStream(1), "Stream 1 not registered"); + + RegisterStream(2, kNotStatic, {kV3HighestPriority, kIncremental}); + UnregisterStream(2); + EXPECT_QUICHE_BUG(UnregisterStream(2), "Stream 2 not registered"); +} + TEST_F(QuicWriteBlockedListTest, UpdateStreamPriority) { RegisterStream(40, kNotStatic, {kV3LowestPriority, kNotIncremental}); RegisterStream(23, kNotStatic, {6, kIncremental});
diff --git a/quiche/quic/core/stream_delegate_interface.h b/quiche/quic/core/stream_delegate_interface.h index b6db350..a5f03f0 100644 --- a/quiche/quic/core/stream_delegate_interface.h +++ b/quiche/quic/core/stream_delegate_interface.h
@@ -44,7 +44,7 @@ virtual void RegisterStreamPriority(QuicStreamId id, bool is_static, const QuicStreamPriority& priority) = 0; // Called on stream destruction to clear priority. - virtual void UnregisterStreamPriority(QuicStreamId id, bool is_static) = 0; + virtual void UnregisterStreamPriority(QuicStreamId id) = 0; // Called by the stream on SetPriority to update priority. virtual void UpdateStreamPriority(QuicStreamId id, const QuicStreamPriority& new_priority) = 0;
diff --git a/quiche/quic/tools/quic_simple_server_session_test.cc b/quiche/quic/tools/quic_simple_server_session_test.cc index d6eb67f..c30283c 100644 --- a/quiche/quic/tools/quic_simple_server_session_test.cc +++ b/quiche/quic/tools/quic_simple_server_session_test.cc
@@ -491,8 +491,7 @@ if (!VersionUsesHttp3(transport_version())) { session_->UnregisterStreamPriority( - QuicUtils::GetHeadersStreamId(transport_version()), - /*is_static=*/true); + QuicUtils::GetHeadersStreamId(transport_version())); } // Assume encryption already established. QuicSimpleServerSessionPeer::SetCryptoStream(session_.get(), nullptr);