remove read client/handler and write handler when corresponding fabri…

…c is removed

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<T> *& aObjectList, ObjectPoo
     ObjectList<T> * current = aObjectList;
     while (current != nullptr)
     {
-        ObjectList<T> * next = current->mpNext;
+        ObjectList<T> * 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

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

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);
         }

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

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);

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<unsigned>(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

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,
@@ -3651,6 +3652,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<TestContext *>(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
 
@@ -3695,6 +3761,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),

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<TestContext *>(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<uint16_t>::Missing(),
+                                 static_cast<uint16_t>(900) /* reserved buffer size */);
+
+    ByteSpan list[5];
+
+    err = writeClient.EncodeAttribute(attributePath, app::DataModel::List<ByteSpan>(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),

src/messaging/tests/MessagingContext.cpp

@@ -53,13 +53,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());
@@ -98,6 +93,20 @@ void MessagingContext::ShutdownAndRestoreExisting(MessagingContext & existing)
     existing.mTransport->SetSessionManager(&existing.GetSecureSessionManager());
 }
 
+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()
 {
     return mSessionManager.InjectPaseSessionWithTestKey(mSessionBobToAlice, kBobKeyId, GetAliceFabric()->GetNodeId(), kAliceKeyId,

src/messaging/tests/MessagingContext.h

@@ -133,6 +133,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; }