remove read client/handler and write handler when corresponding fabric is removed (#21204)
* remove read client/handler and write handler when corresponding fabric is removed
* address comments
* address comments
* address comments
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
index 9913c1a..5196e5f 100644
--- a/src/app/InteractionModelEngine.cpp
+++ b/src/app/InteractionModelEngine.cpp
@@ -56,6 +56,7 @@
mpFabricTable = apFabricTable;
mpCASESessionMgr = apCASESessionMgr;
+ ReturnErrorOnFailure(mpFabricTable->AddFabricDelegate(this));
ReturnErrorOnFailure(mpExchangeMgr->RegisterUnsolicitedMessageHandlerForProtocol(Protocols::InteractionModel::Id, this));
mReportingEngine.Init();
@@ -76,9 +77,9 @@
//
while (handlerIter)
{
- CommandHandlerInterface * next = handlerIter->GetNext();
+ CommandHandlerInterface * nextHandler = handlerIter->GetNext();
handlerIter->SetNext(nullptr);
- handlerIter = next;
+ handlerIter = nextHandler;
}
mCommandHandlerList = nullptr;
@@ -235,24 +236,6 @@
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())
@@ -1213,9 +1196,9 @@
ObjectList<T> * current = aObjectList;
while (current != nullptr)
{
- ObjectList<T> * next = current->mpNext;
+ ObjectList<T> * nextObject = current->mpNext;
aObjectPool.ReleaseObject(current);
- current = next;
+ current = nextObject;
}
aObjectList = nullptr;
@@ -1429,5 +1412,41 @@
return numDirtySubscriptions;
}
+void InteractionModelEngine::OnFabricRemoved(const FabricTable & fabricTable, FabricIndex fabricIndex)
+{
+ mReadHandlers.ForEachActiveObject([fabricIndex](ReadHandler * handler) {
+ if (handler->GetAccessingFabricIndex() == fabricIndex)
+ {
+ ChipLogProgress(InteractionModel, "Deleting expired ReadHandler for NodeId: " ChipLogFormatX64 ", FabricIndex: %u",
+ ChipLogValueX64(handler->GetInitiatorNodeId()), 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_ERROR_IM_FABRIC_DELETED, false);
+ }
+ }
+
+ 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 aee1ad6..0c2efa0 100644
--- a/src/app/InteractionModelEngine.h
+++ b/src/app/InteractionModelEngine.h
@@ -74,7 +74,8 @@
class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
public Messaging::ExchangeDelegate,
public CommandHandler::Callback,
- public ReadHandler::ManagementCallback
+ public ReadHandler::ManagementCallback,
+ public FabricTable::Delegate
{
public:
/**
@@ -143,12 +144,6 @@
*/
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;
@@ -289,6 +284,9 @@
*/
uint16_t GetMinGuaranteedSubscriptionsPerFabric() const;
+ // virtual method from FabricTable::Delegate
+ 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.h b/src/app/ReadClient.h
index 0f5a72c..89ea133 100644
--- a/src/app/ReadClient.h
+++ b/src/app/ReadClient.h
@@ -272,14 +272,6 @@
*/
~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 7a27266..a1b8831 100644
--- a/src/app/WriteHandler.h
+++ b/src/app/WriteHandler.h
@@ -76,6 +76,11 @@
*/
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 @@
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 ab0b3f8..ae60c18 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 @@
void CleanupSessionsForFabric(SessionManager & sessionMgr, FabricIndex fabricIndex)
{
- InteractionModelEngine::GetInstance()->CloseTransactionsFromFabricIndex(fabricIndex);
sessionMgr.ExpireAllSessionsForFabric(fabricIndex);
}
@@ -379,6 +378,7 @@
ChipLogProgress(Zcl, "OpCreds: Fabric index 0x%x was removed", static_cast<unsigned>(fabricIndex));
EventManagement::GetInstance().FabricRemoved(fabricIndex);
+
NotifyFabricTableChanged();
}
diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp
index 1123f5c..54f19a7 100644
--- a/src/app/tests/TestReadInteraction.cpp
+++ b/src/app/tests/TestReadInteraction.cpp
@@ -335,6 +335,7 @@
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,67 @@
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);
+
+ ReadPrepareParams readPrepareParams(ctx.GetSessionBobToAlice());
+ readPrepareParams.mpAttributePathParamsList = new chip::app::AttributePathParams[1];
+ readPrepareParams.mAttributePathParamsListSize = 1;
+
+ 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;
+
+ {
+ 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);
+ 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, delegate.mError == CHIP_ERROR_IM_FABRIC_DELETED);
+ ctx.ExpireSessionAliceToBob();
+ ctx.ExpireSessionBobToAlice();
+ ctx.CreateAliceFabric();
+ ctx.CreateBobFabric();
+ ctx.CreateSessionAliceToBob();
+ ctx.CreateSessionBobToAlice();
+ }
+ NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadClients() == 0);
+ engine->Shutdown();
+ NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0);
+}
+
} // namespace app
} // namespace chip
@@ -3712,6 +3774,7 @@
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 dcd13e5..65f6e8b 100644
--- a/src/app/tests/TestWriteInteraction.cpp
+++ b/src/app/tests/TestWriteInteraction.cpp
@@ -69,6 +69,7 @@
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 @@
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 @@
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/lib/core/CHIPError.cpp b/src/lib/core/CHIPError.cpp
index c66f2d3..1dc00ab 100644
--- a/src/lib/core/CHIPError.cpp
+++ b/src/lib/core/CHIPError.cpp
@@ -545,8 +545,8 @@
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";
diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h
index be89841..5a17051 100644
--- a/src/lib/core/CHIPError.h
+++ b/src/lib/core/CHIPError.h
@@ -1879,13 +1879,12 @@
#define CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND CHIP_CORE_ERROR(0xa0)
/**
- * @def CHIP_ERROR_PROFILE_STRING_CONTEXT_ALREADY_REGISTERED
+ * @def CHIP_ERROR_IM_FABRIC_DELETED
*
- * @brief
- * The specified profile string support context is already registered.
- *
+ * @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
diff --git a/src/lib/core/tests/TestCHIPErrorStr.cpp b/src/lib/core/tests/TestCHIPErrorStr.cpp
index e071d13..ebf5cd0 100644
--- a/src/lib/core/tests/TestCHIPErrorStr.cpp
+++ b/src/lib/core/tests/TestCHIPErrorStr.cpp
@@ -209,7 +209,7 @@
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,
diff --git a/src/messaging/tests/MessagingContext.cpp b/src/messaging/tests/MessagingContext.cpp
index 00fd532..24c6ff2 100644
--- a/src/messaging/tests/MessagingContext.cpp
+++ b/src/messaging/tests/MessagingContext.cpp
@@ -55,13 +55,8 @@
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());
@@ -122,6 +117,20 @@
}
}
+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,
diff --git a/src/messaging/tests/MessagingContext.h b/src/messaging/tests/MessagingContext.h
index f4108ff..d60f057 100644
--- a/src/messaging/tests/MessagingContext.h
+++ b/src/messaging/tests/MessagingContext.h
@@ -148,6 +148,9 @@
SessionHandle GetSessionDavidToCharlie();
SessionHandle GetSessionBobToFriends();
+ CHIP_ERROR CreateAliceFabric();
+ CHIP_ERROR CreateBobFabric();
+
const Transport::PeerAddress & GetAliceAddress() { return mAliceAddress; }
const Transport::PeerAddress & GetBobAddress() { return mBobAddress; }
