From 7e8804dfd707ad4ede2c2288354c4aaeb41c5c5c Mon Sep 17 00:00:00 2001 From: yunhanw Date: Mon, 25 Jul 2022 17:29:37 -0700 Subject: [PATCH 1/4] remove read client/handler and write handler when corresponding fabric is removed --- src/app/InteractionModelEngine.cpp | 45 +++++++++++-- src/app/InteractionModelEngine.h | 5 +- src/app/ReadClient.cpp | 2 +- src/app/ReadClient.h | 8 --- src/app/WriteHandler.h | 9 +-- .../operational-credentials-server.cpp | 8 ++- src/app/tests/TestReadInteraction.cpp | 67 +++++++++++++++++++ src/app/tests/TestWriteInteraction.cpp | 52 ++++++++++++++ src/messaging/tests/MessagingContext.cpp | 22 ++++-- src/messaging/tests/MessagingContext.h | 3 + 10 files changed, 194 insertions(+), 27 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 745d99293a23e0..86439c683e045b 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -56,6 +56,7 @@ CHIP_ERROR InteractionModelEngine::Init(Messaging::ExchangeManager * apExchangeM mpFabricTable = apFabricTable; mpCASESessionMgr = apCASESessionMgr; + ReturnErrorOnFailure(mpFabricTable->AddFabricDelegate(this)); ReturnErrorOnFailure(mpExchangeMgr->RegisterUnsolicitedMessageHandlerForProtocol(Protocols::InteractionModel::Id, this)); mReportingEngine.Init(); @@ -76,9 +77,9 @@ void InteractionModelEngine::Shutdown() // while (handlerIter) { - CommandHandlerInterface * next = handlerIter->GetNext(); + CommandHandlerInterface * pNext = handlerIter->GetNext(); handlerIter->SetNext(nullptr); - handlerIter = next; + handlerIter = pNext; } mCommandHandlerList = nullptr; @@ -1217,9 +1218,9 @@ void InteractionModelEngine::ReleasePool(ObjectList *& aObjectList, ObjectPoo ObjectList * current = aObjectList; while (current != nullptr) { - ObjectList * next = current->mpNext; + ObjectList * pNext = current->mpNext; aObjectPool.ReleaseObject(current); - current = next; + current = pNext; } aObjectList = nullptr; @@ -1434,5 +1435,41 @@ size_t InteractionModelEngine::GetNumDirtySubscriptions() const return numDirtySubscriptions; } +void InteractionModelEngine::OnFabricRemoved(const FabricTable & fabricTable, FabricIndex fabricIndex) +{ + mReadHandlers.ForEachActiveObject([fabricIndex](ReadHandler * handler) { + if (handler->GetAccessingFabricIndex() == fabricIndex) + { + ChipLogProgress(InteractionModel, "Fabric removed, deleting obsolete read handler with FabricIndex: %u", fabricIndex); + handler->Close(); + } + + return Loop::Continue; + }); + + for (auto * readClient = mpActiveReadClientList; readClient != nullptr; readClient = readClient->GetNextClient()) + { + if (readClient->GetFabricIndex() == fabricIndex) + { + ChipLogProgress(InteractionModel, "Fabric removed, deleting obsolete read client with FabricIndex: %u", fabricIndex); + readClient->Close(CHIP_NO_ERROR, false); + RemoveReadClient(readClient); + } + } + + for (auto & handler : mWriteHandlers) + { + if (!(handler.IsFree()) && handler.GetAccessingFabricIndex() == fabricIndex) + { + ChipLogProgress(InteractionModel, "Fabric removed, deleting obsolete write handler with FabricIndex: %u", fabricIndex); + handler.Close(); + } + } + + // Applications may hold references to CommandHandler instances for async command processing. + // Therefore we can't forcible destroy CommandHandlers here. Their exchanges will get closed by + // the fabric removal, though, so they will fail when they try to actually send their command response + // and will close at that point. +} } // namespace app } // namespace chip diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index 6dffea8e9fdcc6..8da9281f326ac2 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -74,7 +74,8 @@ namespace app { class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, public Messaging::ExchangeDelegate, public CommandHandler::Callback, - public ReadHandler::ManagementCallback + public ReadHandler::ManagementCallback, + public FabricTable::Delegate { public: /** @@ -289,6 +290,8 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, */ uint16_t GetMinGuaranteedSubscriptionsPerFabric() const; + void OnFabricRemoved(const FabricTable & fabricTable, FabricIndex fabricIndex) override; + #if CONFIG_BUILD_FOR_HOST_UNIT_TEST // // Get direct access to the underlying read handler pool diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 7ba7e9b9b1fb35..453e8aff4f63b3 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -86,7 +86,7 @@ ReadClient::~ReadClient() // This won't be the case if the engine shut down before this destructor was called (in which case, mpImEngine // will point to null) // - if (mpImEngine) + if (mpImEngine && mpImEngine->InActiveReadClientList(this)) { mpImEngine->RemoveReadClient(this); } diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h index 0f5a72c72eb714..89ea133fad5b1b 100644 --- a/src/app/ReadClient.h +++ b/src/app/ReadClient.h @@ -272,14 +272,6 @@ class ReadClient : public Messaging::ExchangeDelegate */ ~ReadClient() override; - /* - * This forcibly closes the exchange context if a valid one is pointed to. Such a situation does - * not arise during normal message processing flows that all normally call Close() above. This can only - * arise due to application-initiated destruction of the object when this object is handling receiving/sending - * message payloads. Abort() should be called first before the object is destroyed. - */ - void Abort(); - /** * Send a request. There can be one request outstanding on a given ReadClient. * If SendRequest returns success, no more SendRequest calls can happen on this ReadClient diff --git a/src/app/WriteHandler.h b/src/app/WriteHandler.h index 7a27266d4722d3..a1b8831165437a 100644 --- a/src/app/WriteHandler.h +++ b/src/app/WriteHandler.h @@ -76,6 +76,11 @@ class WriteHandler : public Messaging::ExchangeDelegate */ void Abort(); + /** + * Clean up state when we are done sending the write response. + */ + void Close(); + bool IsFree() const { return mState == State::Uninitialized; } ~WriteHandler() override = default; @@ -133,10 +138,6 @@ class WriteHandler : public Messaging::ExchangeDelegate void MoveToState(const State aTargetState); void ClearState(); const char * GetStateStr() const; - /** - * Clean up state when we are done sending the write response. - */ - void Close(); void DeliverListWriteBegin(const ConcreteAttributePath & aPath); void DeliverListWriteEnd(const ConcreteAttributePath & aPath, bool writeWasSuccessful); diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index ab0b3f8216857e..26e2c52d409418 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -282,7 +282,6 @@ CHIP_ERROR DeleteFabricFromTable(FabricIndex fabricIndex) void CleanupSessionsForFabric(SessionManager & sessionMgr, FabricIndex fabricIndex) { - InteractionModelEngine::GetInstance()->CloseTransactionsFromFabricIndex(fabricIndex); sessionMgr.ExpireAllSessionsForFabric(fabricIndex); } @@ -379,11 +378,16 @@ class OpCredsFabricTableDelegate : public chip::FabricTable::Delegate ChipLogProgress(Zcl, "OpCreds: Fabric index 0x%x was removed", static_cast(fabricIndex)); EventManagement::GetInstance().FabricRemoved(fabricIndex); + NotifyFabricTableChanged(); } // Gets called when a fabric is added/updated, but not necessarily committed to storage - void OnFabricUpdated(const FabricTable & fabricTable, FabricIndex fabricIndex) override { NotifyFabricTableChanged(); } + void OnFabricUpdated(const FabricTable & fabricTable, FabricIndex fabricIndex) override + { + ChipLogProgress(InteractionModel, "OnFabricUpdated debugging from opertonal-credential!!!!!!!"); + NotifyFabricTableChanged(); + } // Gets called when a fabric in FabricTable is persisted to storage void OnFabricCommitted(const FabricTable & fabricTable, FabricIndex fabricIndex) override diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index 1123f5ca10674f..a518e8419af22f 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -335,6 +335,7 @@ class TestReadInteraction static void TestSubscribeSendUnknownMessage(nlTestSuite * apSuite, void * apContext); static void TestSubscribeSendInvalidStatusReport(nlTestSuite * apSuite, void * apContext); static void TestReadHandlerInvalidSubscribeRequest(nlTestSuite * apSuite, void * apContext); + static void TestSubscribeInvalidateFabric(nlTestSuite * apSuite, void * apContext); private: static void GenerateReportData(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload, @@ -3668,6 +3669,71 @@ void TestReadInteraction::TestReadHandlerInvalidSubscribeRequest(nlTestSuite * a NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } +// Create the subscription, then remove the corresponding fabric in client and handler, the corresponding +// client and handler would be released as well. +void TestReadInteraction::TestSubscribeInvalidateFabric(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; + + Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); + // Shouldn't have anything in the retransmit table when starting the test. + NL_TEST_ASSERT(apSuite, rm->TestGetCountRetransTable() == 0); + + GenerateEvents(apSuite, apContext); + + MockInteractionModelApp delegate; + auto * engine = chip::app::InteractionModelEngine::GetInstance(); + err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable()); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(apSuite, !delegate.mGotEventResponse); + + ReadPrepareParams readPrepareParams(ctx.GetSessionBobToAlice()); + readPrepareParams.mEventPathParamsListSize = 0; + + readPrepareParams.mpAttributePathParamsList = new chip::app::AttributePathParams[2]; + readPrepareParams.mAttributePathParamsListSize = 2; + + readPrepareParams.mMinIntervalFloorSeconds = 0; + readPrepareParams.mMaxIntervalCeilingSeconds = 0; + + { + app::ReadClient readClient(chip::app::InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), delegate, + chip::app::ReadClient::InteractionType::Subscribe); + + delegate.mGotReport = false; + + err = readClient.SendAutoResubscribeRequest(std::move(readPrepareParams)); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + ctx.DrainAndServiceIO(); + + NL_TEST_ASSERT(apSuite, delegate.mGotReport); + + // We have 29 attributes in our mock attribute storage. And we subscribed twice. + // And attribute 3/2/4 is a list with 6 elements and list chunking is applied to it, thus we should receive ( 29 + 6 ) * 2 = + // 70 attribute data in total. + NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 70); + NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers(ReadHandler::InteractionType::Subscribe) == 1); + NL_TEST_ASSERT(apSuite, engine->ActiveHandlerAt(0) != nullptr); + delegate.mpReadHandler = engine->ActiveHandlerAt(0); + + ctx.GetFabricTable().Delete(ctx.GetAliceFabricIndex()); + NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers(ReadHandler::InteractionType::Subscribe) == 0); + ctx.GetFabricTable().Delete(ctx.GetBobFabricIndex()); + NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadClients() == 0); + ctx.ExpireSessionAliceToBob(); + ctx.ExpireSessionBobToAlice(); + ctx.CreateAliceFabric(); + ctx.CreateBobFabric(); + ctx.CreateSessionAliceToBob(); + ctx.CreateSessionBobToAlice(); + } + + engine->Shutdown(); + NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); +} + } // namespace app } // namespace chip @@ -3712,6 +3778,7 @@ const nlTest sTests[] = NL_TEST_DEF("TestSubscribeSendUnknownMessage", chip::app::TestReadInteraction::TestSubscribeSendUnknownMessage), NL_TEST_DEF("TestSubscribeSendInvalidStatusReport", chip::app::TestReadInteraction::TestSubscribeSendInvalidStatusReport), NL_TEST_DEF("TestReadHandlerInvalidSubscribeRequest", chip::app::TestReadInteraction::TestReadHandlerInvalidSubscribeRequest), + NL_TEST_DEF("TestSubscribeInvalidateFabric", chip::app::TestReadInteraction::TestSubscribeInvalidateFabric), NL_TEST_DEF("TestSubscribeUrgentWildcardEvent", chip::app::TestReadInteraction::TestSubscribeUrgentWildcardEvent), NL_TEST_DEF("TestSubscribeWildcard", chip::app::TestReadInteraction::TestSubscribeWildcard), NL_TEST_DEF("TestSubscribePartialOverlap", chip::app::TestReadInteraction::TestSubscribePartialOverlap), diff --git a/src/app/tests/TestWriteInteraction.cpp b/src/app/tests/TestWriteInteraction.cpp index dcd13e596859ea..65f6e8b5d300d1 100644 --- a/src/app/tests/TestWriteInteraction.cpp +++ b/src/app/tests/TestWriteInteraction.cpp @@ -69,6 +69,7 @@ class TestWriteInteraction static void TestWriteRoundtripWithClusterObjectsVersionMismatch(nlTestSuite * apSuite, void * apContext); #if CONFIG_BUILD_FOR_HOST_UNIT_TEST static void TestWriteHandlerReceiveInvalidMessage(nlTestSuite * apSuite, void * apContext); + static void TestWriteHandlerInvalidateFabric(nlTestSuite * apSuite, void * apContext); #endif private: static void AddAttributeDataIB(nlTestSuite * apSuite, void * apContext, WriteClient & aWriteClient); @@ -626,6 +627,56 @@ void TestWriteInteraction::TestWriteHandlerReceiveInvalidMessage(nlTestSuite * a ctx.CreateSessionAliceToBob(); ctx.CreateSessionBobToAlice(); } + +// This test is to create Chunked write requests, we drop the message since the 3rd message, then remove fabrics for client and +// handler, the corresponding client and handler would be released as well. +void TestWriteInteraction::TestWriteHandlerInvalidateFabric(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + auto sessionHandle = ctx.GetSessionBobToAlice(); + + app::AttributePathParams attributePath(2, 3, 4); + + CHIP_ERROR err = CHIP_NO_ERROR; + Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); + // Shouldn't have anything in the retransmit table when starting the test. + NL_TEST_ASSERT(apSuite, rm->TestGetCountRetransTable() == 0); + + TestWriteClientCallback writeCallback; + auto * engine = chip::app::InteractionModelEngine::GetInstance(); + err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable()); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + app::WriteClient writeClient(&ctx.GetExchangeManager(), &writeCallback, Optional::Missing(), + static_cast(900) /* reserved buffer size */); + + ByteSpan list[5]; + + err = writeClient.EncodeAttribute(attributePath, app::DataModel::List(list, 5)); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + ctx.GetLoopback().mDroppedMessageCount = 0; + ctx.GetLoopback().mSentMessageCount = 0; + ctx.GetLoopback().mNumMessagesToDrop = 1; + ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 2; + err = writeClient.SendWriteRequest(sessionHandle); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + ctx.DrainAndServiceIO(); + + NL_TEST_ASSERT(apSuite, InteractionModelEngine::GetInstance()->GetNumActiveWriteHandlers() == 1); + NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 3); + NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1); + + ctx.GetFabricTable().Delete(ctx.GetAliceFabricIndex()); + NL_TEST_ASSERT(apSuite, InteractionModelEngine::GetInstance()->GetNumActiveWriteHandlers() == 0); + engine->Shutdown(); + ctx.ExpireSessionAliceToBob(); + ctx.ExpireSessionBobToAlice(); + ctx.CreateAliceFabric(); + ctx.CreateSessionAliceToBob(); + ctx.CreateSessionBobToAlice(); +} + #endif // Write Client sends a write request, receives an unexpected message type, sends a status response to that. @@ -917,6 +968,7 @@ const nlTest sTests[] = NL_TEST_DEF("TestWriteRoundtripWithClusterObjectsVersionMismatch", chip::app::TestWriteInteraction::TestWriteRoundtripWithClusterObjectsVersionMismatch), #if CONFIG_BUILD_FOR_HOST_UNIT_TEST NL_TEST_DEF("TestWriteHandlerReceiveInvalidMessage", chip::app::TestWriteInteraction::TestWriteHandlerReceiveInvalidMessage), + NL_TEST_DEF("TestWriteHandlerInvalidateFabric", chip::app::TestWriteInteraction::TestWriteHandlerInvalidateFabric), #endif NL_TEST_DEF("TestWriteInvalidMessage1", chip::app::TestWriteInteraction::TestWriteInvalidMessage1), NL_TEST_DEF("TestWriteInvalidMessage2", chip::app::TestWriteInteraction::TestWriteInvalidMessage2), diff --git a/src/messaging/tests/MessagingContext.cpp b/src/messaging/tests/MessagingContext.cpp index 00fd5329925474..4696a8850f23b7 100644 --- a/src/messaging/tests/MessagingContext.cpp +++ b/src/messaging/tests/MessagingContext.cpp @@ -55,13 +55,8 @@ CHIP_ERROR MessagingContext::Init(TransportMgrBase * transport, IOContext * ioCo if (mInitializeNodes) { - ReturnErrorOnFailure(mFabricTable.AddNewFabricForTestIgnoringCollisions(GetRootACertAsset().mCert, GetIAA1CertAsset().mCert, - GetNodeA1CertAsset().mCert, - GetNodeA1CertAsset().mKey, &mAliceFabricIndex)); - - ReturnErrorOnFailure(mFabricTable.AddNewFabricForTestIgnoringCollisions(GetRootACertAsset().mCert, GetIAA1CertAsset().mCert, - GetNodeA2CertAsset().mCert, - GetNodeA2CertAsset().mKey, &mBobFabricIndex)); + ReturnErrorOnFailure(CreateAliceFabric()); + ReturnErrorOnFailure(CreateBobFabric()); ReturnErrorOnFailure(CreateSessionBobToAlice()); ReturnErrorOnFailure(CreateSessionAliceToBob()); @@ -120,6 +115,19 @@ void MessagingContext::SetMRPMode(MRPMode mode) mSessionDavidToCharlie->AsSecureSession()->SetRemoteMRPConfig( ReliableMessageProtocolConfig(System::Clock::Milliseconds32(10), System::Clock::Milliseconds32(10))); } + +CHIP_ERROR MessagingContext::CreateAliceFabric() +{ + return mFabricTable.AddNewFabricForTestIgnoringCollisions(GetRootACertAsset().mCert, GetIAA1CertAsset().mCert, + GetNodeA1CertAsset().mCert, GetNodeA1CertAsset().mKey, + &mAliceFabricIndex); +} + +CHIP_ERROR MessagingContext::CreateBobFabric() +{ + return mFabricTable.AddNewFabricForTestIgnoringCollisions(GetRootACertAsset().mCert, GetIAA1CertAsset().mCert, + GetNodeA2CertAsset().mCert, GetNodeA2CertAsset().mKey, + &mBobFabricIndex); } CHIP_ERROR MessagingContext::CreateSessionBobToAlice() diff --git a/src/messaging/tests/MessagingContext.h b/src/messaging/tests/MessagingContext.h index f4108ffb96949c..d60f057463806d 100644 --- a/src/messaging/tests/MessagingContext.h +++ b/src/messaging/tests/MessagingContext.h @@ -148,6 +148,9 @@ class MessagingContext : public PlatformMemoryUser SessionHandle GetSessionDavidToCharlie(); SessionHandle GetSessionBobToFriends(); + CHIP_ERROR CreateAliceFabric(); + CHIP_ERROR CreateBobFabric(); + const Transport::PeerAddress & GetAliceAddress() { return mAliceAddress; } const Transport::PeerAddress & GetBobAddress() { return mBobAddress; } From f83c21b4685f7d5a5159513acdfa1cfeea186d75 Mon Sep 17 00:00:00 2001 From: yunhanw Date: Wed, 10 Aug 2022 19:23:10 -0700 Subject: [PATCH 2/4] address comments --- src/app/InteractionModelEngine.cpp | 28 ++++--------------- src/app/InteractionModelEngine.h | 7 +---- src/app/ReadClient.cpp | 2 +- .../operational-credentials-server.cpp | 6 +--- src/app/tests/TestReadInteraction.cpp | 14 ++++------ src/lib/core/CHIPError.cpp | 3 ++ src/lib/core/CHIPError.h | 8 ++++++ 7 files changed, 25 insertions(+), 43 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 86439c683e045b..f3b5ff46fb010c 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -77,9 +77,9 @@ void InteractionModelEngine::Shutdown() // while (handlerIter) { - CommandHandlerInterface * pNext = handlerIter->GetNext(); + CommandHandlerInterface * nextHandler = handlerIter->GetNext(); handlerIter->SetNext(nullptr); - handlerIter = pNext; + handlerIter = nextHandler; } mCommandHandlerList = nullptr; @@ -236,24 +236,6 @@ uint32_t InteractionModelEngine::GetNumActiveWriteHandlers() const return numActive; } -void InteractionModelEngine::CloseTransactionsFromFabricIndex(FabricIndex aFabricIndex) -{ - // - // Walk through all existing subscriptions and shut down those whose subscriber matches - // that which just came in. - // - mReadHandlers.ForEachActiveObject([this, aFabricIndex](ReadHandler * handler) { - if (handler->GetAccessingFabricIndex() == aFabricIndex) - { - ChipLogProgress(InteractionModel, "Deleting expired ReadHandler for NodeId: " ChipLogFormatX64 ", FabricIndex: %u", - ChipLogValueX64(handler->GetInitiatorNodeId()), aFabricIndex); - mReadHandlers.ReleaseObject(handler); - } - - return Loop::Continue; - }); -} - CHIP_ERROR InteractionModelEngine::ShutdownSubscription(SubscriptionId aSubscriptionId) { for (auto * readClient = mpActiveReadClientList; readClient != nullptr; readClient = readClient->GetNextClient()) @@ -1440,7 +1422,8 @@ void InteractionModelEngine::OnFabricRemoved(const FabricTable & fabricTable, Fa mReadHandlers.ForEachActiveObject([fabricIndex](ReadHandler * handler) { if (handler->GetAccessingFabricIndex() == fabricIndex) { - ChipLogProgress(InteractionModel, "Fabric removed, deleting obsolete read handler with FabricIndex: %u", fabricIndex); + ChipLogProgress(InteractionModel, "Deleting expired ReadHandler for NodeId: " ChipLogFormatX64 ", FabricIndex: %u", + ChipLogValueX64(handler->GetInitiatorNodeId()), fabricIndex); handler->Close(); } @@ -1452,8 +1435,9 @@ void InteractionModelEngine::OnFabricRemoved(const FabricTable & fabricTable, Fa if (readClient->GetFabricIndex() == fabricIndex) { ChipLogProgress(InteractionModel, "Fabric removed, deleting obsolete read client with FabricIndex: %u", fabricIndex); - readClient->Close(CHIP_NO_ERROR, false); RemoveReadClient(readClient); + readClient->mpImEngine = nullptr; + readClient->Close(CHIP_ERROR_IM_FABRIC_DELETED, false); } } diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index 8da9281f326ac2..77081ed4907ee5 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -144,12 +144,6 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, */ void ShutdownAllSubscriptions(); - /** - * Expire active transactions and release related objects for the given fabric index. - * This is used for releasing transactions that won't be closed when a fabric is removed. - */ - void CloseTransactionsFromFabricIndex(FabricIndex aFabricIndex); - uint32_t GetNumActiveReadHandlers() const; uint32_t GetNumActiveReadHandlers(ReadHandler::InteractionType type) const; @@ -290,6 +284,7 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, */ uint16_t GetMinGuaranteedSubscriptionsPerFabric() const; + // virtual method from FabricTable::Delegate void OnFabricRemoved(const FabricTable & fabricTable, FabricIndex fabricIndex) override; #if CONFIG_BUILD_FOR_HOST_UNIT_TEST diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 453e8aff4f63b3..7ba7e9b9b1fb35 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -86,7 +86,7 @@ ReadClient::~ReadClient() // This won't be the case if the engine shut down before this destructor was called (in which case, mpImEngine // will point to null) // - if (mpImEngine && mpImEngine->InActiveReadClientList(this)) + if (mpImEngine) { mpImEngine->RemoveReadClient(this); } diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index 26e2c52d409418..ae60c18f70fe4f 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -383,11 +383,7 @@ class OpCredsFabricTableDelegate : public chip::FabricTable::Delegate } // Gets called when a fabric is added/updated, but not necessarily committed to storage - void OnFabricUpdated(const FabricTable & fabricTable, FabricIndex fabricIndex) override - { - ChipLogProgress(InteractionModel, "OnFabricUpdated debugging from opertonal-credential!!!!!!!"); - NotifyFabricTableChanged(); - } + void OnFabricUpdated(const FabricTable & fabricTable, FabricIndex fabricIndex) override { NotifyFabricTableChanged(); } // Gets called when a fabric in FabricTable is persisted to storage void OnFabricCommitted(const FabricTable & fabricTable, FabricIndex fabricIndex) override diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index a518e8419af22f..ac5fb91f0cf52b 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -3686,13 +3686,14 @@ void TestReadInteraction::TestSubscribeInvalidateFabric(nlTestSuite * apSuite, v auto * engine = chip::app::InteractionModelEngine::GetInstance(); err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable()); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - NL_TEST_ASSERT(apSuite, !delegate.mGotEventResponse); ReadPrepareParams readPrepareParams(ctx.GetSessionBobToAlice()); - readPrepareParams.mEventPathParamsListSize = 0; + readPrepareParams.mpAttributePathParamsList = new chip::app::AttributePathParams[1]; + readPrepareParams.mAttributePathParamsListSize = 1; - readPrepareParams.mpAttributePathParamsList = new chip::app::AttributePathParams[2]; - readPrepareParams.mAttributePathParamsListSize = 2; + readPrepareParams.mpAttributePathParamsList[0].mEndpointId = Test::kMockEndpoint3; + readPrepareParams.mpAttributePathParamsList[0].mClusterId = Test::MockClusterId(2); + readPrepareParams.mpAttributePathParamsList[0].mAttributeId = Test::MockAttributeId(4); readPrepareParams.mMinIntervalFloorSeconds = 0; readPrepareParams.mMaxIntervalCeilingSeconds = 0; @@ -3709,11 +3710,6 @@ void TestReadInteraction::TestSubscribeInvalidateFabric(nlTestSuite * apSuite, v ctx.DrainAndServiceIO(); NL_TEST_ASSERT(apSuite, delegate.mGotReport); - - // We have 29 attributes in our mock attribute storage. And we subscribed twice. - // And attribute 3/2/4 is a list with 6 elements and list chunking is applied to it, thus we should receive ( 29 + 6 ) * 2 = - // 70 attribute data in total. - NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 70); NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers(ReadHandler::InteractionType::Subscribe) == 1); NL_TEST_ASSERT(apSuite, engine->ActiveHandlerAt(0) != nullptr); delegate.mpReadHandler = engine->ActiveHandlerAt(0); diff --git a/src/lib/core/CHIPError.cpp b/src/lib/core/CHIPError.cpp index c66f2d3cd51321..ebbd7bac93ea57 100644 --- a/src/lib/core/CHIPError.cpp +++ b/src/lib/core/CHIPError.cpp @@ -731,6 +731,9 @@ bool FormatCHIPError(char * buf, uint16_t bufSize, CHIP_ERROR err) case CHIP_ERROR_MISSING_URI_SEPARATOR.AsInteger(): desc = "The URI separator is missing"; break; + case CHIP_ERROR_IM_FABRIC_DELETED.AsInteger(): + desc = "The fabric is deleted, and the corresponding IM resources are released"; + break; } #endif // !CHIP_CONFIG_SHORT_ERROR_STR diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h index be898418b86b8e..271d02c5c4ad27 100644 --- a/src/lib/core/CHIPError.h +++ b/src/lib/core/CHIPError.h @@ -2428,6 +2428,14 @@ using CHIP_ERROR = ::chip::ChipError; */ #define CHIP_ERROR_MISSING_URI_SEPARATOR CHIP_CORE_ERROR(0xe0) +/** + * @def CHIP_ERROR_IM_FABRIC_DELETED + * + * @brief + * The fabric is deleted, and the corresponding IM resources are released + */ +#define CHIP_ERROR_IM_FABRIC_DELETED CHIP_CORE_ERROR(0xe1) + // clang-format on // !!!!! IMPORTANT !!!!! If you add new CHIP errors, please update the translation From 8d8cc7fe3b11c257a25bfb5105cb6b1bd6ddbfef Mon Sep 17 00:00:00 2001 From: yunhanw Date: Thu, 11 Aug 2022 10:11:40 -0700 Subject: [PATCH 3/4] address comments --- src/app/InteractionModelEngine.cpp | 2 -- src/app/tests/TestReadInteraction.cpp | 4 ++-- src/messaging/tests/MessagingContext.cpp | 1 + 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index f3b5ff46fb010c..de111f4595d8e0 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -1435,8 +1435,6 @@ void InteractionModelEngine::OnFabricRemoved(const FabricTable & fabricTable, Fa if (readClient->GetFabricIndex() == fabricIndex) { ChipLogProgress(InteractionModel, "Fabric removed, deleting obsolete read client with FabricIndex: %u", fabricIndex); - RemoveReadClient(readClient); - readClient->mpImEngine = nullptr; readClient->Close(CHIP_ERROR_IM_FABRIC_DELETED, false); } } diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index ac5fb91f0cf52b..54f19a7207c57b 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -3717,7 +3717,7 @@ void TestReadInteraction::TestSubscribeInvalidateFabric(nlTestSuite * apSuite, v ctx.GetFabricTable().Delete(ctx.GetAliceFabricIndex()); NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers(ReadHandler::InteractionType::Subscribe) == 0); ctx.GetFabricTable().Delete(ctx.GetBobFabricIndex()); - NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadClients() == 0); + NL_TEST_ASSERT(apSuite, delegate.mError == CHIP_ERROR_IM_FABRIC_DELETED); ctx.ExpireSessionAliceToBob(); ctx.ExpireSessionBobToAlice(); ctx.CreateAliceFabric(); @@ -3725,7 +3725,7 @@ void TestReadInteraction::TestSubscribeInvalidateFabric(nlTestSuite * apSuite, v ctx.CreateSessionAliceToBob(); ctx.CreateSessionBobToAlice(); } - + NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadClients() == 0); engine->Shutdown(); NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } diff --git a/src/messaging/tests/MessagingContext.cpp b/src/messaging/tests/MessagingContext.cpp index 4696a8850f23b7..24c6ff216d4ae3 100644 --- a/src/messaging/tests/MessagingContext.cpp +++ b/src/messaging/tests/MessagingContext.cpp @@ -115,6 +115,7 @@ void MessagingContext::SetMRPMode(MRPMode mode) mSessionDavidToCharlie->AsSecureSession()->SetRemoteMRPConfig( ReliableMessageProtocolConfig(System::Clock::Milliseconds32(10), System::Clock::Milliseconds32(10))); } +} CHIP_ERROR MessagingContext::CreateAliceFabric() { From 0bea2ed1745553c62c59c7802377232351d8e500 Mon Sep 17 00:00:00 2001 From: yunhanw Date: Thu, 11 Aug 2022 15:06:18 -0700 Subject: [PATCH 4/4] address comments --- src/app/InteractionModelEngine.cpp | 4 ++-- src/lib/core/CHIPError.cpp | 7 ++----- src/lib/core/CHIPError.h | 17 ++++------------- src/lib/core/tests/TestCHIPErrorStr.cpp | 2 +- 4 files changed, 9 insertions(+), 21 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index de111f4595d8e0..83ba10b2e42ada 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -1200,9 +1200,9 @@ void InteractionModelEngine::ReleasePool(ObjectList *& aObjectList, ObjectPoo ObjectList * current = aObjectList; while (current != nullptr) { - ObjectList * pNext = current->mpNext; + ObjectList * nextObject = current->mpNext; aObjectPool.ReleaseObject(current); - current = pNext; + current = nextObject; } aObjectList = nullptr; diff --git a/src/lib/core/CHIPError.cpp b/src/lib/core/CHIPError.cpp index ebbd7bac93ea57..1dc00abcbcf0c8 100644 --- a/src/lib/core/CHIPError.cpp +++ b/src/lib/core/CHIPError.cpp @@ -545,8 +545,8 @@ bool FormatCHIPError(char * buf, uint16_t bufSize, CHIP_ERROR err) case CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND.AsInteger(): desc = "Value not found in the persisted storage"; break; - case CHIP_ERROR_PROFILE_STRING_CONTEXT_ALREADY_REGISTERED.AsInteger(): - desc = "String context already registered"; + case CHIP_ERROR_IM_FABRIC_DELETED.AsInteger(): + desc = "The fabric is deleted, and the corresponding IM resources are released"; break; case CHIP_ERROR_PROFILE_STRING_CONTEXT_NOT_REGISTERED.AsInteger(): desc = "String context not registered"; @@ -731,9 +731,6 @@ bool FormatCHIPError(char * buf, uint16_t bufSize, CHIP_ERROR err) case CHIP_ERROR_MISSING_URI_SEPARATOR.AsInteger(): desc = "The URI separator is missing"; break; - case CHIP_ERROR_IM_FABRIC_DELETED.AsInteger(): - desc = "The fabric is deleted, and the corresponding IM resources are released"; - break; } #endif // !CHIP_CONFIG_SHORT_ERROR_STR diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h index 271d02c5c4ad27..5a170519126f2f 100644 --- a/src/lib/core/CHIPError.h +++ b/src/lib/core/CHIPError.h @@ -1879,13 +1879,12 @@ using CHIP_ERROR = ::chip::ChipError; #define CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND CHIP_CORE_ERROR(0xa0) /** - * @def CHIP_ERROR_PROFILE_STRING_CONTEXT_ALREADY_REGISTERED - * - * @brief - * The specified profile string support context is already registered. + * @def CHIP_ERROR_IM_FABRIC_DELETED * + * @brief + * The fabric is deleted, and the corresponding IM resources are released */ -#define CHIP_ERROR_PROFILE_STRING_CONTEXT_ALREADY_REGISTERED CHIP_CORE_ERROR(0xa1) +#define CHIP_ERROR_IM_FABRIC_DELETED CHIP_CORE_ERROR(0xa1) /** * @def CHIP_ERROR_PROFILE_STRING_CONTEXT_NOT_REGISTERED @@ -2428,14 +2427,6 @@ using CHIP_ERROR = ::chip::ChipError; */ #define CHIP_ERROR_MISSING_URI_SEPARATOR CHIP_CORE_ERROR(0xe0) -/** - * @def CHIP_ERROR_IM_FABRIC_DELETED - * - * @brief - * The fabric is deleted, and the corresponding IM resources are released - */ -#define CHIP_ERROR_IM_FABRIC_DELETED CHIP_CORE_ERROR(0xe1) - // clang-format on // !!!!! IMPORTANT !!!!! If you add new CHIP errors, please update the translation diff --git a/src/lib/core/tests/TestCHIPErrorStr.cpp b/src/lib/core/tests/TestCHIPErrorStr.cpp index e071d13b55b792..ebf5cd0f32a72f 100644 --- a/src/lib/core/tests/TestCHIPErrorStr.cpp +++ b/src/lib/core/tests/TestCHIPErrorStr.cpp @@ -209,7 +209,7 @@ static const CHIP_ERROR kTestElements[] = CHIP_ERROR_DEFAULT_EVENT_HANDLER_NOT_CALLED, CHIP_ERROR_PERSISTED_STORAGE_FAILED, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND, - CHIP_ERROR_PROFILE_STRING_CONTEXT_ALREADY_REGISTERED, + CHIP_ERROR_IM_FABRIC_DELETED, CHIP_ERROR_PROFILE_STRING_CONTEXT_NOT_REGISTERED, CHIP_ERROR_INCOMPATIBLE_SCHEMA_VERSION, CHIP_ERROR_ACCESS_DENIED,