diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index ec999d3cc53a31..76060d2d5649be 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -1967,13 +1967,20 @@ void InteractionModelEngine::OnFabricRemoved(const FabricTable & fabricTable, Fa }); #if CHIP_CONFIG_ENABLE_READ_CLIENT - for (auto * readClient = mpActiveReadClientList; readClient != nullptr; readClient = readClient->GetNextClient()) + for (auto * readClient = mpActiveReadClientList; readClient != nullptr;) { if (readClient->GetFabricIndex() == fabricIndex) { ChipLogProgress(InteractionModel, "Fabric removed, deleting obsolete read client with FabricIndex: %u", fabricIndex); + auto * nextReadClient = readClient->GetNextClient(); readClient->Close(CHIP_ERROR_IM_FABRIC_DELETED, false); + readClient = nextReadClient; + } + else + { + readClient = readClient->GetNextClient(); } + } #endif // CHIP_CONFIG_ENABLE_READ_CLIENT diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index 362cdecab9910a..8463cd172edfa3 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -324,12 +324,19 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, /** * @brief Function decrements the number of subscriptions to resume counter - mNumOfSubscriptionsToResume. * This should be called after we have completed a re-subscribe attempt on a persisted subscription wether the attempt - * was succesful or not. + * was successful or not. */ void DecrementNumSubscriptionsToResume(); #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS #if CONFIG_BUILD_FOR_HOST_UNIT_TEST + + // + // Get fabric table + // + FabricTable * GetFabricTable() { return mpFabricTable; } + + // // Get direct access to the underlying read handler pool // @@ -714,7 +721,7 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, #endif // CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS - FabricTable * mpFabricTable; + FabricTable * mpFabricTable = nullptr; CASESessionManager * mpCASESessionMgr = nullptr; diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 2d609eb1afa05a..5547c68ea17b7d 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -113,6 +113,9 @@ ReadClient::~ReadClient() mpImEngine->RemoveReadClient(this); } } + mpExchangeMgr = nullptr; + mpNext = nullptr; + mpImEngine = nullptr; } uint32_t ReadClient::ComputeTimeTillNextSubscription() diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp index 136fbc243db340..957768ac2ec26e 100644 --- a/src/controller/tests/data_model/TestRead.cpp +++ b/src/controller/tests/data_model/TestRead.cpp @@ -4510,6 +4510,64 @@ TEST_F(TestRead, TestReadHandler_TwoParallelReadsSecondTooManyPaths) engine->SetForceHandlerQuota(false); } +TEST_F(TestRead, TestReadHandler_MultipleSubscriptions_OnFabricRemoved) +{ + auto sessionHandle = GetSessionBobToAlice(); + uint32_t numSuccessCalls = 0; + uint32_t numSubscriptionEstablishedCalls = 0; + + ScopedChange directive(gReadResponseDirective, ReadResponseDirective::kSendDataResponse); + + // 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 = [&numSuccessCalls](const app::ConcreteDataAttributePath & attributePath, const auto & dataResponse) { + numSuccessCalls++; + }; + + // 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 = [](const app::ConcreteDataAttributePath * attributePath, CHIP_ERROR aError) { + }; + + auto onSubscriptionEstablishedCb = [&numSubscriptionEstablishedCalls](const app::ReadClient & readClient, + chip::SubscriptionId aSubscriptionId) { + numSubscriptionEstablishedCalls++; + }; + + // + // Try to issue parallel subscriptions that will exceed the value for app::InteractionModelEngine::kReadHandlerPoolSize. + // If heap allocation is correctly setup, this should result in it successfully servicing more than the number + // present in that define. + // + for (size_t i = 0; i < (app::InteractionModelEngine::kReadHandlerPoolSize + 1); i++) + { + EXPECT_EQ(Controller::SubscribeAttribute( + &GetExchangeManager(), sessionHandle, kTestEndpointId, onSuccessCb, onFailureCb, 0, 20, + onSubscriptionEstablishedCb, nullptr, false, true), + CHIP_NO_ERROR); + } + + // There are too many messages and the test (gcc_debug, which includes many sanity checks) will be quite slow. Note: report + // engine is using ScheduleWork which cannot be handled by DrainAndServiceIO correctly. + GetIOContext().DriveIOUntil(System::Clock::Seconds16(60), [&]() { + return numSuccessCalls == (app::InteractionModelEngine::kReadHandlerPoolSize + 1) && + numSubscriptionEstablishedCalls == (app::InteractionModelEngine::kReadHandlerPoolSize + 1); + }); + + EXPECT_EQ(numSuccessCalls, (app::InteractionModelEngine::kReadHandlerPoolSize + 1)); + EXPECT_EQ(numSubscriptionEstablishedCalls, (app::InteractionModelEngine::kReadHandlerPoolSize + 1)); + EXPECT_EQ(mNumActiveSubscriptions, static_cast(app::InteractionModelEngine::kReadHandlerPoolSize + 1)); + + app::InteractionModelEngine::GetInstance()->GetFabricTable()->DeleteAllFabrics(); + + EXPECT_EQ(mNumActiveSubscriptions, 0); + size_t numActiveReadClients = app::InteractionModelEngine::GetInstance()->GetNumActiveReadClients(); + EXPECT_EQ(numActiveReadClients, 0u); + EXPECT_EQ(GetExchangeManager().GetNumActiveExchanges(), 0u); + + SetMRPMode(chip::Test::MessagingContext::MRPMode::kDefault); +} + TEST_F(TestRead, TestReadAttribute_ManyDataValues) { auto sessionHandle = GetSessionBobToAlice();