Remove dependency of InteractionModelEngine in CommandHandler (#31830)

* Remove dependency of InteractionModelEngine in CommandHandler * Address comments and fix tizen test * Fix tizen example * Fix segfault in CommandHandler::Handle::Get() * Change CommandHandler::Callback function name * Add virtual function in CommandHandler::Callback in CommandResponseSender * Add mpCallback null checks * Add weak reference to CommandHandler * Remove magic number from ImEngine * self clean up * Restyled by clang-format * Use IntrusiveList for weak references of the Handles * Update src/app/CommandHandler.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Update src/app/CommandHandler.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Update src/app/CommandHandler.h Co-authored-by: Boris Zbarsky <[email protected]> * compile fix after renaming --------- Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · May 7, 2024 · 6090618 · 6090618
1 parent a2792ff
commit 6090618
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 43 deletions.
diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp
@@ -35,6 +35,7 @@
 #include <lib/core/CHIPConfig.h>
 #include <lib/core/TLVData.h>
 #include <lib/core/TLVUtilities.h>
+#include <lib/support/IntrusiveList.h>
 #include <lib/support/TypeTraits.h>
 #include <platform/LockTracker.h>
 #include <protocols/secure_channel/Constants.h>
@@ -58,6 +59,11 @@ CommandHandler::CommandHandler(TestOnlyOverrides & aTestOverride, Callback * apC
     }
 }
 
+CommandHandler::~CommandHandler()
+{
+    InvalidateHandles();
+}
+
 CHIP_ERROR CommandHandler::AllocateBuffer()
 {
     // We should only allocate a buffer if we will be sending out a response.
@@ -266,23 +272,48 @@ void CommandHandler::Close()
     // reference is the stack shutting down, in which case Close() is not called. So the below check should always pass.
     VerifyOrDieWithMsg(mPendingWork == 0, DataManagement, "CommandHandler::Close() called with %u unfinished async work items",
                        static_cast<unsigned int>(mPendingWork));
+    InvalidateHandles();
 
     if (mpCallback)
     {
         mpCallback->OnDone(*this);
     }
 }
 
-void CommandHandler::IncrementHoldOff()
+void CommandHandler::AddToHandleList(Handle * apHandle)
+{
+    mpHandleList.PushBack(apHandle);
+}
+
+void CommandHandler::RemoveFromHandleList(Handle * apHandle)
+{
+    VerifyOrDie(mpHandleList.Contains(apHandle));
+    mpHandleList.Remove(apHandle);
+}
+
+void CommandHandler::InvalidateHandles()
+{
+    for (auto handle = mpHandleList.begin(); handle != mpHandleList.end(); ++handle)
+    {
+        handle->Invalidate();
+    }
+}
+
+void CommandHandler::IncrementHoldOff(Handle * apHandle)
 {
     mPendingWork++;
+    AddToHandleList(apHandle);
 }
 
-void CommandHandler::DecrementHoldOff()
+void CommandHandler::DecrementHoldOff(Handle * apHandle)
 {
+
     mPendingWork--;
     ChipLogDetail(DataManagement, "Decreasing reference count for CommandHandler, remaining %u",
                   static_cast<unsigned int>(mPendingWork));
+
+    RemoveFromHandleList(apHandle);
+
     if (mPendingWork != 0)
     {
         return;
@@ -771,35 +802,35 @@ FabricIndex CommandHandler::GetAccessingFabricIndex() const
     return mpResponder->GetAccessingFabricIndex();
 }
 
+void CommandHandler::Handle::Init(CommandHandler * handler)
+{
+    if (handler != nullptr)
+    {
+        handler->IncrementHoldOff(this);
+        mpHandler = handler;
+    }
+}
+
 CommandHandler * CommandHandler::Handle::Get()
 {
     // Not safe to work with CommandHandler in parallel with other Matter work.
     assertChipStackLockedByCurrentThread();
 
-    return (mMagic == InteractionModelEngine::GetInstance()->GetMagicNumber()) ? mpHandler : nullptr;
+    return mpHandler;
 }
 
 void CommandHandler::Handle::Release()
 {
     if (mpHandler != nullptr)
     {
-        if (mMagic == InteractionModelEngine::GetInstance()->GetMagicNumber())
-        {
-            mpHandler->DecrementHoldOff();
-        }
-        mpHandler = nullptr;
-        mMagic    = 0;
+        mpHandler->DecrementHoldOff(this);
+        Invalidate();
     }
 }
 
-CommandHandler::Handle::Handle(CommandHandler * handle)
+CommandHandler::Handle::Handle(CommandHandler * handler)
 {
-    if (handle != nullptr)
-    {
-        handle->IncrementHoldOff();
-        mpHandler = handle;
-        mMagic    = InteractionModelEngine::GetInstance()->GetMagicNumber();
-    }
+    Init(handler);
 }
 
 CHIP_ERROR CommandHandler::FinalizeInvokeResponseMessageAndPrepareNext()

diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h
@@ -41,6 +41,7 @@
 #include <lib/support/BitFlags.h>
 #include <lib/support/CodeUtils.h>
 #include <lib/support/DLLUtil.h>
+#include <lib/support/IntrusiveList.h>
 #include <lib/support/Scoped.h>
 #include <lib/support/logging/CHIPLogging.h>
 #include <messaging/ExchangeHolder.h>
@@ -110,29 +111,26 @@ class CommandHandler
      * The Invoke Response will not be sent until all outstanding Handles have
      * been destroyed or have had Release called.
      */
-    class Handle
+    class Handle : public IntrusiveListNodeBase<>
     {
     public:
         Handle() {}
         Handle(const Handle & handle) = delete;
         Handle(Handle && handle)
         {
-            mpHandler        = handle.mpHandler;
-            mMagic           = handle.mMagic;
-            handle.mpHandler = nullptr;
-            handle.mMagic    = 0;
+            Init(handle.mpHandler);
+            handle.Release();
         }
         Handle(decltype(nullptr)) {}
-        Handle(CommandHandler * handle);
+        Handle(CommandHandler * handler);
         ~Handle() { Release(); }
 
         Handle & operator=(Handle && handle)
         {
             Release();
-            mpHandler        = handle.mpHandler;
-            mMagic           = handle.mMagic;
-            handle.mpHandler = nullptr;
-            handle.mMagic    = 0;
+            Init(handle.mpHandler);
+
+            handle.Release();
             return *this;
         }
 
@@ -143,16 +141,19 @@ class CommandHandler
         }
 
         /**
-         * Get the CommandHandler object it holds. Get() may return a nullptr if the CommandHandler object is holds is no longer
+         * Get the CommandHandler object it holds. Get() may return a nullptr if the CommandHandler object it holds is no longer
          * valid.
          */
         CommandHandler * Get();
 
         void Release();
 
+        void Invalidate() { mpHandler = nullptr; }
+
     private:
+        void Init(CommandHandler * handler);
+
         CommandHandler * mpHandler = nullptr;
-        uint32_t mMagic            = 0;
     };
 
     // Previously we kept adding arguments with default values individually as parameters. This is because there
