From 24c40ad78c3ecac210294e6d8bc72c0747ee8159 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Thu, 13 Apr 2023 09:30:25 -0700 Subject: [PATCH] Added logic to identify expected values to avoid race Fixed review comments about spelling Removed MTRPair class --- src/darwin/Framework/CHIP/MTRDevice.mm | 159 +++++++++++------- .../Framework/CHIPTests/MTRDeviceTests.m | 7 +- 2 files changed, 106 insertions(+), 60 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 95da91d66965e2..6c9230dc8c4eb9 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -48,27 +48,7 @@ // Consider moving utility classes to their own file #pragma mark - Utility Classes -@interface MTRPair : NSObject -+ (instancetype)pairWithFirst:(FirstObjectType)first second:(SecondObjectType)second; -@property (nonatomic, readonly) FirstObjectType first; -@property (nonatomic, readonly) SecondObjectType second; -@end - -@implementation MTRPair -- (instancetype)initWithFirst:(id)first second:(id)second -{ - if (self = [super init]) { - _first = first; - _second = second; - } - return self; -} -+ (instancetype)pairWithFirst:(id)first second:(id)second -{ - return [[self alloc] initWithFirst:first second:second]; -} -@end - +// This class is for storing weak references in a container @interface MTRWeakReference : NSObject + (instancetype)weakReferenceWithObject:(ObjectType)object; - (instancetype)initWithObject:(ObjectType)object; @@ -171,6 +151,12 @@ MTREventPriority MTREventPriorityForValidPriorityLevel(chip::app::PriorityLevel } // anonymous namespace #pragma mark - MTRDevice +typedef NS_ENUM(NSUInteger, MTRDeviceExpectedValueFieldIndex) { + MTRDeviceExpectedValueFieldExpirationTimeIndex = 0, + MTRDeviceExpectedValueFieldValueIndex = 1, + MTRDeviceExpectedValueFieldIDIndex = 2 +}; + @interface MTRDevice () @property (nonatomic, readonly) os_unfair_lock lock; // protects the caches and device state @property (nonatomic) chip::FabricIndex fabricIndex; @@ -200,9 +186,11 @@ @interface MTRDevice () // See MTRDeviceResponseHandler definition for value dictionary details. @property (nonatomic) NSMutableDictionary * readCache; -// Expected value cache is attributePath => MTRPair of [NSDate of expiration time, NSDictionary of value] +// Expected value cache is attributePath => NSArray of [NSDate of expiration time, NSDictionary of value, expected value ID] +// - MTRDeviceExpectedValueFieldIndex // See MTRDeviceResponseHandler definition for value dictionary details. -@property (nonatomic) NSMutableDictionary *> * expectedValueCache; +@property (nonatomic) NSMutableDictionary * expectedValueCache; +@property (nonatomic) uint64_t expectedValueNextID; @property (nonatomic) BOOL expirationCheckScheduled; @@ -719,6 +707,13 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID MTRAttributePath * attributePath = [MTRAttributePath attributePathWithEndpointID:endpointID clusterID:clusterID attributeID:attributeID]; + // Commit change into expected value cache + NSDictionary * newExpectedValueDictionary = @{ MTRAttributePathKey : attributePath, MTRDataKey : value }; + uint64_t expectedValueID; + [self setExpectedValues:@[ newExpectedValueDictionary ] + expectedValueInterval:expectedValueInterval + expectedValueID:&expectedValueID]; + MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.queue]; MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) { MTR_LOG_INFO("%@ dequeueWorkItem %@", logPrefix, self->_asyncCallbackWorkQueue); @@ -734,18 +729,13 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID MTR_LOG_INFO("%@ completion error %@ endWork", logPrefix, error); [workItem endWork]; if (error) { - [self removeExpectedValuesForAttributePath:attributePath]; + [self removeExpectedValueForAttributePath:attributePath expectedValueID:expectedValueID]; } }]; }; workItem.readyHandler = readyHandler; MTR_LOG_INFO("%@ enqueueWorkItem %@", logPrefix, _asyncCallbackWorkQueue); [_asyncCallbackWorkQueue enqueueWorkItem:workItem]; - - // Commit change into expected value cache - NSDictionary * newExpectedValueDictionary = @{ MTRAttributePathKey : attributePath, MTRDataKey : value }; - - [self setExpectedValues:@[ newExpectedValueDictionary ] expectedValueInterval:expectedValueInterval]; } - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID @@ -767,6 +757,16 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID } else { expectedValueInterval = MTRClampedNumber(expectedValueInterval, @(1), @(UINT32_MAX)); } + + uint64_t expectedValueID = 0; + NSMutableArray * attributePaths = nil; + if (expectedValues) { + [self setExpectedValues:expectedValues expectedValueInterval:expectedValueInterval expectedValueID:&expectedValueID]; + attributePaths = [NSMutableArray array]; + for (NSDictionary * expectedValue in expectedValues) { + [attributePaths addObject:expectedValue[MTRAttributePathKey]]; + } + } MTRAsyncCallbackQueueWorkItem * workItem = [[MTRAsyncCallbackQueueWorkItem alloc] initWithQueue:self.queue]; MTRAsyncCallbackReadyHandler readyHandler = ^(MTRDevice * device, NSUInteger retryCount) { MTR_LOG_INFO("%@ dequeueWorkItem %@", logPrefix, self->_asyncCallbackWorkQueue); @@ -784,15 +784,14 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID completion(values, error); }); [workItem endWork]; + if (error && expectedValues) { + [self removeExpectedValuesForAttributePaths:attributePaths expectedValueID:expectedValueID]; + } }]; }; workItem.readyHandler = readyHandler; MTR_LOG_INFO("%@ enqueueWorkItem %@", logPrefix, _asyncCallbackWorkQueue); [_asyncCallbackWorkQueue enqueueWorkItem:workItem]; - - if (expectedValues) { - [self setExpectedValues:expectedValues expectedValueInterval:expectedValueInterval]; - } } - (void)openCommissioningWindowWithSetupPasscode:(NSNumber *)setupPasscode @@ -828,14 +827,15 @@ - (void)_checkExpiredExpectedValues // find expired attributes, and calculate next timer fire date NSDate * now = [NSDate date]; NSDate * nextExpirationDate = nil; - NSMutableSet *> * attributeInfoToRemove = [NSMutableSet set]; + // Set of NSArray with 2 elements [path, value] - this is used in this method only + NSMutableSet * attributeInfoToRemove = [NSMutableSet set]; for (MTRAttributePath * attributePath in _expectedValueCache) { - MTRPair * expectedValue = _expectedValueCache[attributePath]; - NSDate * attributeExpirationDate = expectedValue.first; + NSArray * expectedValue = _expectedValueCache[attributePath]; + NSDate * attributeExpirationDate = expectedValue[MTRDeviceExpectedValueFieldExpirationTimeIndex]; if (expectedValue) { if ([now compare:attributeExpirationDate] == NSOrderedDescending) { // expired - save [path, values] pair to attributeToRemove - [attributeInfoToRemove addObject:[MTRPair pairWithFirst:attributePath second:expectedValue.second]]; + [attributeInfoToRemove addObject:@[ attributePath, expectedValue[MTRDeviceExpectedValueFieldValueIndex] ]]; } else { // get the next expiration date if (!nextExpirationDate || [nextExpirationDate compare:attributeExpirationDate] == NSOrderedDescending) { @@ -848,10 +848,10 @@ - (void)_checkExpiredExpectedValues // remove from expected value cache and report attributes as needed NSMutableArray * attributesToReport = [NSMutableArray array]; NSMutableArray * attributePathsToReport = [NSMutableArray array]; - for (MTRPair * attributeInfo in attributeInfoToRemove) { + for (NSArray * attributeInfo in attributeInfoToRemove) { // compare with known value and mark for report if different - MTRAttributePath * attributePath = attributeInfo.first; - NSDictionary * attributeDataValue = attributeInfo.second; + MTRAttributePath * attributePath = attributeInfo[0]; + NSDictionary * attributeDataValue = attributeInfo[1]; NSDictionary * cachedAttributeDataValue = _readCache[attributePath]; if (cachedAttributeDataValue && ![self _attributeDataValue:attributeDataValue isEqualToDataValue:cachedAttributeDataValue]) { @@ -898,17 +898,17 @@ - (void)_performScheduledExpirationCheck os_unfair_lock_lock(&self->_lock); // First check expected value cache - MTRPair * expectedValue = _expectedValueCache[attributePath]; + NSArray * expectedValue = _expectedValueCache[attributePath]; if (expectedValue) { NSDate * now = [NSDate date]; - if ([now compare:expectedValue.first] == NSOrderedDescending) { + if ([now compare:expectedValue[MTRDeviceExpectedValueFieldExpirationTimeIndex]] == NSOrderedDescending) { // expired - purge and fall through _expectedValueCache[attributePath] = nil; } else { os_unfair_lock_unlock(&self->_lock); // not yet expired - return result - return expectedValue.second; + return expectedValue[MTRDeviceExpectedValueFieldValueIndex]; } } @@ -965,9 +965,10 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray * expectedValue = _expectedValueCache[attributePath]; + NSArray * expectedValue = _expectedValueCache[attributePath]; if (expectedValue) { - if (![self _attributeDataValue:attributeDataValue isEqualToDataValue:expectedValue.second]) { + if (![self _attributeDataValue:attributeDataValue + isEqualToDataValue:expectedValue[MTRDeviceExpectedValueFieldValueIndex]]) { shouldReportAttribute = YES; } _expectedValueCache[attributePath] = nil; @@ -998,23 +999,30 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray *)expectedAttributeValue attributePath:(MTRAttributePath *)attributePath expirationTime:(NSDate *)expirationTime shouldReportValue:(BOOL *)shouldReportValue attributeValueToReport:(NSDictionary **)attributeValueToReport + expectedValueID:(uint64_t *)expectedValueID { + os_unfair_lock_assert_owner(&self->_lock); + *shouldReportValue = NO; - MTRPair * previousExpectedValue = _expectedValueCache[attributePath]; + NSArray * previousExpectedValue = _expectedValueCache[attributePath]; if (previousExpectedValue) { if (expectedAttributeValue - && ![self _attributeDataValue:expectedAttributeValue isEqualToDataValue:previousExpectedValue.second]) { + && ![self _attributeDataValue:expectedAttributeValue + isEqualToDataValue:previousExpectedValue[MTRDeviceExpectedValueFieldValueIndex]]) { // Case where new expected value overrides previous expected value - report new expected value *shouldReportValue = YES; *attributeValueToReport = expectedAttributeValue; } else if (!expectedAttributeValue - && ![self _attributeDataValue:previousExpectedValue.second isEqualToDataValue:_readCache[attributePath]]) { + && ![self _attributeDataValue:previousExpectedValue[MTRDeviceExpectedValueFieldValueIndex] + isEqualToDataValue:_readCache[attributePath]]) { // Case of removing expected value that is different than read cache - report read cache value *shouldReportValue = YES; *attributeValueToReport = _readCache[attributePath]; @@ -1031,15 +1039,24 @@ - (void)_setExpectedValue:(NSDictionary *)expectedAttributeValue } if (expectedAttributeValue) { - _expectedValueCache[attributePath] = [MTRPair pairWithFirst:expirationTime second:expectedAttributeValue]; - } else { - _expectedValueCache[attributePath] = nil; + uint64_t expectedValueIDToReturn = _expectedValueNextID++; + _expectedValueCache[attributePath] = @[ expirationTime, expectedAttributeValue, @(expectedValueIDToReturn) ]; + if (expectedValueID) { + *expectedValueID = expectedValueIDToReturn; + } + } else if (previousExpectedValue) { + // Remove previous expected value only if it's from the same setExpectedValues operation + NSNumber * previousExpectedValueID = previousExpectedValue[MTRDeviceExpectedValueFieldIDIndex]; + if (previousExpectedValueID.unsignedLongLongValue == *expectedValueID) { + _expectedValueCache[attributePath] = nil; + } } } // assume lock is held - (NSArray *)_getAttributesToReportWithNewExpectedValues:(NSArray *> *)expectedAttributeValues expirationTime:(NSDate *)expirationTime + expectedValueID:(uint64_t *)expectedValueID { os_unfair_lock_assert_owner(&self->_lock); @@ -1055,7 +1072,8 @@ - (NSArray *)_getAttributesToReportWithNewExpectedValues:(NSArray *> *)values expectedValueInterval:(NSNumber *)expectedValueInterval +{ + [self setExpectedValues:values expectedValueInterval:expectedValueInterval expectedValueID:nil]; +} + +- (void)setExpectedValues:(NSArray *> *)values + expectedValueInterval:(NSNumber *)expectedValueInterval + expectedValueID:(uint64_t *)expectedValueID { // since NSTimeInterval is in seconds, convert ms into seconds in double NSDate * expirationTime = [NSDate dateWithTimeIntervalSinceNow:expectedValueInterval.doubleValue / 1000]; @@ -1079,25 +1104,46 @@ - (void)setExpectedValues:(NSArray *> *)values expe os_unfair_lock_lock(&self->_lock); // _getAttributesToReportWithNewExpectedValues will log attribute paths reported - NSArray * attributesToReport = [self _getAttributesToReportWithNewExpectedValues:values expirationTime:expirationTime]; + NSArray * attributesToReport = [self _getAttributesToReportWithNewExpectedValues:values + expirationTime:expirationTime + expectedValueID:expectedValueID]; [self _reportAttributes:attributesToReport]; [self _checkExpiredExpectedValues]; os_unfair_lock_unlock(&self->_lock); } -- (void)removeExpectedValuesForAttributePath:(MTRAttributePath *)attributePath +- (void)removeExpectedValuesForAttributePaths:(NSArray *)attributePaths + expectedValueID:(uint64_t)expectedValueID { os_unfair_lock_lock(&self->_lock); + for (MTRAttributePath * attributePath in attributePaths) { + [self _removeExpectedValueForAttributePath:attributePath expectedValueID:expectedValueID]; + } + os_unfair_lock_unlock(&self->_lock); +} + +- (void)removeExpectedValueForAttributePath:(MTRAttributePath *)attributePath expectedValueID:(uint64_t)expectedValueID +{ + os_unfair_lock_lock(&self->_lock); + [self _removeExpectedValueForAttributePath:attributePath expectedValueID:expectedValueID]; + os_unfair_lock_unlock(&self->_lock); +} + +- (void)_removeExpectedValueForAttributePath:(MTRAttributePath *)attributePath expectedValueID:(uint64_t)expectedValueID +{ + os_unfair_lock_assert_owner(&self->_lock); + BOOL shouldReportValue; NSDictionary * attributeValueToReport; [self _setExpectedValue:nil attributePath:attributePath expirationTime:nil shouldReportValue:&shouldReportValue - attributeValueToReport:&attributeValueToReport]; + attributeValueToReport:&attributeValueToReport + expectedValueID:&expectedValueID]; - MTR_LOG_INFO("%@ remove expected values for path %@ should report %@", self, attributePath, shouldReportValue ? @"YES" : @"NO"); + MTR_LOG_INFO("%@ remove expected value for path %@ should report %@", self, attributePath, shouldReportValue ? @"YES" : @"NO"); if (shouldReportValue) { if (attributeValueToReport) { @@ -1106,7 +1152,6 @@ - (void)removeExpectedValuesForAttributePath:(MTRAttributePath *)attributePath [self _reportAttributes:@[ @{ MTRAttributePathKey : attributePath } ]]; } } - os_unfair_lock_unlock(&self->_lock); } - (MTRBaseDevice *)newBaseDevice diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index 15ebe24f7e1aba..80757382ba11d1 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -1405,7 +1405,7 @@ - (void)test017_TestMTRDeviceBasics // Before resubscribe, first test write failure and expected value effects NSNumber * testEndpointID = @(1); NSNumber * testClusterID = @(8); - NSNumber * testAttributeID = @(10000); + NSNumber * testAttributeID = @(10000); // choose a nonexistent attribute to cause a failure XCTestExpectation * expectedValueReportedExpectation = [self expectationWithDescription:@"Expected value reported"]; XCTestExpectation * expectedValueRemovedExpectation = [self expectationWithDescription:@"Expected value removed"]; delegate.onAttributeDataReceived = ^(NSArray *> * attributeReport) { @@ -1430,10 +1430,11 @@ - (void)test017_TestMTRDeviceBasics clusterID:testClusterID attributeID:testAttributeID value:writeValue - expectedValueInterval:@(20) + expectedValueInterval:@(20000) timedWriteTimeout:nil]; - // expected value interval is 20 but expect it get reverted immediately as the write fails + // expected value interval is 20s but expect it get reverted immediately as the write fails because it's writing to a + // nonexistent attribute [self waitForExpectations:@[ expectedValueReportedExpectation, expectedValueRemovedExpectation ] timeout:5 enforceOrder:YES]; // reset the onAttributeDataReceived to validate the following resubscribe test