diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 005067c96bd7c4..89ff52f9669d2e 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -35,6 +35,7 @@ #include #include #include +#include #include #include #include @@ -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,6 +272,7 @@ 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(mPendingWork)); + InvalidateHandles(); if (mpCallback) { @@ -273,16 +280,40 @@ void CommandHandler::Close() } } -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(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 index 2eed73daccdda9..cce19879a6c8a9 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -41,6 +41,7 @@ #include #include #include +#include #include #include #include @@ -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 mpHandleList; chip::System::PacketBufferTLVWriter mCommandMessageWriter; TLV::TLVWriter mBackupWriter; diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 64c06f92354f48..97528768cb2561 100644 --- 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 index baf02c6af73d1c..87c497c9e9955d 100644 --- 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