From 07f755fdc76249ea229bfd7ab9491cc57cf9e1f2 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Thu, 12 Dec 2024 16:10:13 -0800 Subject: [PATCH] [Darwin] Unstored attributes should be flushed to storage on shutdown (#36791) * [Darwin] Unstored attributes should be flushed to storage on shutdown * Added unit test * Restyled fix * Fixed unit test issue * Update src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h Co-authored-by: Boris Zbarsky * Restyled by whitespace * More unit test fix * Restyled fix * Fix typo from previous suggestion. * Clarify comment for flushing write operations --------- Co-authored-by: Justin Wood Co-authored-by: Boris Zbarsky Co-authored-by: Restyled.io --- .../CHIP/MTRDeviceControllerDataStore.h | 10 ++ .../CHIP/MTRDeviceControllerDataStore.mm | 9 ++ .../CHIP/MTRDeviceController_Concrete.mm | 8 ++ .../Framework/CHIP/MTRDevice_Concrete.mm | 3 + .../CHIPTests/MTRPerControllerStorageTests.m | 123 ++++++++++++++++++ .../TestHelpers/MTRTestPerControllerStorage.h | 3 + .../TestHelpers/MTRTestPerControllerStorage.m | 85 +++++++----- 7 files changed, 207 insertions(+), 34 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h index 508092b40fa613..943cd4c2cfb33c 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h @@ -93,6 +93,16 @@ typedef void (^MTRDeviceControllerDataStoreClusterDataHandler)(NSDictionary *)getStoredDeviceDataForNodeID:(NSNumber *)nodeID; - (void)storeDeviceData:(NSDictionary *)data forNodeID:(NSNumber *)nodeID; +/** + * Mechanism for an API client to perform a block after previous async operations (writes) on the storage queue have executed. + * + * This should be used only when something really needs to wait for the asynchronous writes + * to complete and can't proceed until they have. + * + * If no block is passed in, then the method returns after having synchronously flushed the queue. + */ +- (void)synchronouslyPerformBlock:(void (^_Nullable)(void))block; + @end NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm index 8d57a5b0a1927d..a874e26f44f428 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm @@ -1169,6 +1169,15 @@ - (void)storeDeviceData:(NSDictionary *)data forNodeID:(NSNumber }); } +- (void)synchronouslyPerformBlock:(void (^_Nullable)(void))block +{ + dispatch_sync(_storageDelegateQueue, ^{ + if (block) { + block(); + } + }); +} + @end @implementation MTRCASESessionResumptionInfo diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm index 99398867ccfa68..b27844425baa50 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm @@ -449,6 +449,14 @@ - (void)cleanupAfterStartup for (MTRDevice * device in devices) { [device invalidate]; } + + // Since MTRDevice invalidate may issue asynchronous writes to storage, perform a + // block synchronously on the storage delegate queue so the async write operations + // get to run, in case the API client tears down the storage backend afterwards. + [self.controllerDataStore synchronouslyPerformBlock:^{ + MTR_LOG("%@ Finished flushing data write operations", self); + }]; + [self stopBrowseForCommissionables]; [_factory controllerShuttingDown:self]; diff --git a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm index 1d0a113553666a..996e41ff12cf2d 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm @@ -850,6 +850,9 @@ - (void)invalidate os_unfair_lock_lock(&self->_lock); + // Flush unstored attributes if any + [self _persistClusterData]; + _state = MTRDeviceStateUnknown; // Make sure we don't try to resubscribe if we have a pending resubscribe diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m index 8270fe8593c98b..8a4b87ccb0c0a0 100644 --- a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m +++ b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m @@ -1692,6 +1692,7 @@ - (void)doDataStoreMTRDeviceTestWithStorageDelegate:(id *> * attributeReport = @[ @{ + MTRAttributePathKey : [MTRAttributePath attributePathWithEndpointID:@(0) clusterID:@(1) attributeID:@(1)], + MTRDataKey : @ { + MTRDataVersionKey : @(1), + MTRTypeKey : MTRUnsignedIntegerValueType, + MTRValueKey : @(1), + } + } ]; + + // Inject first report as priming report, which gets persisted immediately + [device unitTestInjectAttributeReport:attributeReport fromSubscription:YES]; + + // No additional entries immediately after injected report + XCTAssertEqual(storageDelegate.count, baseStorageKeyCount); + + sleep(1); + + // Verify priming report persisted before hitting storage delay + XCTAssertGreaterThan(storageDelegate.count, baseStorageKeyCount); + // Now set the base count to the after-priming number + baseStorageKeyCount = storageDelegate.count; + + NSArray *> * attributeReport2 = @[ @{ + MTRAttributePathKey : [MTRAttributePath attributePathWithEndpointID:@(0) clusterID:@(2) attributeID:@(2)], + MTRDataKey : @ { + MTRDataVersionKey : @(2), + MTRTypeKey : MTRUnsignedIntegerValueType, + MTRValueKey : @(2), + } + } ]; + + // Inject second report with different cluster + [device unitTestInjectAttributeReport:attributeReport2 fromSubscription:YES]; + + sleep(1); + + // No additional entries a second after report - under storage delay + XCTAssertEqual(storageDelegate.count, baseStorageKeyCount); + + // Immediately shut down controller and force flush to storage + [controller shutdown]; + XCTAssertFalse([controller isRunning]); + + // Make sure there are more than base count entries + XCTAssertGreaterThan(storageDelegate.count, baseStorageKeyCount); + + // Now restart controller to decommission the device + controller = [self startControllerWithRootKeys:rootKeys + operationalKeys:operationalKeys + fabricID:fabricID + nodeID:nodeID + storage:storageDelegate + error:&error + certificateIssuer:&certificateIssuer + storageBehaviorConfiguration:storageBehaviorConfiguration]; + XCTAssertNil(error); + XCTAssertNotNil(controller); + XCTAssertTrue([controller isRunning]); + + XCTAssertEqualObjects(controller.controllerNodeID, nodeID); + + // Reset our commissionee. + __auto_type * baseDevice = [MTRBaseDevice deviceWithNodeID:deviceID controller:controller]; + ResetCommissionee(baseDevice, queue, self, kTimeoutInSeconds); + + [controller shutdown]; +} + // TODO: This might want to go in a separate test file, with some shared setup // across multiple tests, maybe. Would need to factor out // startControllerWithRootKeys into a test helper. diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestPerControllerStorage.h b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestPerControllerStorage.h index b02bff9c01a46b..cc9b30258528c5 100644 --- a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestPerControllerStorage.h +++ b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestPerControllerStorage.h @@ -38,6 +38,9 @@ NS_ASSUME_NONNULL_BEGIN removeValueForKey:(NSString *)key securityLevel:(MTRStorageSecurityLevel)securityLevel sharingType:(MTRStorageSharingType)sharingType; + +// For testing - direct access to the current count of keys in storage +@property (nonatomic, readonly) NSUInteger count; @end @interface MTRTestPerControllerStorageWithBulkReadWrite : MTRTestPerControllerStorage diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestPerControllerStorage.m b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestPerControllerStorage.m index f0bce87bc47af1..76ce8f2ac30500 100644 --- a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestPerControllerStorage.m +++ b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestPerControllerStorage.m @@ -40,19 +40,21 @@ - (instancetype)initWithControllerID:(NSUUID *)controllerID securityLevel:(MTRStorageSecurityLevel)securityLevel sharingType:(MTRStorageSharingType)sharingType { - XCTAssertEqualObjects(_controllerID, controller.uniqueIdentifier); + @synchronized(self) { + XCTAssertEqualObjects(_controllerID, controller.uniqueIdentifier); - __auto_type * data = self.storage[key]; - if (data == nil) { - return data; - } + __auto_type * data = self.storage[key]; + if (data == nil) { + return data; + } - NSError * error; - id value = [NSKeyedUnarchiver unarchivedObjectOfClasses:MTRDeviceControllerStorageClasses() fromData:data error:&error]; - XCTAssertNil(error); - XCTAssertNotNil(data); + NSError * error; + id value = [NSKeyedUnarchiver unarchivedObjectOfClasses:MTRDeviceControllerStorageClasses() fromData:data error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(data); - return value; + return value; + } } - (BOOL)controller:(MTRDeviceController *)controller @@ -61,15 +63,17 @@ - (BOOL)controller:(MTRDeviceController *)controller securityLevel:(MTRStorageSecurityLevel)securityLevel sharingType:(MTRStorageSharingType)sharingType { - XCTAssertEqualObjects(_controllerID, controller.uniqueIdentifier); + @synchronized(self) { + XCTAssertEqualObjects(_controllerID, controller.uniqueIdentifier); - NSError * error; - NSData * data = [NSKeyedArchiver archivedDataWithRootObject:value requiringSecureCoding:YES error:&error]; - XCTAssertNil(error); - XCTAssertNotNil(data); + NSError * error; + NSData * data = [NSKeyedArchiver archivedDataWithRootObject:value requiringSecureCoding:YES error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(data); - self.storage[key] = data; - return YES; + self.storage[key] = data; + return YES; + } } - (BOOL)controller:(MTRDeviceController *)controller @@ -77,9 +81,18 @@ - (BOOL)controller:(MTRDeviceController *)controller securityLevel:(MTRStorageSecurityLevel)securityLevel sharingType:(MTRStorageSharingType)sharingType { - XCTAssertEqualObjects(_controllerID, controller.uniqueIdentifier); - self.storage[key] = nil; - return YES; + @synchronized(self) { + XCTAssertEqualObjects(_controllerID, controller.uniqueIdentifier); + self.storage[key] = nil; + return YES; + } +} + +- (NSUInteger)count +{ + @synchronized(self) { + return self.storage.count; + } } @end @@ -88,29 +101,33 @@ @implementation MTRTestPerControllerStorageWithBulkReadWrite - (NSDictionary> *)valuesForController:(MTRDeviceController *)controller securityLevel:(MTRStorageSecurityLevel)securityLevel sharingType:(MTRStorageSharingType)sharingType { - XCTAssertEqualObjects(self.controllerID, controller.uniqueIdentifier); + @synchronized(self) { + XCTAssertEqualObjects(self.controllerID, controller.uniqueIdentifier); - if (!self.storage.count) { - return nil; - } + if (!self.storage.count) { + return nil; + } - NSMutableDictionary * valuesToReturn = [NSMutableDictionary dictionary]; - for (NSString * key in self.storage) { - valuesToReturn[key] = [self controller:controller valueForKey:key securityLevel:securityLevel sharingType:sharingType]; - } + NSMutableDictionary * valuesToReturn = [NSMutableDictionary dictionary]; + for (NSString * key in self.storage) { + valuesToReturn[key] = [self controller:controller valueForKey:key securityLevel:securityLevel sharingType:sharingType]; + } - return valuesToReturn; + return valuesToReturn; + } } - (BOOL)controller:(MTRDeviceController *)controller storeValues:(NSDictionary> *)values securityLevel:(MTRStorageSecurityLevel)securityLevel sharingType:(MTRStorageSharingType)sharingType { - XCTAssertEqualObjects(self.controllerID, controller.uniqueIdentifier); + @synchronized(self) { + XCTAssertEqualObjects(self.controllerID, controller.uniqueIdentifier); - for (NSString * key in values) { - [self controller:controller storeValue:values[key] forKey:key securityLevel:securityLevel sharingType:sharingType]; - } + for (NSString * key in values) { + [self controller:controller storeValue:values[key] forKey:key securityLevel:securityLevel sharingType:sharingType]; + } - return YES; + return YES; + } } @end