From 2108178d3707c04fa0200d59b2b0b61acaccc2fc Mon Sep 17 00:00:00 2001 From: Song GUO Date: Fri, 12 Nov 2021 13:05:58 +0800 Subject: [PATCH] [IM] Add refcount to CommandHandler for async commands (#11367) * [IM] Add refcount methnism for async background work * Invalid CommandHandler::Handle when chip stack shutdown * mRefCount -> mPendingWork * Upd --- src/app/CommandHandler.cpp | 77 ++++++++++++++++--- src/app/CommandHandler.h | 72 +++++++++++++++-- src/app/InteractionModelEngine.cpp | 31 ++++---- src/app/InteractionModelEngine.h | 9 +++ src/app/tests/TestCommandInteraction.cpp | 42 ++++++++++ .../tests/data_model/TestCommands.cpp | 60 +++++++++++++++ 6 files changed, 260 insertions(+), 31 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 9e01074e1ed2f3..cc0b19bc3f7adc 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -65,25 +65,19 @@ CHIP_ERROR CommandHandler::AllocateBuffer() CHIP_ERROR CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader, System::PacketBufferHandle && payload) { - CHIP_ERROR err = CHIP_NO_ERROR; System::PacketBufferHandle response; - VerifyOrReturnError(mState == CommandState::Idle, CHIP_ERROR_INCORRECT_STATE); // NOTE: we already know this is an InvokeCommand Request message because we explicitly registered with the // Exchange Manager for unsolicited InvokeCommand Requests. - mpExchangeCtx = ec; - err = ProcessInvokeRequest(std::move(payload)); - SuccessOrExit(err); - - err = SendCommandResponse(); - SuccessOrExit(err); + // Use the RAII feature, if this is the only Handle when this function returns, DecrementHoldOff will trigger sending response. + Handle workHandle(this); + mpExchangeCtx->WillSendMessage(); + ReturnErrorOnFailure(ProcessInvokeRequest(std::move(payload))); -exit: - Close(); - return err; + return CHIP_NO_ERROR; } CHIP_ERROR CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payload) @@ -125,6 +119,12 @@ void CommandHandler::Close() mSuppressResponse = false; MoveToState(CommandState::AwaitingDestruction); + // We must finish all async work before we can shut down a CommandHandler. The actual CommandHandler MUST finish their work + // in reasonable time or there is a bug. The only case for releasing CommandHandler without CommandHandler::Handle releasing its + // 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 %zu unfinished async work items", + mPendingWork); + Command::Close(); if (mpCallback) @@ -133,10 +133,37 @@ void CommandHandler::Close() } } +void CommandHandler::IncrementHoldOff() +{ + mPendingWork++; +} + +void CommandHandler::DecrementHoldOff() +{ + mPendingWork--; + ChipLogDetail(DataManagement, "Decreasing reference count for CommandHandler, remaining %zu", mPendingWork); + if (mPendingWork != 0) + { + return; + } + CHIP_ERROR err = SendCommandResponse(); + if (err != CHIP_NO_ERROR) + { + ChipLogError(DataManagement, "Failed to send command response: %" CHIP_ERROR_FORMAT, err.Format()); + // We marked the exchange as "WillSendMessage", need to shutdown the exchange manually to avoid leaking exchanges. + if (mpExchangeCtx != nullptr) + { + mpExchangeCtx->Close(); + } + } + Close(); +} + CHIP_ERROR CommandHandler::SendCommandResponse() { System::PacketBufferHandle commandPacket; + VerifyOrReturnError(mPendingWork == 0, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mState == CommandState::AddedCommand, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mpExchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE); @@ -343,6 +370,34 @@ TLV::TLVWriter * CommandHandler::GetCommandDataIBTLVWriter() } } +CommandHandler * CommandHandler::Handle::Get() +{ + return (mMagic == InteractionModelEngine::GetInstance()->GetMagicNumber()) ? mpHandler : nullptr; +} + +void CommandHandler::Handle::Release() +{ + if (mpHandler != nullptr) + { + if (mMagic == InteractionModelEngine::GetInstance()->GetMagicNumber()) + { + mpHandler->DecrementHoldOff(); + } + mpHandler = nullptr; + mMagic = 0; + } +} + +CommandHandler::Handle::Handle(CommandHandler * handle) +{ + if (handle != nullptr) + { + handle->IncrementHoldOff(); + mpHandler = handle; + mMagic = InteractionModelEngine::GetInstance()->GetMagicNumber(); + } +} + } // namespace app } // namespace chip diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 5ea3810c2e17a4..544d4e46710521 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -71,6 +71,51 @@ class CommandHandler : public Command virtual bool CommandExists(const ConcreteCommandPath & aCommandPath) = 0; }; + class Handle + { + public: + Handle() {} + Handle(const Handle & handle) = delete; + Handle(Handle && handle) + { + mpHandler = handle.mpHandler; + mMagic = handle.mMagic; + handle.mpHandler = nullptr; + handle.mMagic = 0; + } + Handle(decltype(nullptr)) {} + Handle(CommandHandler * handle); + ~Handle() { Release(); } + + Handle & operator=(Handle && handle) + { + Release(); + mpHandler = handle.mpHandler; + mMagic = handle.mMagic; + handle.mpHandler = nullptr; + handle.mMagic = 0; + return *this; + } + + Handle & operator=(decltype(nullptr)) + { + Release(); + return *this; + } + + /** + * Get the CommandHandler object it holds. Get() may return a nullptr if the CommandHandler object is holds is no longer + * valid. + */ + CommandHandler * Get(); + + void Release(); + + private: + CommandHandler * mpHandler = nullptr; + uint32_t mMagic = 0; + }; + /* * Constructor. * @@ -122,6 +167,22 @@ class CommandHandler : public Command private: friend class TestCommandInteraction; + friend class CommandHandler::Handle; + + /** + * IncrementHoldOff will increase the inner refcount of the 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(); + + /** + * 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(); + /* * Allocates a packet buffer used for encoding an invoke response payload. * @@ -130,11 +191,11 @@ class CommandHandler : public Command */ CHIP_ERROR AllocateBuffer(); - // - // Called internally to signal the completion of all work on this object, gracefully close the - // exchange (by calling into the base class) and finally, signal to a registerd callback that it's - // safe to release this object. - // + /** + * Called internally to signal the completion of all work on this object, gracefully close the + * exchange (by calling into the base class) and finally, signal to a registerd callback that it's + * safe to release this object. + */ void Close(); CHIP_ERROR ProcessCommandDataIB(CommandDataIB::Parser & aCommandElement); @@ -146,6 +207,7 @@ class CommandHandler : public Command Callback * mpCallback = nullptr; InvokeResponseMessage::Builder mInvokeResponseBuilder; TLV::TLVType mDataElementContainerType = TLV::kTLVType_NotSpecified; + size_t mPendingWork = 0; bool mSuppressResponse = false; bool mTimedRequest = false; }; diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index b48978ee13d0e2..9aee8b3009079e 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -56,6 +56,8 @@ CHIP_ERROR InteractionModelEngine::Init(Messaging::ExchangeManager * apExchangeM mClusterInfoPool[CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS - 1].mpNext = nullptr; mpNextAvailableClusterInfo = mClusterInfoPool; + mMagic++; + return CHIP_NO_ERROR; } @@ -76,21 +78,20 @@ void InteractionModelEngine::Shutdown() mCommandHandlerList = nullptr; - // - // Since modifying the pool during iteration is generally frowned upon, - // I've chosen to just destroy the object but not necessarily de-allocate it. - // - // This poses a problem when shutting down and restarting the stack, since the - // IMEngine is a statically constructed singleton, and this lingering state will - // cause issues. - // - // This doesn't pose a problem right now because there shouldn't be any actual objects - // left here due to the synchronous nature of command handling. - // - // Filed #10332 to track this. - // - mCommandHandlerObjs.ForEachActiveObject([](CommandHandler * obj) -> bool { - obj->~CommandHandler(); + // Increase magic number to invalidate all Handle-s. + mMagic++; + + mCommandHandlerObjs.ForEachActiveObject([this](CommandHandler * obj) -> bool { + // Modifying the pool during iteration is generally frowned upon. + // This is almost safe since mCommandHandlerObjs is a BitMapObjectPool which won't malfunction when modifying the inner + // record while during traversal. But this behavior is not guranteed, so we should fix this by implementing DeallocateAll. + // + // Deallocate an CommandHandler will call its destructor (and abort the exchange context it holds) without calling + // Shutdown(). + // + // TODO(@kghost, #10332) Implement DeallocateAll and replace this. + + mCommandHandlerObjs.Deallocate(obj); return true; }); diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index ccb8bbd009ca40..05bdd4b9f07cf7 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -177,6 +177,11 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman uint16_t GetWriteClientArrayIndex(const WriteClient * const apWriteClient) const; + /** + * The Magic number of this InteractionModelEngine, the magic number is set during Init() + */ + uint32_t GetMagicNumber() { return mMagic; } + reporting::Engine & GetReportingEngine() { return mReportingEngine; } void ReleaseClusterInfoList(ClusterInfo *& aClusterInfo); @@ -245,6 +250,10 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman reporting::Engine mReportingEngine; ClusterInfo mClusterInfoPool[CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS]; ClusterInfo * mpNextAvailableClusterInfo = 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; }; void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip::TLV::TLVReader & aReader, diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 5883e649bd7fa8..c67cfb5db9fd18 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -52,6 +52,7 @@ namespace { bool isCommandDispatched = false; bool sendResponse = true; +bool asyncCommand = false; constexpr EndpointId kTestEndpointId = 1; constexpr ClusterId kTestClusterId = 3; @@ -61,6 +62,9 @@ constexpr CommandId kTestNonExistCommandId = 0; } // namespace namespace app { + +CommandHandler::Handle asyncCommandHandle; + bool ServerClusterCommandExists(const ConcreteCommandPath & aCommandPath) { // Mock cluster catalog, only support one command on one cluster on one endpoint. @@ -75,6 +79,12 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip "Received Cluster Command: Endpoint=%" PRIx16 " Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI, aCommandPath.mEndpointId, ChipLogValueMEI(aCommandPath.mClusterId), ChipLogValueMEI(aCommandPath.mCommandId)); + if (asyncCommand) + { + asyncCommandHandle = apCommandObj; + asyncCommand = false; + } + if (sendResponse) { if (aCommandPath.mCommandId == kTestCommandId) @@ -167,6 +177,7 @@ class TestCommandInteraction static void TestCommandHandlerWithProcessReceivedEmptyDataMsg(nlTestSuite * apSuite, void * apContext); static void TestCommandSenderCommandSuccessResponseFlow(nlTestSuite * apSuite, void * apContext); + static void TestCommandSenderCommandAsyncSuccessResponseFlow(nlTestSuite * apSuite, void * apContext); static void TestCommandSenderCommandFailureResponseFlow(nlTestSuite * apSuite, void * apContext); static void TestCommandSenderCommandSpecificResponseFlow(nlTestSuite * apSuite, void * apContext); @@ -593,6 +604,36 @@ void TestCommandInteraction::TestCommandSenderCommandSuccessResponseFlow(nlTestS NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } +void TestCommandInteraction::TestCommandSenderCommandAsyncSuccessResponseFlow(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; + + mockCommandSenderDelegate.ResetCounter(); + app::CommandSender commandSender(&mockCommandSenderDelegate, &ctx.GetExchangeManager()); + + AddInvokeRequestData(apSuite, apContext, &commandSender); + asyncCommand = true; + err = commandSender.SendCommandRequest(ctx.GetSessionBobToAlice()); + + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(apSuite, + mockCommandSenderDelegate.onResponseCalledTimes == 0 && mockCommandSenderDelegate.onFinalCalledTimes == 0 && + mockCommandSenderDelegate.onErrorCalledTimes == 0); + + NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 1); + NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 2); + + // Decrease CommandHandler refcount and send response + asyncCommandHandle = nullptr; + NL_TEST_ASSERT(apSuite, + mockCommandSenderDelegate.onResponseCalledTimes == 1 && mockCommandSenderDelegate.onFinalCalledTimes == 1 && + mockCommandSenderDelegate.onErrorCalledTimes == 0); + + NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0); + NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); +} + void TestCommandInteraction::TestCommandSenderCommandSpecificResponseFlow(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); @@ -692,6 +733,7 @@ const nlTest sTests[] = NL_TEST_DEF("TestCommandHandlerWithProcessReceivedEmptyDataMsg", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg), NL_TEST_DEF("TestCommandSenderCommandSuccessResponseFlow", chip::app::TestCommandInteraction::TestCommandSenderCommandSuccessResponseFlow), + NL_TEST_DEF("TestCommandSenderCommandAsyncSuccessResponseFlow", chip::app::TestCommandInteraction::TestCommandSenderCommandAsyncSuccessResponseFlow), NL_TEST_DEF("TestCommandSenderCommandSpecificResponseFlow", chip::app::TestCommandInteraction::TestCommandSenderCommandSpecificResponseFlow), NL_TEST_DEF("TestCommandSenderCommandFailureResponseFlow", chip::app::TestCommandInteraction::TestCommandSenderCommandFailureResponseFlow), NL_TEST_DEF("TestCommandSenderAbruptDestruction", chip::app::TestCommandInteraction::TestCommandSenderAbruptDestruction), diff --git a/src/controller/tests/data_model/TestCommands.cpp b/src/controller/tests/data_model/TestCommands.cpp index d390771df8f27c..faec93baf52364 100644 --- a/src/controller/tests/data_model/TestCommands.cpp +++ b/src/controller/tests/data_model/TestCommands.cpp @@ -39,6 +39,7 @@ using TestContext = chip::Test::AppContext; using namespace chip; +using namespace chip::app; using namespace chip::app::Clusters; namespace { @@ -54,9 +55,11 @@ enum ResponseDirective kSendError, kSendSuccessStatusCodeWithClusterStatus, kSendErrorWithClusterStatus, + kAsync, }; ResponseDirective responseDirective; +CommandHandler::Handle asyncHandle; } // namespace @@ -118,6 +121,10 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip { apCommandObj->AddClusterSpecificFailure(aCommandPath, kTestFailureClusterStatus); } + else if (responseDirective == kAsync) + { + asyncHandle = apCommandObj; + } } } @@ -149,6 +156,7 @@ class TestCommandInteraction TestCommandInteraction() {} static void TestDataResponse(nlTestSuite * apSuite, void * apContext); static void TestSuccessNoDataResponse(nlTestSuite * apSuite, void * apContext); + static void TestAsyncResponse(nlTestSuite * apSuite, void * apContext); static void TestFailure(nlTestSuite * apSuite, void * apContext); static void TestSuccessNoDataResponseWithClusterStatus(nlTestSuite * apSuite, void * apContext); static void TestFailureWithClusterStatus(nlTestSuite * apSuite, void * apContext); @@ -247,6 +255,57 @@ void TestCommandInteraction::TestSuccessNoDataResponse(nlTestSuite * apSuite, vo NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } +void TestCommandInteraction::TestAsyncResponse(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + TestCluster::Commands::TestSimpleArgumentRequest::Type request; + auto sessionHandle = ctx.GetSessionBobToAlice(); + + bool onSuccessWasCalled = false; + bool onFailureWasCalled = false; + bool statusCheck = false; + request.arg1 = true; + + // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's + // not safe to do so. + auto onSuccessCb = [&onSuccessWasCalled, &statusCheck](const app::ConcreteCommandPath & commandPath, + const app::StatusIB & aStatus, const auto & dataResponse) { + statusCheck = (aStatus.mStatus == Protocols::InteractionModel::Status::Success); + onSuccessWasCalled = true; + }; + + // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's + // not safe to do so. + auto onFailureCb = [&onFailureWasCalled](const app::StatusIB & aStatus, CHIP_ERROR aError) { onFailureWasCalled = true; }; + + responseDirective = kAsync; + + chip::Controller::InvokeCommandRequest(&ctx.GetExchangeManager(), sessionHandle, kTestEndpointId, request, onSuccessCb, + onFailureCb); + + ctx.DrainAndServiceIO(); + + NL_TEST_ASSERT(apSuite, !onSuccessWasCalled && !onFailureWasCalled && !statusCheck); + NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 2); + + CommandHandler * commandHandle = asyncHandle.Get(); + NL_TEST_ASSERT(apSuite, commandHandle != nullptr); + + if (commandHandle == nullptr) + { + return; + } + + commandHandle->AddStatus(ConcreteCommandPath(kTestEndpointId, request.GetClusterId(), request.GetCommandId()), + Protocols::InteractionModel::Status::Success); + asyncHandle.Release(); + + ctx.DrainAndServiceIO(); + + NL_TEST_ASSERT(apSuite, onSuccessWasCalled && !onFailureWasCalled && statusCheck); + NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); +} + void TestCommandInteraction::TestFailure(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); @@ -356,6 +415,7 @@ const nlTest sTests[] = { NL_TEST_DEF("TestDataResponse", TestCommandInteraction::TestDataResponse), NL_TEST_DEF("TestSuccessNoDataResponse", TestCommandInteraction::TestSuccessNoDataResponse), + NL_TEST_DEF("TestAsyncResponse", TestCommandInteraction::TestAsyncResponse), NL_TEST_DEF("TestFailure", TestCommandInteraction::TestFailure), NL_TEST_DEF("TestSuccessNoDataResponseWithClusterStatus", TestCommandInteraction::TestSuccessNoDataResponseWithClusterStatus), NL_TEST_DEF("TestFailureWithClusterStatus", TestCommandInteraction::TestFailureWithClusterStatus),