Skip to content

Commit

Permalink
Fix the status codes returned from failed Read interactions. (#18575)
Browse files Browse the repository at this point in the history 
Fixes #18343
  • Loading branch information
@bzbarsky-apple
bzbarsky-apple authored May 19, 2022
1 parent 3baabc1 commit f662b71
Show file tree
Hide file tree
Showing 4 changed files with 189 additions and 9 deletions.
18 changes: 12 additions & 6 deletions src/app/InteractionModelEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,7 @@ bool InteractionModelEngine::EnsureResourceForSubscription(FabricIndex aFabricIn
return true;
}

bool InteractionModelEngine::CanEstablishReadTransaction(const ReadHandler * apReadHandler)
CHIP_ERROR InteractionModelEngine::CanEstablishReadTransaction(const ReadHandler * apReadHandler)
{
#if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP && !CHIP_CONFIG_IM_FORCE_FABRIC_QUOTA_CHECK
#if CONFIG_IM_BUILD_FOR_UNIT_TEST
Expand All @@ -759,6 +759,15 @@ bool InteractionModelEngine::CanEstablishReadTransaction(const ReadHandler * apR
size_t activeReadHandlersOnCurrentFabric = 0;

// It is safe to use & here since this function will be called on current stack.
if (apReadHandler->GetAttributePathCount() > kReservedPathsPerReadRequest ||
apReadHandler->GetEventPathCount() > kReservedPathsPerReadRequest ||
apReadHandler->GetDataVersionFilterCount() > kReservedPathsPerReadRequest)
{
// Doesn't matter how many other reads are in progress; we are not
// going to handle this request.
return (allowUnlimited ? CHIP_NO_ERROR : CHIP_IM_GLOBAL_STATUS(PathsExhausted));
}

mReadHandlers.ForEachActiveObject([&](ReadHandler * handler) {
if (handler->GetAccessingFabricIndex() == currentFabricIndex && handler->IsType(ReadHandler::InteractionType::Read))
{
Expand All @@ -770,13 +779,10 @@ bool InteractionModelEngine::CanEstablishReadTransaction(const ReadHandler * apR
// The incoming read handler here is also counted above.
if (activeReadHandlersOnCurrentFabric > kReservedReadHandlersPerFabricForReadRequests)
{
return allowUnlimited;
return (allowUnlimited ? CHIP_NO_ERROR : CHIP_IM_GLOBAL_STATUS(Busy));
}

return (apReadHandler->GetAttributePathCount() <= kReservedPathsPerReadRequest &&
apReadHandler->GetEventPathCount() <= kReservedPathsPerReadRequest &&
apReadHandler->GetDataVersionFilterCount() <= kReservedPathsPerReadRequest) ||
allowUnlimited;
return CHIP_NO_ERROR;
}

void InteractionModelEngine::RemoveReadClient(ReadClient * apReadClient)
Expand Down
7 changes: 6 additions & 1 deletion src/app/InteractionModelEngine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,10 +244,15 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
* kReservedPathsPerReadRequest. This function will check if the given ReadHandler will exceed the limitations for the accessing
* fabric.
*
* If CHIP_NO_ERROR is returned, it's OK to proceed with the read.
*
* Otherwise the CHIP_ERROR encodes an interaction model status that needs
* to turn into a Status Response to the client.
*
* TODO: (#17418) We are now reserving resources for read requests, could be changed to similar algorithm for read resources
* minimas.
*/
bool CanEstablishReadTransaction(const ReadHandler * apReadHandler);
CHIP_ERROR CanEstablishReadTransaction(const ReadHandler * apReadHandler);

/**
* Select the oldest (and the one that exceeds the per subscription resource minimum if there are any) read handler on the
Expand Down
5 changes: 3 additions & 2 deletions src/app/ReadHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,8 +384,9 @@ CHIP_ERROR ReadHandler::ProcessReadRequest(System::PacketBufferHandle && aPayloa
}
ReturnErrorOnFailure(err);

// Ensure the read transaction doesn't exceed the resources dedicated to read transactions.
VerifyOrReturnError(InteractionModelEngine::GetInstance()->CanEstablishReadTransaction(this), CHIP_ERROR_NO_MEMORY);
// Ensure the read transaction doesn't exceed the resources dedicated to
// read transactions.
ReturnErrorOnFailure(InteractionModelEngine::GetInstance()->CanEstablishReadTransaction(this));

bool isFabricFiltered;
ReturnErrorOnFailure(readRequestParser.GetIsFabricFiltered(&isFabricFiltered));
Expand Down
168 changes: 168 additions & 0 deletions src/controller/tests/data_model/TestRead.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,9 @@ class TestReadInteraction : public app::ReadHandler::ApplicationCallback
static void TestReadSubscribeAttributeResponseWithCache(nlTestSuite * apSuite, void * apContext);
static void TestReadHandler_KillOverQuotaSubscriptions(nlTestSuite * apSuite, void * apContext);
static void TestReadHandler_KillOldestSubscriptions(nlTestSuite * apSuite, void * apContext);
static void TestReadHandler_TwoParallelReads(nlTestSuite * apSuite, void * apContext);
static void TestReadHandler_TooManyPaths(nlTestSuite * apSuite, void * apContext);
static void TestReadHandler_TwoParallelReadsSecondTooManyPaths(nlTestSuite * apSuite, void * apContext);

private:
static constexpr uint16_t kTestMinInterval = 33;
Expand Down Expand Up @@ -1823,6 +1826,168 @@ void TestReadInteraction::TestReadHandler_KillOldestSubscriptions(nlTestSuite *
app::InteractionModelEngine::GetInstance()->SetPathPoolCapacityForSubscriptions(-1);
}

void TestReadInteraction::TestReadHandler_TwoParallelReads(nlTestSuite * apSuite, void * apContext)
{
using namespace chip::app;

TestContext & ctx = *static_cast<TestContext *>(apContext);

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

auto * engine = app::InteractionModelEngine::GetInstance();
engine->SetForceHandlerQuota(true);

app::ReadPrepareParams readPrepareParams(ctx.GetSessionBobToAlice());
// Read full wildcard paths, repeat twice to ensure chunking.
chip::app::AttributePathParams attributePathParams[2];
readPrepareParams.mpAttributePathParamsList = attributePathParams;
readPrepareParams.mAttributePathParamsListSize = ArraySize(attributePathParams);

{
MockInteractionModelApp delegate1;
NL_TEST_ASSERT(apSuite, delegate1.mNumAttributeResponse == 0);
NL_TEST_ASSERT(apSuite, !delegate1.mReadError);
app::ReadClient readClient1(InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), delegate1,
ReadClient::InteractionType::Read);

MockInteractionModelApp delegate2;
NL_TEST_ASSERT(apSuite, delegate2.mNumAttributeResponse == 0);
NL_TEST_ASSERT(apSuite, !delegate2.mReadError);
ReadClient readClient2(InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), delegate2,
ReadClient::InteractionType::Read);

CHIP_ERROR err = readClient1.SendRequest(readPrepareParams);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

err = readClient2.SendRequest(readPrepareParams);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

ctx.DrainAndServiceIO();

NL_TEST_ASSERT(apSuite, delegate1.mNumAttributeResponse != 0);
NL_TEST_ASSERT(apSuite, !delegate1.mReadError);

NL_TEST_ASSERT(apSuite, delegate2.mNumAttributeResponse == 0);
NL_TEST_ASSERT(apSuite, delegate2.mReadError);

StatusIB status(delegate2.mError);
NL_TEST_ASSERT(apSuite, status.mStatus == Protocols::InteractionModel::Status::Busy);
}

NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadClients() == 0);
NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0);
engine->SetForceHandlerQuota(false);
}

// Needs to be larger than our plausible path pool.
constexpr size_t sTooLargePathCount = 200;

void TestReadInteraction::TestReadHandler_TooManyPaths(nlTestSuite * apSuite, void * apContext)
{
using namespace chip::app;

TestContext & ctx = *static_cast<TestContext *>(apContext);

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

auto * engine = InteractionModelEngine::GetInstance();
engine->SetForceHandlerQuota(true);

ReadPrepareParams readPrepareParams(ctx.GetSessionBobToAlice());
// Needs to be larger than our plausible path pool.
chip::app::AttributePathParams attributePathParams[sTooLargePathCount];
readPrepareParams.mpAttributePathParamsList = attributePathParams;
readPrepareParams.mAttributePathParamsListSize = ArraySize(attributePathParams);

{
MockInteractionModelApp delegate;
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 0);
NL_TEST_ASSERT(apSuite, !delegate.mReadError);
ReadClient readClient(InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), delegate,
ReadClient::InteractionType::Read);

CHIP_ERROR err = readClient.SendRequest(readPrepareParams);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

ctx.DrainAndServiceIO();

NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 0);
NL_TEST_ASSERT(apSuite, delegate.mReadError);

StatusIB status(delegate.mError);
NL_TEST_ASSERT(apSuite, status.mStatus == Protocols::InteractionModel::Status::PathsExhausted);
}

NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadClients() == 0);
NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0);
engine->SetForceHandlerQuota(false);
}

void TestReadInteraction::TestReadHandler_TwoParallelReadsSecondTooManyPaths(nlTestSuite * apSuite, void * apContext)
{
using namespace chip::app;

TestContext & ctx = *static_cast<TestContext *>(apContext);

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

auto * engine = InteractionModelEngine::GetInstance();
engine->SetForceHandlerQuota(true);

{
MockInteractionModelApp delegate1;
NL_TEST_ASSERT(apSuite, delegate1.mNumAttributeResponse == 0);
NL_TEST_ASSERT(apSuite, !delegate1.mReadError);
ReadClient readClient1(InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), delegate1,
ReadClient::InteractionType::Read);

MockInteractionModelApp delegate2;
NL_TEST_ASSERT(apSuite, delegate2.mNumAttributeResponse == 0);
NL_TEST_ASSERT(apSuite, !delegate2.mReadError);
ReadClient readClient2(InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), delegate2,
ReadClient::InteractionType::Read);

ReadPrepareParams readPrepareParams1(ctx.GetSessionBobToAlice());
// Read full wildcard paths, repeat twice to ensure chunking.
chip::app::AttributePathParams attributePathParams1[2];
readPrepareParams1.mpAttributePathParamsList = attributePathParams1;
readPrepareParams1.mAttributePathParamsListSize = ArraySize(attributePathParams1);

CHIP_ERROR err = readClient1.SendRequest(readPrepareParams1);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

ReadPrepareParams readPrepareParams2(ctx.GetSessionBobToAlice());
// Read full wildcard paths, repeat twice to ensure chunking.
chip::app::AttributePathParams attributePathParams2[sTooLargePathCount];
readPrepareParams2.mpAttributePathParamsList = attributePathParams2;
readPrepareParams2.mAttributePathParamsListSize = ArraySize(attributePathParams2);

err = readClient2.SendRequest(readPrepareParams2);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

ctx.DrainAndServiceIO();

NL_TEST_ASSERT(apSuite, delegate1.mNumAttributeResponse != 0);
NL_TEST_ASSERT(apSuite, !delegate1.mReadError);

NL_TEST_ASSERT(apSuite, delegate2.mNumAttributeResponse == 0);
NL_TEST_ASSERT(apSuite, delegate2.mReadError);

StatusIB status(delegate2.mError);
NL_TEST_ASSERT(apSuite, status.mStatus == Protocols::InteractionModel::Status::PathsExhausted);
}

NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadClients() == 0);
NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0);
engine->SetForceHandlerQuota(false);
}

// clang-format off
const nlTest sTests[] =
{
Expand All @@ -1845,6 +2010,9 @@ const nlTest sTests[] =
NL_TEST_DEF("TestReadSubscribeAttributeResponseWithCache", TestReadInteraction::TestReadSubscribeAttributeResponseWithCache),
NL_TEST_DEF("TestReadHandler_KillOverQuotaSubscriptions", TestReadInteraction::TestReadHandler_KillOverQuotaSubscriptions),
NL_TEST_DEF("TestReadHandler_KillOldestSubscriptions", TestReadInteraction::TestReadHandler_KillOldestSubscriptions),
NL_TEST_DEF("TestReadHandler_TwoParallelReads", TestReadInteraction::TestReadHandler_TwoParallelReads),
NL_TEST_DEF("TestReadHandler_TooManyPaths", TestReadInteraction::TestReadHandler_TooManyPaths),
NL_TEST_DEF("TestReadHandler_TwoParallelReadsSecondTooManyPaths", TestReadInteraction::TestReadHandler_TwoParallelReadsSecondTooManyPaths),
NL_TEST_SENTINEL()
};
// clang-format on
Expand Down

0 comments on commit f662b71

Please sign in to comment.