@@ -191,6 +192,14 @@ class CommandHandler
      */
     CommandHandler(Callback * apCallback);
 
+    /*
+     * Destructor.
+     *
+     * The call will also invalidate all Handles created for this CommandHandler.
+     *
+     */
+    ~CommandHandler();
+
     /*
      * Constructor to override the number of supported paths per invoke and command responder.
      *
@@ -525,13 +534,13 @@ class CommandHandler
      * Users should use CommandHandler::Handle for management the lifespan of the CommandHandler.
      * DefRef should be released in reasonable time, and Close() should only be called when the refcount reached 0.
      */
-    void IncrementHoldOff();
+    void IncrementHoldOff(Handle * apHandle);
 
     /**
      * DecrementHoldOff is used by CommandHandler::Handle for decreasing the refcount of the CommandHandler.
      * When refcount reached 0, CommandHandler will send the response to the peer and shutdown.
      */
-    void DecrementHoldOff();
+    void DecrementHoldOff(Handle * apHandle);
 
     /*
      * Allocates a packet buffer used for encoding an invoke response payload.
@@ -672,12 +681,20 @@ class CommandHandler
 
     size_t MaxPathsPerInvoke() const { return mMaxPathsPerInvoke; }
 
+    void AddToHandleList(Handle * handle);
+
+    void RemoveFromHandleList(Handle * handle);
+
+    void InvalidateHandles();
+
     bool TestOnlyIsInIdleState() const { return mState == State::Idle; }
 
     Callback * mpCallback = nullptr;
     InvokeResponseMessage::Builder mInvokeResponseBuilder;
     TLV::TLVType mDataElementContainerType = TLV::kTLVType_NotSpecified;
     size_t mPendingWork                    = 0;
+    /* List to store all currently-outstanding Handles for this Command Handler.*/
+    IntrusiveList<Handle> mpHandleList;
 
     chip::System::PacketBufferTLVWriter mCommandMessageWriter;
     TLV::TLVWriter mBackupWriter;

diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -85,7 +85,6 @@ CHIP_ERROR InteractionModelEngine::Init(Messaging::ExchangeManager * apExchangeM
     ReturnErrorOnFailure(mpExchangeMgr->RegisterUnsolicitedMessageHandlerForProtocol(Protocols::InteractionModel::Id, this));
 
     mReportingEngine.Init();
-    mMagic++;
 
     StatusIB::RegisterErrorFormatter();
 
@@ -111,9 +110,6 @@ void InteractionModelEngine::Shutdown()
 
     mCommandHandlerList = nullptr;
 
-    // Increase magic number to invalidate all Handle-s.
-    mMagic++;
-
     mCommandResponderObjs.ReleaseAll();
 
     mTimedHandlers.ForEachActiveObject([this](TimedHandler * obj) -> Loop {

diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h
@@ -192,11 +192,6 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
      */
     WriteHandler * ActiveWriteHandlerAt(unsigned int aIndex);
 
-    /**
-     * The Magic number of this InteractionModelEngine, the magic number is set during Init()
-     */
-    uint32_t GetMagicNumber() const { return mMagic; }
-
     reporting::Engine & GetReportingEngine() { return mReportingEngine; }
 
     reporting::ReportScheduler * GetReportScheduler() { return mReportScheduler; }
@@ -706,10 +701,6 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
     CASESessionManager * mpCASESessionMgr = nullptr;
 
     SubscriptionResumptionStorage * mpSubscriptionResumptionStorage = nullptr;
-
-    // A magic number for tracking values between stack Shutdown()-s and Init()-s.
-    // An ObjectHandle is valid iff. its magic equals to this one.
-    uint32_t mMagic = 0;
 };
 
 } // namespace app