Skip to content

Commit

Permalink
Added logic to identify expected values to avoid race
Browse files Browse the repository at this point in the history 
Fixed review comments about spelling
Removed MTRPair class
  • Loading branch information
@jtung-apple
jtung-apple committed Apr 13, 2023
1 parent 7f767c3 commit 24c40ad
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 60 deletions.
159 changes: 102 additions & 57 deletions src/darwin/Framework/CHIP/MTRDevice.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,27 +48,7 @@

// Consider moving utility classes to their own file
#pragma mark - Utility Classes
@interface MTRPair<FirstObjectType, SecondObjectType> : 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<ObjectType> : NSObject
+ (instancetype)weakReferenceWithObject:(ObjectType)object;
- (instancetype)initWithObject:(ObjectType)object;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -200,9 +186,11 @@ @interface MTRDevice ()
// See MTRDeviceResponseHandler definition for value dictionary details.
@property (nonatomic) NSMutableDictionary<MTRAttributePath *, NSDictionary *> * 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<MTRAttributePath *, MTRPair<NSDate *, NSDictionary *> *> * expectedValueCache;
@property (nonatomic) NSMutableDictionary<MTRAttributePath *, NSArray *> * expectedValueCache;
@property (nonatomic) uint64_t expectedValueNextID;

@property (nonatomic) BOOL expirationCheckScheduled;

Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand All @@ -767,6 +757,16 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID
} else {
expectedValueInterval = MTRClampedNumber(expectedValueInterval, @(1), @(UINT32_MAX));
}

uint64_t expectedValueID = 0;
NSMutableArray<MTRAttributePath *> * attributePaths = nil;
if (expectedValues) {
[self setExpectedValues:expectedValues expectedValueInterval:expectedValueInterval expectedValueID:&expectedValueID];
attributePaths = [NSMutableArray array];
for (NSDictionary<NSString *, id> * 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);
Expand All @@ -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
Expand Down Expand Up @@ -828,14 +827,15 @@ - (void)_checkExpiredExpectedValues
// find expired attributes, and calculate next timer fire date
NSDate * now = [NSDate date];
NSDate * nextExpirationDate = nil;
NSMutableSet<MTRPair<MTRAttributePath *, NSDictionary *> *> * attributeInfoToRemove = [NSMutableSet set];
// Set of NSArray with 2 elements [path, value] - this is used in this method only
NSMutableSet<NSArray *> * attributeInfoToRemove = [NSMutableSet set];
for (MTRAttributePath * attributePath in _expectedValueCache) {
MTRPair<NSDate *, NSDictionary *> * 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) {
Expand All @@ -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<MTRAttributePath *, NSDictionary *> * 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]) {
Expand Down Expand Up @@ -898,17 +898,17 @@ - (void)_performScheduledExpirationCheck
os_unfair_lock_lock(&self->_lock);

// First check expected value cache
MTRPair<NSDate *, NSDictionary *> * 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];
}
}

Expand Down Expand Up @@ -965,9 +965,10 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
_readCache[attributePath] = nil;
} else {
// if expected values exists, purge and update read cache
MTRPair<NSDate *, NSDictionary *> * 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;
Expand Down Expand Up @@ -998,23 +999,30 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
return attributesToReport;
}

// If value is non-nil, return new expected value ID in the expectedValueID.
// If value is nil, use the expectedValueID pointed
- (void)_setExpectedValue:(NSDictionary<NSString *, id> *)expectedAttributeValue
attributePath:(MTRAttributePath *)attributePath
expirationTime:(NSDate *)expirationTime
shouldReportValue:(BOOL *)shouldReportValue
attributeValueToReport:(NSDictionary<NSString *, id> **)attributeValueToReport
expectedValueID:(uint64_t *)expectedValueID
{
os_unfair_lock_assert_owner(&self->_lock);

*shouldReportValue = NO;

MTRPair<NSDate *, NSDictionary *> * 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];
Expand All @@ -1031,15 +1039,24 @@ - (void)_setExpectedValue:(NSDictionary<NSString *, id> *)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<NSDictionary<NSString *, id> *> *)expectedAttributeValues
expirationTime:(NSDate *)expirationTime
expectedValueID:(uint64_t *)expectedValueID
{
os_unfair_lock_assert_owner(&self->_lock);

Expand All @@ -1055,7 +1072,8 @@ - (NSArray *)_getAttributesToReportWithNewExpectedValues:(NSArray<NSDictionary<N
attributePath:attributePath
expirationTime:expirationTime
shouldReportValue:&shouldReportValue
attributeValueToReport:&attributeValueToReport];
attributeValueToReport:&attributeValueToReport
expectedValueID:expectedValueID];

if (shouldReportValue) {
[attributesToReport addObject:@{ MTRAttributePathKey : attributePath, MTRDataKey : attributeValueToReport }];
Expand All @@ -1069,6 +1087,13 @@ - (NSArray *)_getAttributesToReportWithNewExpectedValues:(NSArray<NSDictionary<N
}

- (void)setExpectedValues:(NSArray<NSDictionary<NSString *, id> *> *)values expectedValueInterval:(NSNumber *)expectedValueInterval
{
[self setExpectedValues:values expectedValueInterval:expectedValueInterval expectedValueID:nil];
}

- (void)setExpectedValues:(NSArray<NSDictionary<NSString *, id> *> *)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];
Expand All @@ -1079,25 +1104,46 @@ - (void)setExpectedValues:(NSArray<NSDictionary<NSString *, id> *> *)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<MTRAttributePath *> *)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<NSString *, id> * 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) {
Expand All @@ -1106,7 +1152,6 @@ - (void)removeExpectedValuesForAttributePath:(MTRAttributePath *)attributePath
[self _reportAttributes:@[ @{ MTRAttributePathKey : attributePath } ]];
}
}
os_unfair_lock_unlock(&self->_lock);
}

- (MTRBaseDevice *)newBaseDevice
Expand Down
7 changes: 4 additions & 3 deletions src/darwin/Framework/CHIPTests/MTRDeviceTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<NSDictionary<NSString *, id> *> * attributeReport) {
Expand All @@ -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
Expand Down

0 comments on commit 24c40ad

Please sign in to comment.