Factor the QuicDispatcher's blocked writer list into a stand-alone QuicBlockedWriterList. PiperOrigin-RevId: 627519607
diff --git a/build/source_list.bzl b/build/source_list.bzl index 83c2a51..a28d283 100644 --- a/build/source_list.bzl +++ b/build/source_list.bzl
@@ -275,6 +275,7 @@ "quic/core/quic_arena_scoped_ptr.h", "quic/core/quic_bandwidth.h", "quic/core/quic_blocked_writer_interface.h", + "quic/core/quic_blocked_writer_list.h", "quic/core/quic_buffered_packet_store.h", "quic/core/quic_chaos_protector.h", "quic/core/quic_clock.h", @@ -606,6 +607,7 @@ "quic/core/quic_ack_listener_interface.cc", "quic/core/quic_alarm.cc", "quic/core/quic_bandwidth.cc", + "quic/core/quic_blocked_writer_list.cc", "quic/core/quic_buffered_packet_store.cc", "quic/core/quic_chaos_protector.cc", "quic/core/quic_coalesced_packet.cc", @@ -1239,6 +1241,7 @@ "quic/core/quic_alarm_test.cc", "quic/core/quic_arena_scoped_ptr_test.cc", "quic/core/quic_bandwidth_test.cc", + "quic/core/quic_blocked_writer_list_test.cc", "quic/core/quic_buffered_packet_store_test.cc", "quic/core/quic_chaos_protector_test.cc", "quic/core/quic_coalesced_packet_test.cc",
diff --git a/build/source_list.gni b/build/source_list.gni index 70587e1..5abdeab 100644 --- a/build/source_list.gni +++ b/build/source_list.gni
@@ -275,6 +275,7 @@ "src/quiche/quic/core/quic_arena_scoped_ptr.h", "src/quiche/quic/core/quic_bandwidth.h", "src/quiche/quic/core/quic_blocked_writer_interface.h", + "src/quiche/quic/core/quic_blocked_writer_list.h", "src/quiche/quic/core/quic_buffered_packet_store.h", "src/quiche/quic/core/quic_chaos_protector.h", "src/quiche/quic/core/quic_clock.h", @@ -606,6 +607,7 @@ "src/quiche/quic/core/quic_ack_listener_interface.cc", "src/quiche/quic/core/quic_alarm.cc", "src/quiche/quic/core/quic_bandwidth.cc", + "src/quiche/quic/core/quic_blocked_writer_list.cc", "src/quiche/quic/core/quic_buffered_packet_store.cc", "src/quiche/quic/core/quic_chaos_protector.cc", "src/quiche/quic/core/quic_coalesced_packet.cc", @@ -1240,6 +1242,7 @@ "src/quiche/quic/core/quic_alarm_test.cc", "src/quiche/quic/core/quic_arena_scoped_ptr_test.cc", "src/quiche/quic/core/quic_bandwidth_test.cc", + "src/quiche/quic/core/quic_blocked_writer_list_test.cc", "src/quiche/quic/core/quic_buffered_packet_store_test.cc", "src/quiche/quic/core/quic_chaos_protector_test.cc", "src/quiche/quic/core/quic_coalesced_packet_test.cc",
diff --git a/build/source_list.json b/build/source_list.json index 191c426..38d2b75 100644 --- a/build/source_list.json +++ b/build/source_list.json
@@ -274,6 +274,7 @@ "quiche/quic/core/quic_arena_scoped_ptr.h", "quiche/quic/core/quic_bandwidth.h", "quiche/quic/core/quic_blocked_writer_interface.h", + "quiche/quic/core/quic_blocked_writer_list.h", "quiche/quic/core/quic_buffered_packet_store.h", "quiche/quic/core/quic_chaos_protector.h", "quiche/quic/core/quic_clock.h", @@ -605,6 +606,7 @@ "quiche/quic/core/quic_ack_listener_interface.cc", "quiche/quic/core/quic_alarm.cc", "quiche/quic/core/quic_bandwidth.cc", + "quiche/quic/core/quic_blocked_writer_list.cc", "quiche/quic/core/quic_buffered_packet_store.cc", "quiche/quic/core/quic_chaos_protector.cc", "quiche/quic/core/quic_coalesced_packet.cc", @@ -1239,6 +1241,7 @@ "quiche/quic/core/quic_alarm_test.cc", "quiche/quic/core/quic_arena_scoped_ptr_test.cc", "quiche/quic/core/quic_bandwidth_test.cc", + "quiche/quic/core/quic_blocked_writer_list_test.cc", "quiche/quic/core/quic_buffered_packet_store_test.cc", "quiche/quic/core/quic_chaos_protector_test.cc", "quiche/quic/core/quic_coalesced_packet_test.cc",
diff --git a/quiche/quic/core/quic_blocked_writer_list.cc b/quiche/quic/core/quic_blocked_writer_list.cc new file mode 100644 index 0000000..105d8b9 --- /dev/null +++ b/quiche/quic/core/quic_blocked_writer_list.cc
@@ -0,0 +1,59 @@ +// Copyright (c) 2024 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "quiche/quic/core/quic_blocked_writer_list.h" + +#include "quiche/quic/platform/api/quic_bug_tracker.h" +#include "quiche/quic/platform/api/quic_flag_utils.h" + +namespace quic { + +void QuicBlockedWriterList::Add(QuicBlockedWriterInterface& blocked_writer) { + if (!blocked_writer.IsWriterBlocked()) { + // It is a programming error if this ever happens. When we are sure it is + // not happening, replace it with a QUICHE_DCHECK. + QUIC_BUG(quic_bug_12724_4) + << "Tried to add writer into blocked list when it shouldn't be added"; + // Return without adding the connection to the blocked list, to avoid + // infinite loops in OnCanWrite. + return; + } + + write_blocked_list_.insert(std::make_pair(&blocked_writer, true)); +} + +bool QuicBlockedWriterList::Empty() const { + return write_blocked_list_.empty(); +} + +bool QuicBlockedWriterList::Remove(QuicBlockedWriterInterface& blocked_writer) { + return write_blocked_list_.erase(&blocked_writer) != 0; +} + +void QuicBlockedWriterList::OnWriterUnblocked() { + // Move every blocked writer in |write_blocked_list_| to a temporary list. + const size_t num_blocked_writers_before = write_blocked_list_.size(); + WriteBlockedList temp_list; + temp_list.swap(write_blocked_list_); + QUICHE_DCHECK(write_blocked_list_.empty()); + + // Give each blocked writer a chance to write what they intended to write. + // If they are blocked again, they will call |OnWriteBlocked| to add + // themselves back into |write_blocked_list_|. + while (!temp_list.empty()) { + QuicBlockedWriterInterface* blocked_writer = temp_list.begin()->first; + temp_list.erase(temp_list.begin()); + blocked_writer->OnBlockedWriterCanWrite(); + } + const size_t num_blocked_writers_after = write_blocked_list_.size(); + if (num_blocked_writers_after != 0) { + if (num_blocked_writers_before == num_blocked_writers_after) { + QUIC_CODE_COUNT(quic_zero_progress_on_can_write); + } else { + QUIC_CODE_COUNT(quic_blocked_again_on_can_write); + } + } +} + +} // namespace quic
diff --git a/quiche/quic/core/quic_blocked_writer_list.h b/quiche/quic/core/quic_blocked_writer_list.h new file mode 100644 index 0000000..5cbcb67 --- /dev/null +++ b/quiche/quic/core/quic_blocked_writer_list.h
@@ -0,0 +1,41 @@ +// Copyright (c) 2024 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef QUICHE_QUIC_CORE_QUIC_BLOCKED_WRITER_LIST_H_ +#define QUICHE_QUIC_CORE_QUIC_BLOCKED_WRITER_LIST_H_ + +#include "quiche/quic/core/quic_blocked_writer_interface.h" +#include "quiche/common/quiche_linked_hash_map.h" + +namespace quic { + +// Maintains a list of blocked writers which can be resumed when unblocked. +class QUICHE_EXPORT QuicBlockedWriterList { + public: + // Adds `blocked_writer` (which must be write blocked) to the list. If + // `blocked_writer` is already in the list, this method has no effect. + void Add(QuicBlockedWriterInterface& blocked_writer); + + // Returns false if there are any blocked writers. + bool Empty() const; + + // Removes `blocked_writer` to the list. Returns true if `blocked_writer` + // was in the list and false otherwise. + bool Remove(QuicBlockedWriterInterface& blocked_writer); + + // Calls `OnCanWrite()` on all the writers in the list. + void OnWriterUnblocked(); + + private: + // Ideally we'd have a linked_hash_set: the boolean is unused. + using WriteBlockedList = + quiche::QuicheLinkedHashMap<QuicBlockedWriterInterface*, bool>; + + // The list of writers waiting to write. + WriteBlockedList write_blocked_list_; +}; + +} // namespace quic + +#endif // QUICHE_QUIC_CORE_QUIC_BLOCKED_WRITER_LIST_H_
diff --git a/quiche/quic/core/quic_blocked_writer_list_test.cc b/quiche/quic/core/quic_blocked_writer_list_test.cc new file mode 100644 index 0000000..5e5531e --- /dev/null +++ b/quiche/quic/core/quic_blocked_writer_list_test.cc
@@ -0,0 +1,127 @@ +// Copyright (c) 2024 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "quiche/quic/core/quic_blocked_writer_list.h" + +#include "quiche/quic/core/quic_blocked_writer_interface.h" +#include "quiche/quic/platform/api/quic_test.h" + +namespace quic { + +using testing::Invoke; +using testing::Return; + +namespace { +class TestWriter : public QuicBlockedWriterInterface { + public: + ~TestWriter() override = default; + + MOCK_METHOD(void, OnBlockedWriterCanWrite, ()); + MOCK_METHOD(bool, IsWriterBlocked, (), (const)); +}; +} // namespace + +TEST(QuicBlockedWriterList, Empty) { + QuicBlockedWriterList list; + EXPECT_TRUE(list.Empty()); +} + +TEST(QuicBlockedWriterList, NotEmpty) { + QuicBlockedWriterList list; + testing::StrictMock<TestWriter> writer1; + EXPECT_CALL(writer1, IsWriterBlocked()).WillOnce(Return(true)); + list.Add(writer1); + EXPECT_FALSE(list.Empty()); + list.Remove(writer1); + EXPECT_TRUE(list.Empty()); +} + +TEST(QuicBlockedWriterList, OnWriterUnblocked) { + QuicBlockedWriterList list; + testing::StrictMock<TestWriter> writer1; + + EXPECT_CALL(writer1, IsWriterBlocked()).WillOnce(Return(true)); + list.Add(writer1); + EXPECT_CALL(writer1, OnBlockedWriterCanWrite()); + list.OnWriterUnblocked(); + EXPECT_TRUE(list.Empty()); +} + +TEST(QuicBlockedWriterList, OnWriterUnblockedInOrder) { + QuicBlockedWriterList list; + testing::StrictMock<TestWriter> writer1; + testing::StrictMock<TestWriter> writer2; + testing::StrictMock<TestWriter> writer3; + + EXPECT_CALL(writer1, IsWriterBlocked()).WillOnce(Return(true)); + EXPECT_CALL(writer2, IsWriterBlocked()).WillOnce(Return(true)); + EXPECT_CALL(writer3, IsWriterBlocked()).WillOnce(Return(true)); + + list.Add(writer1); + list.Add(writer2); + list.Add(writer3); + + testing::InSequence s; + EXPECT_CALL(writer1, OnBlockedWriterCanWrite()); + EXPECT_CALL(writer2, OnBlockedWriterCanWrite()); + EXPECT_CALL(writer3, OnBlockedWriterCanWrite()); + list.OnWriterUnblocked(); + EXPECT_TRUE(list.Empty()); +} + +TEST(QuicBlockedWriterList, OnWriterUnblockedInOrderAfterReinsertion) { + QuicBlockedWriterList list; + testing::StrictMock<TestWriter> writer1; + testing::StrictMock<TestWriter> writer2; + testing::StrictMock<TestWriter> writer3; + + EXPECT_CALL(writer1, IsWriterBlocked()).WillOnce(Return(true)); + EXPECT_CALL(writer2, IsWriterBlocked()).WillOnce(Return(true)); + EXPECT_CALL(writer3, IsWriterBlocked()).WillOnce(Return(true)); + + list.Add(writer1); + list.Add(writer2); + list.Add(writer3); + + EXPECT_CALL(writer1, IsWriterBlocked()).WillOnce(Return(true)); + list.Add(writer1); + + testing::InSequence s; + EXPECT_CALL(writer1, OnBlockedWriterCanWrite()); + EXPECT_CALL(writer2, OnBlockedWriterCanWrite()); + EXPECT_CALL(writer3, OnBlockedWriterCanWrite()); + list.OnWriterUnblocked(); + EXPECT_TRUE(list.Empty()); +} + +TEST(QuicBlockedWriterList, OnWriterUnblockedThenBlocked) { + QuicBlockedWriterList list; + testing::StrictMock<TestWriter> writer1; + testing::StrictMock<TestWriter> writer2; + testing::StrictMock<TestWriter> writer3; + + EXPECT_CALL(writer1, IsWriterBlocked()).WillOnce(Return(true)); + EXPECT_CALL(writer2, IsWriterBlocked()).WillOnce(Return(true)); + EXPECT_CALL(writer3, IsWriterBlocked()).WillOnce(Return(true)); + + list.Add(writer1); + list.Add(writer2); + list.Add(writer3); + + EXPECT_CALL(writer1, OnBlockedWriterCanWrite()); + EXPECT_CALL(writer2, IsWriterBlocked()).WillOnce(Return(true)); + EXPECT_CALL(writer2, OnBlockedWriterCanWrite()).WillOnce(Invoke([&]() { + list.Add(writer2); + })); + + EXPECT_CALL(writer3, OnBlockedWriterCanWrite()); + list.OnWriterUnblocked(); + EXPECT_FALSE(list.Empty()); + + EXPECT_CALL(writer2, OnBlockedWriterCanWrite()); + list.OnWriterUnblocked(); + EXPECT_TRUE(list.Empty()); +} + +} // namespace quic
diff --git a/quiche/quic/core/quic_dispatcher.cc b/quiche/quic/core/quic_dispatcher.cc index 1d371e4..02c69ce 100644 --- a/quiche/quic/core/quic_dispatcher.cc +++ b/quiche/quic/core/quic_dispatcher.cc
@@ -794,7 +794,7 @@ QuicErrorCode /*error*/, const std::string& /*error_details*/, ConnectionCloseSource /*source*/) { - write_blocked_list_.erase(connection); + write_blocked_list_.Remove(*connection); QuicTimeWaitListManager::TimeWaitAction action = QuicTimeWaitListManager::SEND_STATELESS_RESET; if (connection->termination_packets() != nullptr && @@ -872,9 +872,9 @@ } void QuicDispatcher::DeleteSessions() { - if (!write_blocked_list_.empty()) { + if (!write_blocked_list_.Empty()) { for (const auto& session : closed_session_list_) { - if (write_blocked_list_.erase(session->connection()) != 0) { + if (write_blocked_list_.Remove(*session->connection())) { QUIC_BUG(quic_bug_12724_2) << "QuicConnection was in WriteBlockedList before destruction " << session->connection()->connection_id(); @@ -892,32 +892,11 @@ // The socket is now writable. writer_->SetWritable(); - // Move every blocked writer in |write_blocked_list_| to a temporary list. - const size_t num_blocked_writers_before = write_blocked_list_.size(); - WriteBlockedList temp_list; - temp_list.swap(write_blocked_list_); - QUICHE_DCHECK(write_blocked_list_.empty()); - - // Give each blocked writer a chance to write what they intended to write. - // If they are blocked again, they will call |OnWriteBlocked| to add - // themselves back into |write_blocked_list_|. - while (!temp_list.empty()) { - QuicBlockedWriterInterface* blocked_writer = temp_list.begin()->first; - temp_list.erase(temp_list.begin()); - blocked_writer->OnBlockedWriterCanWrite(); - } - const size_t num_blocked_writers_after = write_blocked_list_.size(); - if (num_blocked_writers_after != 0) { - if (num_blocked_writers_before == num_blocked_writers_after) { - QUIC_CODE_COUNT(quic_zero_progress_on_can_write); - } else { - QUIC_CODE_COUNT(quic_blocked_again_on_can_write); - } - } + write_blocked_list_.OnWriterUnblocked(); } bool QuicDispatcher::HasPendingWrites() const { - return !write_blocked_list_.empty(); + return !write_blocked_list_.Empty(); } void QuicDispatcher::Shutdown() { @@ -999,17 +978,7 @@ void QuicDispatcher::OnWriteBlocked( QuicBlockedWriterInterface* blocked_writer) { - if (!blocked_writer->IsWriterBlocked()) { - // It is a programming error if this ever happens. When we are sure it is - // not happening, replace it with a QUICHE_DCHECK. - QUIC_BUG(quic_bug_12724_4) - << "Tried to add writer into blocked list when it shouldn't be added"; - // Return without adding the connection to the blocked list, to avoid - // infinite loops in OnCanWrite. - return; - } - - write_blocked_list_.insert(std::make_pair(blocked_writer, true)); + write_blocked_list_.Add(*blocked_writer); } void QuicDispatcher::OnRstStreamReceived(const QuicRstStreamFrame& /*frame*/) {}
diff --git a/quiche/quic/core/quic_dispatcher.h b/quiche/quic/core/quic_dispatcher.h index d1d0d6f..efe5a2c 100644 --- a/quiche/quic/core/quic_dispatcher.h +++ b/quiche/quic/core/quic_dispatcher.h
@@ -26,6 +26,7 @@ #include "quiche/quic/core/quic_alarm.h" #include "quiche/quic/core/quic_alarm_factory.h" #include "quiche/quic/core/quic_blocked_writer_interface.h" +#include "quiche/quic/core/quic_blocked_writer_list.h" #include "quiche/quic/core/quic_buffered_packet_store.h" #include "quiche/quic/core/quic_connection.h" #include "quiche/quic/core/quic_connection_id.h" @@ -58,10 +59,6 @@ public ProcessPacketInterface, public QuicBufferedPacketStore::VisitorInterface { public: - // Ideally we'd have a linked_hash_set: the boolean is unused. - using WriteBlockedList = - quiche::QuicheLinkedHashMap<QuicBlockedWriterInterface*, bool>; - QuicDispatcher( const QuicConfig* config, const QuicCryptoServerConfig* crypto_config, QuicVersionManager* version_manager, @@ -419,7 +416,7 @@ QuicCompressedCertsCache compressed_certs_cache_; // The list of connections waiting to write. - WriteBlockedList write_blocked_list_; + QuicBlockedWriterList write_blocked_list_; ReferenceCountedSessionMap reference_counted_session_map_;
diff --git a/quiche/quic/core/quic_dispatcher_test.cc b/quiche/quic/core/quic_dispatcher_test.cc index 406bbc8..05e6e69 100644 --- a/quiche/quic/core/quic_dispatcher_test.cc +++ b/quiche/quic/core/quic_dispatcher_test.cc
@@ -2061,7 +2061,7 @@ MockQuicConnectionHelper helper_; MockAlarmFactory alarm_factory_; BlockingWriter* writer_; - QuicDispatcher::WriteBlockedList* blocked_list_; + QuicBlockedWriterList* blocked_list_; }; INSTANTIATE_TEST_SUITE_P(QuicDispatcherWriteBlockedListTests, @@ -2108,7 +2108,7 @@ // Add and remove one connction. SetBlocked(); dispatcher_->OnWriteBlocked(connection1()); - blocked_list_->erase(connection1()); + blocked_list_->Remove(*connection1()); EXPECT_CALL(*connection1(), OnCanWrite()).Times(0); dispatcher_->OnCanWrite(); @@ -2116,14 +2116,14 @@ SetBlocked(); dispatcher_->OnWriteBlocked(connection1()); dispatcher_->OnWriteBlocked(connection2()); - blocked_list_->erase(connection1()); + blocked_list_->Remove(*connection1()); EXPECT_CALL(*connection2(), OnCanWrite()); dispatcher_->OnCanWrite(); // Add it, remove it, and add it back and make sure things are OK. SetBlocked(); dispatcher_->OnWriteBlocked(connection1()); - blocked_list_->erase(connection1()); + blocked_list_->Remove(*connection1()); dispatcher_->OnWriteBlocked(connection1()); EXPECT_CALL(*connection1(), OnCanWrite()).Times(1); dispatcher_->OnCanWrite(); @@ -2134,7 +2134,7 @@ SetBlocked(); dispatcher_->OnWriteBlocked(connection1()); dispatcher_->OnWriteBlocked(connection1()); - blocked_list_->erase(connection1()); + blocked_list_->Remove(*connection1()); EXPECT_CALL(*connection1(), OnCanWrite()).Times(0); dispatcher_->OnCanWrite();
diff --git a/quiche/quic/test_tools/quic_dispatcher_peer.cc b/quiche/quic/test_tools/quic_dispatcher_peer.cc index 8a803c5..f08c2d7 100644 --- a/quiche/quic/test_tools/quic_dispatcher_peer.cc +++ b/quiche/quic/test_tools/quic_dispatcher_peer.cc
@@ -54,7 +54,7 @@ } // static -QuicDispatcher::WriteBlockedList* QuicDispatcherPeer::GetWriteBlockedList( +QuicBlockedWriterList* QuicDispatcherPeer::GetWriteBlockedList( QuicDispatcher* dispatcher) { return &dispatcher->write_blocked_list_; }
diff --git a/quiche/quic/test_tools/quic_dispatcher_peer.h b/quiche/quic/test_tools/quic_dispatcher_peer.h index 1238a89..985dc67 100644 --- a/quiche/quic/test_tools/quic_dispatcher_peer.h +++ b/quiche/quic/test_tools/quic_dispatcher_peer.h
@@ -37,8 +37,7 @@ static QuicAlarmFactory* GetAlarmFactory(QuicDispatcher* dispatcher); - static QuicDispatcher::WriteBlockedList* GetWriteBlockedList( - QuicDispatcher* dispatcher); + static QuicBlockedWriterList* GetWriteBlockedList(QuicDispatcher* dispatcher); // Get the dispatcher's record of the last error reported to its framer // visitor's OnError() method. Then set that record to QUIC_NO_ERROR.