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 <bzbarsky@apple.com>
* Update src/app/CommandHandler.cpp
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Update src/app/CommandHandler.h
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* compile fix after renaming
---------
Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp
index 005067c..89ff52f 100644
--- 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()
+{
+ InvalidateHandles();
+}
+
CHIP_ERROR CommandHandler::AllocateBuffer()
{
// We should only allocate a buffer if we will be sending out a response.
@@ -266,6 +272,7 @@
// 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)
{
@@ -273,16 +280,40 @@
}
}
-void CommandHandler::IncrementHoldOff()
+void CommandHandler::AddToHandleList(Handle * apHandle)
{
- mPendingWork++;
+ mpHandleList.PushBack(apHandle);
}
-void CommandHandler::DecrementHoldOff()
+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(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 @@
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
index 2eed73d..cce1987 100644
--- 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 @@
* 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 @@
}
/**
- * 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
@@ -192,6 +193,14 @@
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.
*
* The callback and any pointers passed via TestOnlyOverrides must outlive this
@@ -525,13 +534,13 @@
* 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 @@
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
index 64c06f9..9752876 100644
--- a/src/app/InteractionModelEngine.cpp
+++ b/src/app/InteractionModelEngine.cpp
@@ -85,7 +85,6 @@
ReturnErrorOnFailure(mpExchangeMgr->RegisterUnsolicitedMessageHandlerForProtocol(Protocols::InteractionModel::Id, this));
mReportingEngine.Init();
- mMagic++;
StatusIB::RegisterErrorFormatter();
@@ -111,9 +110,6 @@
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
index baf02c6..87c497c 100644
--- a/src/app/InteractionModelEngine.h
+++ b/src/app/InteractionModelEngine.h
@@ -192,11 +192,6 @@
*/
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 @@
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
