From 7408828c155a653769a51fd07e51be6e1057a12a Mon Sep 17 00:00:00 2001 From: Zang MingJie Date: Thu, 9 Jun 2022 13:30:32 +0800 Subject: [PATCH] IntrusiveList: Add auto-unlink mode (#19305) --- src/lib/address_resolve/AddressResolve.h | 2 +- src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp | 2 +- src/lib/support/IntrusiveList.h | 157 ++++++++++++++----- src/lib/support/tests/TestIntrusiveList.cpp | 32 +++- src/transport/SessionHolder.h | 2 +- 5 files changed, 154 insertions(+), 41 deletions(-) diff --git a/src/lib/address_resolve/AddressResolve.h b/src/lib/address_resolve/AddressResolve.h index 754abbd37d21ec..789fcddce7f104 100644 --- a/src/lib/address_resolve/AddressResolve.h +++ b/src/lib/address_resolve/AddressResolve.h @@ -66,7 +66,7 @@ class NodeListener /// Implementations extend this class with implementation-specific data like /// storing the 'last known good address' and 'scores' or any additional data /// required to figure out when a resolve is ok. -class NodeLookupHandleBase : public IntrusiveListNodeBase +class NodeLookupHandleBase : public IntrusiveListNodeBase<> { public: NodeLookupHandleBase() {} diff --git a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp index d8c13907fc4598..a90a31a9f6a24e 100644 --- a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp +++ b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp @@ -105,7 +105,7 @@ constexpr size_t kMaxOperationalRecords = 6; /// Represents an allocated operational responder. /// /// Wraps a QueryResponderAllocator. -class OperationalQueryAllocator : public chip::IntrusiveListNodeBase +class OperationalQueryAllocator : public chip::IntrusiveListNodeBase<> { public: using Allocator = QueryResponderAllocator; diff --git a/src/lib/support/IntrusiveList.h b/src/lib/support/IntrusiveList.h index 56330ece6ad874..158c332b6965de 100644 --- a/src/lib/support/IntrusiveList.h +++ b/src/lib/support/IntrusiveList.h @@ -25,26 +25,84 @@ namespace chip { class IntrusiveListBase; -class IntrusiveListNodeBase +enum class IntrusiveMode +{ + // Strict mode features + // + // Node constructor puts the hook in a well-known default state. + // + // Node destructor checks if the hook is in the well-known default state. + // If not, an assertion is raised. + // + // Every time an object is inserted in the intrusive container, the + // container checks if the hook is in the well-known default state. If + // not, an assertion is raised. + // + // Every time an object is being erased from the intrusive container, the + // container puts the erased object in the well-known default state. + // + // Note: The strict mode works similar as safe mode for Boost.Intrusive + // + Strict, + + // Auto-unlink mode features + // + // When the destructor of the node is called, the hook checks if the node + // is inserted in a container. If so, the hook removes the node from the + // container. + // + // The hook has a member function called unlink() that can be used to + // unlink the node from the container at any time, without having any + // reference to the container, if the user wants to do so. + // + // This auto-unlink feature is useful in certain applications but it must + // be used very carefully: + // + // If several threads are using the same container the destructor of the + // auto-unlink hook will be called without any thread synchronization so + // removing the object is thread-unsafe. Container contents change + // silently without modifying the container directly. This can lead to + // surprising effects. + // + // These auto-unlink hooks have also strict-mode properties: + // + // Node constructors put the hook in a well-known default state. + // + // Every time an object is inserted in the intrusive container, the + // container checks if the hook is in the well-known default state. If + // not, an assertion is raised. + // + // Every time an object is erased from an intrusive container, the + // container puts the erased object in the well-known default state. + // + // Note: The strict mode works similar as auto-unlink mode for + // Boost.Intrusive + // + AutoUnlink, +}; + +class IntrusiveListNodePrivateBase { public: - IntrusiveListNodeBase() : mPrev(nullptr), mNext(nullptr) {} - ~IntrusiveListNodeBase() { VerifyOrDie(!IsInList()); } + IntrusiveListNodePrivateBase() : mPrev(nullptr), mNext(nullptr) {} + ~IntrusiveListNodePrivateBase() { VerifyOrDie(!IsInList()); } // Note: The copy construct/assignment is not provided because the list node state is not copyable. // The move construct/assignment is not provided because all modifications to the list shall go through the list object. - IntrusiveListNodeBase(const IntrusiveListNodeBase &) = delete; - IntrusiveListNodeBase & operator=(const IntrusiveListNodeBase &) = delete; - IntrusiveListNodeBase(IntrusiveListNodeBase &&) = delete; - IntrusiveListNodeBase & operator=(IntrusiveListNodeBase &&) = delete; + IntrusiveListNodePrivateBase(const IntrusiveListNodePrivateBase &) = delete; + IntrusiveListNodePrivateBase & operator=(const IntrusiveListNodePrivateBase &) = delete; + IntrusiveListNodePrivateBase(IntrusiveListNodePrivateBase &&) = delete; + IntrusiveListNodePrivateBase & operator=(IntrusiveListNodePrivateBase &&) = delete; bool IsInList() const { return (mPrev != nullptr && mNext != nullptr); } private: friend class IntrusiveListBase; - IntrusiveListNodeBase(IntrusiveListNodeBase * prev, IntrusiveListNodeBase * next) : mPrev(prev), mNext(next) {} + IntrusiveListNodePrivateBase(IntrusiveListNodePrivateBase * prev, IntrusiveListNodePrivateBase * next) : + mPrev(prev), mNext(next) + {} - void TakePlaceOf(const IntrusiveListNodeBase * that) + void TakePlaceOf(const IntrusiveListNodePrivateBase * that) { // prerequisite `that` is in a list // `this` will take place of the position of `that`. @@ -55,7 +113,7 @@ class IntrusiveListNodeBase mNext->mPrev = this; } - void Prepend(IntrusiveListNodeBase * node) + void Prepend(IntrusiveListNodePrivateBase * node) { VerifyOrDie(IsInList()); VerifyOrDie(!node->IsInList()); @@ -65,7 +123,7 @@ class IntrusiveListNodeBase mPrev = node; } - void Append(IntrusiveListNodeBase * node) + void Append(IntrusiveListNodePrivateBase * node) { VerifyOrDie(IsInList()); VerifyOrDie(!node->IsInList()); @@ -75,6 +133,7 @@ class IntrusiveListNodeBase mNext = node; } +protected: void Remove() { VerifyOrDie(IsInList()); @@ -84,11 +143,31 @@ class IntrusiveListNodeBase mNext = nullptr; } - IntrusiveListNodeBase * mPrev; - IntrusiveListNodeBase * mNext; +private: + IntrusiveListNodePrivateBase * mPrev; + IntrusiveListNodePrivateBase * mNext; }; -// non template part of IntrusiveList +// Strict mode implementation +template +class IntrusiveListNodeBase : public IntrusiveListNodePrivateBase +{ +}; + +// Partial specialization implementation for auto-unlink mode. +template <> +class IntrusiveListNodeBase : public IntrusiveListNodePrivateBase +{ +public: + ~IntrusiveListNodeBase() { Unlink(); } + void Unlink() + { + if (IsInList()) + Remove(); + } +}; + +// Non-template part of IntrusiveList. It appears that for list structure, both mode can share same implementation. class IntrusiveListBase { public: @@ -96,9 +175,9 @@ class IntrusiveListBase { private: friend class IntrusiveListBase; - ConstIteratorBase(const IntrusiveListNodeBase * cur) : mCurrent(cur) {} + ConstIteratorBase(const IntrusiveListNodePrivateBase * cur) : mCurrent(cur) {} - const IntrusiveListNodeBase & operator*() { return *mCurrent; } + const IntrusiveListNodePrivateBase & operator*() { return *mCurrent; } public: using difference_type = std::ptrdiff_t; @@ -139,16 +218,16 @@ class IntrusiveListBase } protected: - const IntrusiveListNodeBase * mCurrent; + const IntrusiveListNodePrivateBase * mCurrent; }; class IteratorBase { private: friend class IntrusiveListBase; - IteratorBase(IntrusiveListNodeBase * cur) : mCurrent(cur) {} + IteratorBase(IntrusiveListNodePrivateBase * cur) : mCurrent(cur) {} - IntrusiveListNodeBase & operator*() { return *mCurrent; } + IntrusiveListNodePrivateBase & operator*() { return *mCurrent; } public: using difference_type = std::ptrdiff_t; @@ -189,7 +268,7 @@ class IntrusiveListBase } protected: - IntrusiveListNodeBase * mCurrent; + IntrusiveListNodePrivateBase * mCurrent; }; bool Empty() const { return mNode.mNext == &mNode; } @@ -239,15 +318,15 @@ class IntrusiveListBase IteratorBase begin() { return IteratorBase(mNode.mNext); } IteratorBase end() { return IteratorBase(&mNode); } - void PushFront(IntrusiveListNodeBase * node) { mNode.Append(node); } - void PushBack(IntrusiveListNodeBase * node) { mNode.Prepend(node); } + void PushFront(IntrusiveListNodePrivateBase * node) { mNode.Append(node); } + void PushBack(IntrusiveListNodePrivateBase * node) { mNode.Prepend(node); } - void InsertBefore(IteratorBase pos, IntrusiveListNodeBase * node) { pos.mCurrent->Prepend(node); } - void InsertAfter(IteratorBase pos, IntrusiveListNodeBase * node) { pos.mCurrent->Append(node); } - void Remove(IntrusiveListNodeBase * node) { node->Remove(); } + void InsertBefore(IteratorBase pos, IntrusiveListNodePrivateBase * node) { pos.mCurrent->Prepend(node); } + void InsertAfter(IteratorBase pos, IntrusiveListNodePrivateBase * node) { pos.mCurrent->Append(node); } + void Remove(IntrusiveListNodePrivateBase * node) { node->Remove(); } /// @brief Replace an original node in list with a new node. - void Replace(IntrusiveListNodeBase * original, IntrusiveListNodeBase * replacement) + void Replace(IntrusiveListNodePrivateBase * original, IntrusiveListNodePrivateBase * replacement) { // VerifyOrDie(Contains(original)); This check is too heavy to do, but it shall hold VerifyOrDie(!replacement->IsInList()); @@ -256,7 +335,7 @@ class IntrusiveListBase original->mNext = nullptr; } - bool Contains(const IntrusiveListNodeBase * node) const + bool Contains(const IntrusiveListNodePrivateBase * node) const { for (auto & iter : *this) { @@ -267,24 +346,30 @@ class IntrusiveListBase } private: - IntrusiveListNodeBase mNode; + IntrusiveListNodePrivateBase mNode; }; /// The hook converts between node object T and IntrusiveListNodeBase /// /// When using this hook, the node type (T) MUST inherit from IntrusiveListNodeBase. /// -template +template class IntrusiveListBaseHook { public: - static_assert(std::is_base_of::value, "T must be derived from IntrusiveListNodeBase"); + static_assert(std::is_base_of, T>::value, "T must be derived from IntrusiveListNodeBase"); - static T * ToObject(IntrusiveListNodeBase * node) { return static_cast(node); } - static const T * ToObject(const IntrusiveListNodeBase * node) { return static_cast(node); } + static T * ToObject(IntrusiveListNodePrivateBase * node) { return static_cast(node); } + static const T * ToObject(const IntrusiveListNodePrivateBase * node) { return static_cast(node); } - static IntrusiveListNodeBase * ToNode(T * object) { return static_cast(object); } - static const IntrusiveListNodeBase * ToNode(const T * object) { return static_cast(object); } + static T * ToObject(IntrusiveListNodeBase * node) { return static_cast(node); } + static const T * ToObject(const IntrusiveListNodeBase * node) { return static_cast(node); } + + static IntrusiveListNodeBase * ToNode(T * object) { return static_cast *>(object); } + static const IntrusiveListNodeBase * ToNode(const T * object) + { + return static_cast *>(object); + } }; /// A double-linked list where the data is stored together with the previous/next pointers for cache efficiency / and compactness. @@ -310,12 +395,10 @@ class IntrusiveListBaseHook /// assert(list.Contains(&a) && list.Contains(&b) && !list.Contains(&c)); /// list.Remove(&a); /// list.Remove(&b); -template > +template > class IntrusiveList : public IntrusiveListBase { public: - static_assert(std::is_base_of::value, "T must derive from IntrusiveListNodeBase"); - IntrusiveList() : IntrusiveListBase() {} IntrusiveList(IntrusiveList &&) = default; diff --git a/src/lib/support/tests/TestIntrusiveList.cpp b/src/lib/support/tests/TestIntrusiveList.cpp index d35412fd5057e1..ddb121b1262355 100644 --- a/src/lib/support/tests/TestIntrusiveList.cpp +++ b/src/lib/support/tests/TestIntrusiveList.cpp @@ -27,7 +27,7 @@ namespace { using namespace chip; -class ListNode : public IntrusiveListNodeBase +class ListNode : public IntrusiveListNodeBase<> { }; @@ -191,6 +191,35 @@ void TestMoveList(nlTestSuite * inSuite, void * inContext) } } +class ListNodeAutoUnlink : public IntrusiveListNodeBase +{ +}; + +void TestAutoUnlink(nlTestSuite * inSuite, void * inContext) +{ + IntrusiveList list; + + // Test case 1: Test node->Unlink() + { + ListNodeAutoUnlink a; + NL_TEST_ASSERT(inSuite, !list.Contains(&a)); + list.PushBack(&a); + NL_TEST_ASSERT(inSuite, list.Contains(&a)); + a.Unlink(); + NL_TEST_ASSERT(inSuite, !list.Contains(&a)); + NL_TEST_ASSERT(inSuite, list.Empty()); + } + + // Test case 2: The node is automatically removed when goes out of scope + { + ListNodeAutoUnlink a; + NL_TEST_ASSERT(inSuite, !list.Contains(&a)); + list.PushBack(&a); + NL_TEST_ASSERT(inSuite, list.Contains(&a)); + } + NL_TEST_ASSERT(inSuite, list.Empty()); +} + int Setup(void * inContext) { return SUCCESS; @@ -212,6 +241,7 @@ static const nlTest sTests[] = { NL_TEST_DEF_FN(TestContains), // NL_TEST_DEF_FN(TestReplaceNode), // NL_TEST_DEF_FN(TestMoveList), // + NL_TEST_DEF_FN(TestAutoUnlink), // NL_TEST_SENTINEL(), // }; diff --git a/src/transport/SessionHolder.h b/src/transport/SessionHolder.h index 7e0433a6c345ea..a9510a4a84fcb9 100644 --- a/src/transport/SessionHolder.h +++ b/src/transport/SessionHolder.h @@ -28,7 +28,7 @@ namespace chip { * released when the underlying session is released. One must verify it is available before use. The object can be * created using SessionHandle.Grab() */ -class SessionHolder : public SessionDelegate, public IntrusiveListNodeBase +class SessionHolder : public SessionDelegate, public IntrusiveListNodeBase<> { public: SessionHolder() {}