diff --git a/src/core/lib/gprpp/ref_counted.h b/src/core/lib/gprpp/ref_counted.h index 44070e58bf1..b20136bfca2 100644 --- a/src/core/lib/gprpp/ref_counted.h +++ b/src/core/lib/gprpp/ref_counted.h @@ -37,20 +37,6 @@ namespace grpc_core { -// PolymorphicRefCount enforces polymorphic destruction of RefCounted. -class PolymorphicRefCount { - public: - virtual ~PolymorphicRefCount() = default; -}; - -// NonPolymorphicRefCount does not enforce polymorphic destruction of -// RefCounted. Please refer to grpc_core::RefCounted for more details, and -// when in doubt use PolymorphicRefCount. -class NonPolymorphicRefCount { - public: - ~NonPolymorphicRefCount() = default; -}; - // RefCount is a simple atomic ref-count. // // This is a C++ implementation of gpr_refcount, with inline functions. Due to @@ -218,9 +204,45 @@ class RefCount { Atomic value_; }; +// PolymorphicRefCount enforces polymorphic destruction of RefCounted. +class PolymorphicRefCount { + public: + virtual ~PolymorphicRefCount() = default; +}; + +// NonPolymorphicRefCount does not enforce polymorphic destruction of +// RefCounted. Please refer to grpc_core::RefCounted for more details, and +// when in doubt use PolymorphicRefCount. +class NonPolymorphicRefCount { + public: + ~NonPolymorphicRefCount() = default; +}; + +namespace internal { +template +class Delete; +template +class Delete { + public: + Delete(T* t) { delete t; } +}; +template +class Delete { + public: + Delete(T* t) {} +}; +} // namespace internal + // A base class for reference-counted objects. -// New objects should be created via New() and start with a refcount of 1. -// When the refcount reaches 0, the object will be deleted via delete . +// New objects should be created via new and start with a refcount of 1. +// When the refcount reaches 0, the object will be deleted via delete. +// +// If DeleteUponUnref is false, deletion will not occur when the ref +// count reaches 0. This is useful in cases where all existing objects +// 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. // // This will commonly be used by CRTP (curiously-recurring template pattern) // e.g., class MyClass : public RefCounted @@ -244,7 +266,8 @@ class RefCount { // Child* ch; // ch->Unref(); // -template +template class RefCounted : public Impl { public: // Note: Depending on the Impl used, this dtor can be implicitly virtual. @@ -267,12 +290,12 @@ class RefCounted : public Impl { // friend of this class. void Unref() { if (GPR_UNLIKELY(refs_.Unref())) { - delete static_cast(this); + internal::Delete(static_cast(this)); } } void Unref(const DebugLocation& location, const char* reason) { if (GPR_UNLIKELY(refs_.Unref(location, reason))) { - delete static_cast(this); + internal::Delete(static_cast(this)); } } diff --git a/test/core/gprpp/ref_counted_test.cc b/test/core/gprpp/ref_counted_test.cc index 3205d7c6725..365126942dc 100644 --- a/test/core/gprpp/ref_counted_test.cc +++ b/test/core/gprpp/ref_counted_test.cc @@ -18,6 +18,9 @@ #include "src/core/lib/gprpp/ref_counted.h" +#include + +#include #include #include "src/core/lib/gprpp/memory.h" @@ -48,6 +51,59 @@ TEST(RefCounted, ExtraRef) { foo->Unref(); } +class Value : public RefCounted { + public: + Value(int value, std::set* registry) : value_(value) { + registry->insert(this); + } + + int value() const { return value_; } + + private: + int value_; +}; + +void GarbageCollectRegistry(std::set* registry) { + for (auto it = registry->begin(); it != registry->end();) { + Value* v = *it; + // Check if the object has any refs remaining. + if (v->RefIfNonZero()) { + // 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; + it = registry->erase(it); + } + } +} + +TEST(RefCounted, NoDeleteUponUnref) { + 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))); + // 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))); + // Unref v2 and run GC to remove it. + v2.reset(); + GarbageCollectRegistry(®istry); + EXPECT_THAT(registry, ::testing::UnorderedElementsAre( + ::testing::Property(&Value::value, 1))); + // Now unref v1 and run GC again. + v1.reset(); + GarbageCollectRegistry(®istry); + EXPECT_THAT(registry, ::testing::UnorderedElementsAre()); +} + class FooNonPolymorphic : public RefCounted { public: