From 0db7ea4196ef4cb6926c6908a6fe6ae7eed0f0cc Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Thu, 24 Sep 2020 00:21:47 -0700 Subject: [PATCH] Change RefIfNonZero() to return a RefCountedPtr<>. --- .../resolver/xds/xds_resolver.cc | 4 +-- src/core/lib/channel/channelz_registry.cc | 23 +++++++------ src/core/lib/gprpp/ref_counted.h | 14 +++++--- test/core/gprpp/ref_counted_test.cc | 34 +++++++++---------- 4 files changed, 41 insertions(+), 34 deletions(-) diff --git a/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc b/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc index c4cdde64d36..8df5b0caad2 100644 --- a/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc +++ b/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc @@ -674,8 +674,8 @@ void XdsResolver::GenerateResult() { void XdsResolver::MaybeRemoveUnusedClusters() { bool update_needed = false; for (auto it = cluster_state_map_.begin(); it != cluster_state_map_.end();) { - if (it->second->RefIfNonZero()) { - it->second->Unref(); + RefCountedPtr cluster_state = it->second->RefIfNonZero(); + if (cluster_state != nullptr) { ++it; } else { update_needed = true; diff --git a/src/core/lib/channel/channelz_registry.cc b/src/core/lib/channel/channelz_registry.cc index 2739f5f53e5..6eea6eb5824 100644 --- a/src/core/lib/channel/channelz_registry.cc +++ b/src/core/lib/channel/channelz_registry.cc @@ -78,8 +78,7 @@ RefCountedPtr ChannelzRegistry::InternalGet(intptr_t uuid) { // Found node. Return only if its refcount is not zero (i.e., when we // know that there is no other thread about to destroy it). BaseNode* node = it->second; - if (!node->RefIfNonZero()) return nullptr; - return RefCountedPtr(node); + return node->RefIfNonZero(); } std::string ChannelzRegistry::InternalGetTopChannels( @@ -91,8 +90,9 @@ std::string ChannelzRegistry::InternalGetTopChannels( for (auto it = node_map_.lower_bound(start_channel_id); it != node_map_.end(); ++it) { BaseNode* node = it->second; + RefCountedPtr node_ref; if (node->type() == BaseNode::EntityType::kTopLevelChannel && - node->RefIfNonZero()) { + (node_ref = node->RefIfNonZero()) != nullptr) { // Check if we are over pagination limit to determine if we need to set // the "end" element. If we don't go through this block, we know that // when the loop terminates, we have <= to kPaginationLimit. @@ -100,10 +100,10 @@ std::string ChannelzRegistry::InternalGetTopChannels( // refcount, we need to decrease it, but we can't unref while // holding the lock, because this may lead to a deadlock. if (top_level_channels.size() == kPaginationLimit) { - node_after_pagination_limit.reset(node); + node_after_pagination_limit = std::move(node_ref); break; } - top_level_channels.emplace_back(node); + top_level_channels.emplace_back(std::move(node_ref)); } } } @@ -129,8 +129,9 @@ std::string ChannelzRegistry::InternalGetServers(intptr_t start_server_id) { for (auto it = node_map_.lower_bound(start_server_id); it != node_map_.end(); ++it) { BaseNode* node = it->second; + RefCountedPtr node_ref; if (node->type() == BaseNode::EntityType::kServer && - node->RefIfNonZero()) { + (node_ref = node->RefIfNonZero()) != nullptr) { // Check if we are over pagination limit to determine if we need to set // the "end" element. If we don't go through this block, we know that // when the loop terminates, we have <= to kPaginationLimit. @@ -138,10 +139,10 @@ std::string ChannelzRegistry::InternalGetServers(intptr_t start_server_id) { // refcount, we need to decrease it, but we can't unref while // holding the lock, because this may lead to a deadlock. if (servers.size() == kPaginationLimit) { - node_after_pagination_limit.reset(node); + node_after_pagination_limit = std::move(node_ref); break; } - servers.emplace_back(node); + servers.emplace_back(std::move(node_ref)); } } } @@ -164,9 +165,9 @@ void ChannelzRegistry::InternalLogAllEntities() { { MutexLock lock(&mu_); for (auto& p : node_map_) { - BaseNode* node = p.second; - if (node->RefIfNonZero()) { - nodes.emplace_back(node); + RefCountedPtr node = p.second->RefIfNonZero(); + if (node != nullptr) { + nodes.emplace_back(std::move(node)); } } } diff --git a/src/core/lib/gprpp/ref_counted.h b/src/core/lib/gprpp/ref_counted.h index b20136bfca2..b69ba312312 100644 --- a/src/core/lib/gprpp/ref_counted.h +++ b/src/core/lib/gprpp/ref_counted.h @@ -242,7 +242,7 @@ class Delete { // must be tracked in a registry but the object's entry in the registry // cannot be removed from the object's dtor due to synchronization issues. // In this case, the registry can be cleaned up later by identifying -// entries for which RefIfNonZero() returns false. +// entries for which RefIfNonZero() returns null. // // This will commonly be used by CRTP (curiously-recurring template pattern) // e.g., class MyClass : public RefCounted @@ -299,9 +299,15 @@ class RefCounted : public Impl { } } - bool RefIfNonZero() { return refs_.RefIfNonZero(); } - bool RefIfNonZero(const DebugLocation& location, const char* reason) { - return refs_.RefIfNonZero(location, reason); + RefCountedPtr RefIfNonZero() GRPC_MUST_USE_RESULT { + return RefCountedPtr(refs_.RefIfNonZero() ? static_cast(this) + : nullptr); + } + RefCountedPtr RefIfNonZero(const DebugLocation& location, + const char* reason) GRPC_MUST_USE_RESULT { + return RefCountedPtr(refs_.RefIfNonZero(location, reason) + ? static_cast(this) + : nullptr); } // Not copyable nor movable. diff --git a/test/core/gprpp/ref_counted_test.cc b/test/core/gprpp/ref_counted_test.cc index 365126942dc..c978f020295 100644 --- a/test/core/gprpp/ref_counted_test.cc +++ b/test/core/gprpp/ref_counted_test.cc @@ -53,8 +53,8 @@ TEST(RefCounted, ExtraRef) { class Value : public RefCounted { public: - Value(int value, std::set* registry) : value_(value) { - registry->insert(this); + Value(int value, std::set>* registry) : value_(value) { + registry->emplace(this); } int value() const { return value_; } @@ -63,41 +63,41 @@ class Value : public RefCounted { int value_; }; -void GarbageCollectRegistry(std::set* registry) { +void GarbageCollectRegistry(std::set>* registry) { for (auto it = registry->begin(); it != registry->end();) { - Value* v = *it; + RefCountedPtr v = (*it)->RefIfNonZero(); // Check if the object has any refs remaining. - if (v->RefIfNonZero()) { + if (v != nullptr) { // It has refs remaining, so we do not delete it. - v->Unref(); // Remove the ref we just added. ++it; } else { - // No refs remaining, so delete it and remove from registry. - delete v; + // No refs remaining, so remove it from the registry. it = registry->erase(it); } } } TEST(RefCounted, NoDeleteUponUnref) { - std::set registry; + std::set> registry; // Add two objects to the registry. auto v1 = MakeRefCounted(1, ®istry); auto v2 = MakeRefCounted(2, ®istry); - EXPECT_THAT(registry, ::testing::UnorderedElementsAre( - ::testing::Property(&Value::value, 1), - ::testing::Property(&Value::value, 2))); + EXPECT_THAT(registry, + ::testing::UnorderedElementsAre( + ::testing::Pointee(::testing::Property(&Value::value, 1)), + ::testing::Pointee(::testing::Property(&Value::value, 2)))); // Running garbage collection should not delete anything, since both // entries still have refs. GarbageCollectRegistry(®istry); - EXPECT_THAT(registry, ::testing::UnorderedElementsAre( - ::testing::Property(&Value::value, 1), - ::testing::Property(&Value::value, 2))); + EXPECT_THAT(registry, + ::testing::UnorderedElementsAre( + ::testing::Pointee(::testing::Property(&Value::value, 1)), + ::testing::Pointee(::testing::Property(&Value::value, 2)))); // Unref v2 and run GC to remove it. v2.reset(); GarbageCollectRegistry(®istry); - EXPECT_THAT(registry, ::testing::UnorderedElementsAre( - ::testing::Property(&Value::value, 1))); + EXPECT_THAT(registry, ::testing::UnorderedElementsAre(::testing::Pointee( + ::testing::Property(&Value::value, 1)))); // Now unref v1 and run GC again. v1.reset(); GarbageCollectRegistry(®istry);