HttpStreamPool: Destroy Group when closing the last idle stream
Group::CloseOneIdleStream() could close a stream that is the last
active stream in the group. We should destroy the group when it's the
case. Otherwise, the group will be alive forever.
Bug: 346835898
Change-Id: I79656f5f078fac93223f433e2f51310a1b6674dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6056788
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Nidhi Jaju <nidhijaju@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1389672}
diff --git a/net/http/http_stream_pool_group.cc b/net/http/http_stream_pool_group.cc
index e50057dc..ab18b75 100644
--- a/net/http/http_stream_pool_group.cc
+++ b/net/http/http_stream_pool_group.cc
@@ -251,8 +251,17 @@
return false;
}
+ RecordNetLogClosingSocket(*idle_stream_sockets_.front().stream_socket,
+ kExceededSocketLimits);
idle_stream_sockets_.pop_front();
pool_->DecrementTotalIdleStreamCount();
+ if (CanComplete()) {
+ // Use PostTask since MaybeComplete() may delete `this`, and this method
+ // could be called while iterating all groups.
+ base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
+ FROM_HERE,
+ base::BindOnce(&Group::MaybeComplete, weak_ptr_factory_.GetWeakPtr()));
+ }
return true;
}
@@ -304,11 +313,13 @@
void HttpStreamPool::Group::CloseIdleStreams(
std::string_view net_log_close_reason_utf8) {
CleanupIdleStreamSockets(CleanupMode::kForce, net_log_close_reason_utf8);
- // Use PostTask since MaybeComplete() may delete `this`, and this method could
- // be called while iterating all groups.
- base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
- FROM_HERE,
- base::BindOnce(&Group::MaybeComplete, weak_ptr_factory_.GetWeakPtr()));
+ if (CanComplete()) {
+ // Use PostTask since MaybeComplete() may delete `this`, and this method
+ // could be called while iterating all groups.
+ base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
+ FROM_HERE,
+ base::BindOnce(&Group::MaybeComplete, weak_ptr_factory_.GetWeakPtr()));
+ }
}
void HttpStreamPool::Group::CancelJobs(int error) {
@@ -374,8 +385,12 @@
std::make_unique<AttemptManager>(this, http_network_session()->net_log());
}
+bool HttpStreamPool::Group::CanComplete() const {
+ return ActiveStreamSocketCount() == 0 && !attempt_manager_;
+}
+
void HttpStreamPool::Group::MaybeComplete() {
- if (ActiveStreamSocketCount() > 0 || attempt_manager_) {
+ if (!CanComplete()) {
return;
}
diff --git a/net/http/http_stream_pool_group.h b/net/http/http_stream_pool_group.h
index 02371b0..db90cd7 100644
--- a/net/http/http_stream_pool_group.h
+++ b/net/http/http_stream_pool_group.h
@@ -126,7 +126,8 @@
// Tries to process a pending request.
void ProcessPendingRequest();
- // Closes one idle stream socket. Returns true if it closed a stream.
+ // Closes one idle stream socket. Returns true if it closed a stream. Called
+ // when the pool reached the stream count limit.
bool CloseOneIdleStreamSocket();
// Returns the number of handed out streams.
@@ -204,6 +205,9 @@
void EnsureAttemptManager();
+ // Returns true when `this` can be deleted.
+ bool CanComplete() const;
+
void MaybeComplete();
const raw_ptr<HttpStreamPool> pool_;
diff --git a/net/http/http_stream_pool_group_unittest.cc b/net/http/http_stream_pool_group_unittest.cc
index 6dca845..622d1d9 100644
--- a/net/http/http_stream_pool_group_unittest.cc
+++ b/net/http/http_stream_pool_group_unittest.cc
@@ -298,6 +298,20 @@
ASSERT_EQ(group.IdleStreamSocketCount(), 0u);
}
+// Test that a group is destroyed when closing an idle stream that is the last
+// stream in the group.
+TEST_F(HttpStreamPoolGroupTest, DestroyGroupAfterCloseOneIdleStream) {
+ Group& group = GetOrCreateTestGroup();
+
+ auto stream_socket = std::make_unique<FakeStreamSocket>();
+ group.AddIdleStreamSocket(std::move(stream_socket));
+ ASSERT_EQ(group.IdleStreamSocketCount(), 1u);
+
+ ASSERT_TRUE(group.CloseOneIdleStreamSocket());
+ FastForwardUntilNoTasksRemain();
+ ASSERT_FALSE(GetTestGroup());
+}
+
TEST_F(HttpStreamPoolGroupTest, IPAddressChangeCleanupIdleSocket) {
auto stream_socket = std::make_unique<FakeStreamSocket>();