Skip to content

Commit

Permalink
[Darwin] MTRDevice in-memory cache needs to be resilient to data stor…
Browse files Browse the repository at this point in the history
…e returning nil
  • Loading branch information
jtung-apple committed Jul 19, 2024
1 parent 19a774a commit f6e0179
Show file tree
Hide file tree
Showing 4 changed files with 132 additions and 0 deletions.
40 changes: 40 additions & 0 deletions src/darwin/Framework/CHIP/MTRDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -1235,6 +1235,12 @@ - (void)_handleSubscriptionEstablished
- (void)_handleSubscriptionError:(NSError *)error
{
std::lock_guard lock(_lock);
[self _doHandleSubscriptionError:error];
}

- (void)_doHandleSubscriptionError:(NSError *)error
{
os_unfair_lock_assert_owner(&_lock);

[self _changeInternalState:MTRInternalDeviceStateUnsubscribed];
_unreportedEvents = nil;
Expand Down Expand Up @@ -1400,6 +1406,12 @@ - (void)_handleResubscriptionNeededWithDelay:(NSNumber *)resubscriptionDelayMs
- (void)_handleSubscriptionReset:(NSNumber * _Nullable)retryDelay
{
std::lock_guard lock(_lock);
[self _doHandleSubscriptionReset:retryDelay];
}

- (void)_doHandleSubscriptionReset:(NSNumber * _Nullable)retryDelay
{
os_unfair_lock_assert_owner(&_lock);

// If we are here, then either we failed to establish initial CASE, or we
// failed to send the initial SubscribeRequest message, or our ReadClient
Expand Down Expand Up @@ -2134,6 +2146,9 @@ - (nullable MTRDeviceClusterData *)_clusterDataForPath:(MTRClusterPath *)cluster
MTRDeviceClusterData * data = [_deviceController.controllerDataStore getStoredClusterDataForNodeID:_nodeID endpointID:clusterPath.endpoint clusterID:clusterPath.cluster];
if (data != nil) {
[_persistedClusterData setObject:data forKey:clusterPath];
} else {
// If clusterPath is in _persistedClusters and the data store returns nil for it, then the in-memory cache is now not dependable, and subscription should be reset and reestablished to reload cache from device
[self _resetSubscriptionWithReasonString:[NSString stringWithFormat:@"Data store has no data for cluster %@", clusterPath]];
}

return data;
Expand Down Expand Up @@ -2303,6 +2318,31 @@ - (void)_stopConnectivityMonitoring
}
}

- (void)_resetSubscriptionWithReasonString:(NSString *)reasonString
{
os_unfair_lock_assert_owner(&self->_lock);
MTR_LOG_ERROR("%@ %@ - resetting subscription", self, reasonString);

[_deviceController asyncDispatchToMatterQueue:^{
std::lock_guard lock(self->_lock);
self->_currentReadClient = nullptr;
self->_currentSubscriptionCallback = nullptr;

[self _doHandleSubscriptionError:nil];
// Use nil reset delay so that this keeps existing backoff timing
[self _doHandleSubscriptionReset:nil];
}
errorHandler:nil];
}

#ifdef DEBUG
- (void)unitTestResetSubscription
{
std::lock_guard lock(self->_lock);
[self _resetSubscriptionWithReasonString:@"Unit test reset subscription"];
}
#endif

