From 731f4cca9cd54d31cf8189615e5b09304d352948 Mon Sep 17 00:00:00 2001 From: Yunhan Wang Date: Wed, 18 May 2022 17:25:31 -0700 Subject: [PATCH 1/3] Fix clusterStateCache dataVersion bug --if there exists any path in the request that references a specific attribute, remove affacted wildcard cluster attribute in requestPathSet used in cached so that we would not cache the dataVersion for all clusters affected by that attribute. --- src/app/AttributePathParams.h | 9 +++++ src/app/ClusterStateCache.cpp | 21 ++++++++++++ src/controller/tests/data_model/TestRead.cpp | 35 ++++++++++++++++++++ 3 files changed, 65 insertions(+) diff --git a/src/app/AttributePathParams.h b/src/app/AttributePathParams.h index 47ac890f8de3c3..effb97da71b981 100644 --- a/src/app/AttributePathParams.h +++ b/src/app/AttributePathParams.h @@ -96,6 +96,15 @@ struct AttributePathParams return true; } + bool IsAttributePathOverlapped(const AttributePathParams & other) const + { + VerifyOrReturnError(HasWildcardEndpointId() || other.HasWildcardEndpointId() || mEndpointId == other.mEndpointId, false); + VerifyOrReturnError(HasWildcardClusterId() || other.HasWildcardClusterId() || mClusterId == other.mClusterId, false); + VerifyOrReturnError(HasWildcardAttributeId() || other.HasWildcardAttributeId() || mAttributeId == other.mAttributeId, + false); + return true; + } + bool IncludesAttributesInCluster(const DataVersionFilter & other) const { VerifyOrReturnError(HasWildcardEndpointId() || mEndpointId == other.mEndpointId, false); diff --git a/src/app/ClusterStateCache.cpp b/src/app/ClusterStateCache.cpp index c99daf57e0c999..fad5c665389771 100644 --- a/src/app/ClusterStateCache.cpp +++ b/src/app/ClusterStateCache.cpp @@ -462,6 +462,27 @@ CHIP_ERROR ClusterStateCache::OnUpdateDataVersionFilterList(DataVersionFilterIBs } } + for (auto & attribute1 : aAttributePaths) + { + if (attribute1.HasWildcardAttributeId()) + { + continue; + } + + auto attribute2 = std::begin(mRequestPathSet); + while (attribute2 != std::end(mRequestPathSet)) + { + if (attribute1.IsAttributePathOverlapped(*attribute2)) + { + attribute2 = mRequestPathSet.erase(attribute2); + } + else + { + ++attribute2; + } + } + } + std::vector> filterVector; GetSortedFilters(filterVector); diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp index 85216a0dfe7460..defda1c95f9e79 100644 --- a/src/controller/tests/data_model/TestRead.cpp +++ b/src/controller/tests/data_model/TestRead.cpp @@ -401,6 +401,41 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit app::InteractionModelEngine::GetInstance()->RegisterReadHandlerAppCallback(&gTestReadInteraction); int testId = 0; + + // Read of E2C3A1(dedup), E*C3A1(E1C3A1 not exit, E2C3A1 exist), E2C3A* (5 supported attributes) + // Expect no versions would be cached. + { + testId++; + ChipLogProgress(DataManagement, "\t -- Running Read with ClusterStateCache Test ID %d", testId); + app::ReadClient readClient(chip::app::InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), + cache.GetBufferedCallback(), chip::app::ReadClient::InteractionType::Read); + chip::app::AttributePathParams attributePathParams1[3]; + attributePathParams1[0].mEndpointId = Test::kMockEndpoint2; + attributePathParams1[0].mClusterId = Test::MockClusterId(3); + attributePathParams1[0].mAttributeId = Test::MockAttributeId(1); + + attributePathParams1[1].mEndpointId = kInvalidEndpointId; + attributePathParams1[1].mClusterId = Test::MockClusterId(3); + attributePathParams1[1].mAttributeId = Test::MockAttributeId(1); + + attributePathParams1[2].mEndpointId = Test::kMockEndpoint2; + attributePathParams1[2].mClusterId = Test::MockClusterId(3); + attributePathParams1[2].mAttributeId = kInvalidAttributeId; + + readPrepareParams.mpAttributePathParamsList = attributePathParams1; + readPrepareParams.mAttributePathParamsListSize = 3; + err = readClient.SendRequest(readPrepareParams); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + ctx.DrainAndServiceIO(); + NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 6); + NL_TEST_ASSERT(apSuite, !delegate.mReadError); + Optional version1; + NL_TEST_ASSERT(apSuite, cache.GetVersion(Test::kMockEndpoint2, Test::MockClusterId(3), version1) == CHIP_NO_ERROR); + NL_TEST_ASSERT(apSuite, !version1.HasValue()); + delegate.mNumAttributeResponse = 0; + } + // Read of E2C3A1, E2C3A2 and E3C2A2. // Expect no versions would be cached. { From b958468ee20bc356018e0ebd737c6095091384df Mon Sep 17 00:00:00 2001 From: Yunhan Wang Date: Thu, 19 May 2022 20:42:31 -0700 Subject: [PATCH 2/3] address comments --- src/app/AttributePathParams.h | 2 +- src/app/ClusterStateCache.cpp | 37 ++++++++++---------- src/controller/tests/data_model/TestRead.cpp | 3 +- 3 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/app/AttributePathParams.h b/src/app/AttributePathParams.h index effb97da71b981..adfc77cfe27830 100644 --- a/src/app/AttributePathParams.h +++ b/src/app/AttributePathParams.h @@ -96,7 +96,7 @@ struct AttributePathParams return true; } - bool IsAttributePathOverlapped(const AttributePathParams & other) const + bool Intersects(const AttributePathParams & other) const { VerifyOrReturnError(HasWildcardEndpointId() || other.HasWildcardEndpointId() || mEndpointId == other.mEndpointId, false); VerifyOrReturnError(HasWildcardClusterId() || other.HasWildcardClusterId() || mClusterId == other.mClusterId, false); diff --git a/src/app/ClusterStateCache.cpp b/src/app/ClusterStateCache.cpp index fad5c665389771..748294df725adb 100644 --- a/src/app/ClusterStateCache.cpp +++ b/src/app/ClusterStateCache.cpp @@ -454,31 +454,32 @@ CHIP_ERROR ClusterStateCache::OnUpdateDataVersionFilterList(DataVersionFilterIBs CHIP_ERROR err = CHIP_NO_ERROR; TLV::TLVWriter backup; - for (auto & attribute : aAttributePaths) - { - if (attribute.HasWildcardAttributeId()) - { - mRequestPathSet.insert(attribute); - } - } - + // Only put paths into mRequestPathSet if they cover clusters in their entirety and no other path in our path list + // points to a specific attribute from any of those clusters. + // this would help for data-out-of-sync issue when handling store data version for the particular case on two paths: (E1, C1, + // wildcard), (wildcard, C1, A1) for (auto & attribute1 : aAttributePaths) { + bool intersected = false; if (attribute1.HasWildcardAttributeId()) { - continue; - } - - auto attribute2 = std::begin(mRequestPathSet); - while (attribute2 != std::end(mRequestPathSet)) - { - if (attribute1.IsAttributePathOverlapped(*attribute2)) + for (auto & attribute2 : aAttributePaths) { - attribute2 = mRequestPathSet.erase(attribute2); + if (attribute2.HasWildcardAttributeId()) + { + continue; + } + + if (attribute1.Intersects(attribute2)) + { + intersected = true; + break; + } } - else + + if (!intersected) { - ++attribute2; + mRequestPathSet.insert(attribute1); } } } diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp index defda1c95f9e79..5d93aad5e7a9f0 100644 --- a/src/controller/tests/data_model/TestRead.cpp +++ b/src/controller/tests/data_model/TestRead.cpp @@ -431,7 +431,8 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 6); NL_TEST_ASSERT(apSuite, !delegate.mReadError); Optional version1; - NL_TEST_ASSERT(apSuite, cache.GetVersion(Test::kMockEndpoint2, Test::MockClusterId(3), version1) == CHIP_NO_ERROR); + app::ConcreteClusterPath clusterPath1(Test::kMockEndpoint2, Test::MockClusterId(3)); + NL_TEST_ASSERT(apSuite, cache.GetVersion(clusterPath1, version1) == CHIP_NO_ERROR); NL_TEST_ASSERT(apSuite, !version1.HasValue()); delegate.mNumAttributeResponse = 0; } From cf60e94c6a7c6b0469c4b8e60540c91058e0b6ca Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 20 May 2022 11:01:45 -0400 Subject: [PATCH 3/3] Update src/app/ClusterStateCache.cpp Co-authored-by: Boris Zbarsky --- src/app/ClusterStateCache.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/ClusterStateCache.cpp b/src/app/ClusterStateCache.cpp index 748294df725adb..e7aaca424055fe 100644 --- a/src/app/ClusterStateCache.cpp +++ b/src/app/ClusterStateCache.cpp @@ -460,9 +460,9 @@ CHIP_ERROR ClusterStateCache::OnUpdateDataVersionFilterList(DataVersionFilterIBs // wildcard), (wildcard, C1, A1) for (auto & attribute1 : aAttributePaths) { - bool intersected = false; if (attribute1.HasWildcardAttributeId()) { + bool intersected = false; for (auto & attribute2 : aAttributePaths) { if (attribute2.HasWildcardAttributeId())