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.