// assume lock is held
- (void)_setupSubscriptionWithReason:(NSString *)reason
{
Expand Down
3 changes: 3 additions & 0 deletions src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm
Original file line number Diff line number Diff line change
Expand Up @@ -1011,6 +1011,7 @@ - (void)storeClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *>
BOOL endpointIndexModified = NO;
NSMutableArray<NSNumber *> * endpointIndexToStore;
if (endpointIndex) {
MTR_LOG("No entry found for endpointIndex @ node 0x%016llX - creating", nodeID.unsignedLongLongValue);
endpointIndexToStore = [endpointIndex mutableCopy];
} else {
endpointIndexToStore = [NSMutableArray array];
Expand All @@ -1029,6 +1030,7 @@ - (void)storeClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *>
BOOL clusterIndexModified = NO;
NSMutableArray<NSNumber *> * clusterIndexToStore;
if (clusterIndex) {
MTR_LOG("No entry found for clusterIndex @ node 0x%016llX endpoint %u - creating", nodeID.unsignedLongLongValue, endpointID.unsignedShortValue);
clusterIndexToStore = [clusterIndex mutableCopy];
} else {
clusterIndexToStore = [NSMutableArray array];
Expand Down Expand Up @@ -1074,6 +1076,7 @@ - (void)storeClusterData:(NSDictionary<MTRClusterPath *, MTRDeviceClusterData *>
NSArray<NSNumber *> * nodeIndexToStore = nil;
if (!nodeIndex) {
// Ensure node index exists
MTR_LOG("No entry found for for nodeIndex - creating for node 0x%016llX", nodeID.unsignedLongLongValue);
nodeIndexToStore = [NSArray arrayWithObject:nodeID];
} else if (![nodeIndex containsObject:nodeID]) {
nodeIndexToStore = [nodeIndex arrayByAddingObject:nodeID];
Expand Down
88 changes: 88 additions & 0 deletions src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -2937,4 +2937,92 @@ - (void)testDataStorageUpdatesWhenRemovingAttributes
XCTAssertFalse([controller isRunning]);
}

// Run the test here since detectLeaks is set, and subscription reset needs to not cause leaks
- (void)testMTRDeviceResetSubscription
{
__auto_type * storageDelegate = [[MTRTestPerControllerStorageWithBulkReadWrite alloc] initWithControllerID:[NSUUID UUID]];

__auto_type * factory = [MTRDeviceControllerFactory sharedInstance];
XCTAssertNotNil(factory);

__auto_type queue = dispatch_get_main_queue();

__auto_type * rootKeys = [[MTRTestKeys alloc] init];
XCTAssertNotNil(rootKeys);

__auto_type * operationalKeys = [[MTRTestKeys alloc] init];
XCTAssertNotNil(operationalKeys);

NSNumber * nodeID = @(333);
NSNumber * fabricID = @(444);

NSError * error;

MTRPerControllerStorageTestsCertificateIssuer * certificateIssuer;
MTRDeviceController * controller = [self startControllerWithRootKeys:rootKeys
operationalKeys:operationalKeys
fabricID:fabricID
nodeID:nodeID
storage:storageDelegate
error:&error
certificateIssuer:&certificateIssuer];
XCTAssertNil(error);
XCTAssertNotNil(controller);
XCTAssertTrue([controller isRunning]);

XCTAssertEqualObjects(controller.controllerNodeID, nodeID);

// Now commission the device, to test that that works.
NSNumber * deviceID = @(22);
certificateIssuer.nextNodeID = deviceID;
[self commissionWithController:controller newNodeID:deviceID];

// We should have established CASE using our operational key.
XCTAssertEqual(operationalKeys.signatureCount, 1);

__auto_type * device = [MTRDevice deviceWithNodeID:deviceID controller:controller];
__auto_type * delegate = [[MTRDeviceTestDelegate alloc] init];

XCTestExpectation * subscriptionExpectation = [self expectationWithDescription:@"Subscription has been set up"];

delegate.onReportEnd = ^{
[subscriptionExpectation fulfill];
};

[device setDelegate:delegate queue:queue];

[self waitForExpectations:@[ subscriptionExpectation ] timeout:60];

// Test that subscription reset works

XCTestExpectation * subscriptionExpectation2 = [self expectationWithDescription:@"Subscription has been set up 2"];

__weak __auto_type weakDelegate = delegate;
delegate.onReportEnd = ^{
[subscriptionExpectation2 fulfill];
// reset callback so expectation not fulfilled twice
__strong __auto_type strongDelegate = weakDelegate;
strongDelegate.onReportEnd = nil;
};

// clear cluster data before reset
NSUInteger attributeCountBeforeReset = [device unitTestAttributeCount];
[device unitTestClearClusterData];

[device unitTestResetSubscription];

[self waitForExpectations:@[ subscriptionExpectation2 ] timeout:60];

// check that in-memory cache has recovered
NSUInteger attributeCountAfterReset = [device unitTestAttributeCount];
XCTAssertEqual(attributeCountBeforeReset, attributeCountAfterReset);

// Reset our commissionee.
__auto_type * baseDevice = [MTRBaseDevice deviceWithNodeID:deviceID controller:controller];
ResetCommissionee(baseDevice, queue, self, kTimeoutInSeconds);

[controller shutdown];
XCTAssertFalse([controller isRunning]);
}

@end
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ NS_ASSUME_NONNULL_BEGIN
deviceReportingExcessivelyIntervalThreshold:(NSTimeInterval)deviceReportingExcessivelyIntervalThreshold;
- (void)unitTestSetMostRecentReportTimes:(NSMutableArray<NSDate *> *)mostRecentReportTimes;
- (NSUInteger)unitTestNonnullDelegateCount;
- (void)unitTestResetSubscription;
@end
#endif

Expand Down

0 comments on commit f6e0179

Please sign in to comment.