From 8ef23d3b1e0f38f288583ece15d6eeda484dddb9 Mon Sep 17 00:00:00 2001 From: Nivedita Sarkar Date: Wed, 22 May 2024 14:24:04 -0700 Subject: [PATCH 1/7] Do not set up subscription when setting a delegate for a MTRDevice with a remote controller over XPC - Add tests for testing the behavior when a MTRDevice is created with an XPC controller --- src/darwin/Framework/CHIP/MTRDevice.mm | 11 ++++++- .../Framework/CHIPTests/MTRDeviceTests.m | 31 +++++++++++++++++++ .../TestHelpers/MTRTestDeclarations.h | 21 +++++++++++++ 3 files changed, 62 insertions(+), 1 deletion(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 4fca9139b87d2c..b6e569fc3d7655 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -689,7 +689,8 @@ - (void)setDelegate:(id)delegate queue:(dispatch_queue_t)queu { MTR_LOG_INFO("%@ setDelegate %@", self, delegate); - BOOL setUpSubscription = YES; + // We should not set up a subscription for device controllers over XPC. + BOOL setUpSubscription = ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class];; // For unit testing only #ifdef DEBUG @@ -933,6 +934,14 @@ - (void)_changeInternalState:(MTRInternalDeviceState)state } } +#ifdef DEBUG +- (MTRInternalDeviceState)_getInternalState +{ + std::lock_guard lock(self->_lock); + return _internalDeviceState; +} +#endif + // First Time Sync happens 2 minutes after reachability (this can be changed in the future) #define MTR_DEVICE_TIME_UPDATE_INITIAL_WAIT_TIME_SEC (60 * 2) - (void)_handleSubscriptionEstablished diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index 1c2d5ddfe39a9a..f997a1b3080009 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -3618,6 +3618,37 @@ - (void)test034_TestMTRDeviceHistoricalEvents XCTAssertTrue(eventReportsReceived > 0); } +- (void)test035_TestMTRDeviceSubscriptionNotEstablishedOverXPC +{ + static NSString * const MTRDeviceControllerId = @"MTRController"; + __auto_type remoteController = [MTRDeviceController + sharedControllerWithID:MTRDeviceControllerId + xpcConnectBlock:^NSXPCConnection * _Nonnull { + return nil; + }]; + + __auto_type * device = [MTRDevice deviceWithNodeID:kDeviceId deviceController:remoteController]; + dispatch_queue_t queue = dispatch_get_main_queue(); + + // We should not set up a subscription when creating a MTRDevice with a remote controller. + XCTestExpectation * subscriptionExpectation = [self expectationWithDescription:@"Subscription has been set up"]; + subscriptionExpectation.inverted = YES; + + __auto_type * delegate = [[MTRDeviceTestDelegate alloc] init]; + + XCTAssertTrue([device _getInternalState] == MTRInternalDeviceStateUnsubscribed); + + delegate.onAttributeDataReceived = ^(NSArray *> * attributeReport) { + [subscriptionExpectation fulfill]; + }; + + [device setDelegate:delegate queue:queue]; + [self waitForExpectations:@[ subscriptionExpectation ] timeout:30]; + + XCTAssertTrue([device _getInternalState] == MTRInternalDeviceStateUnsubscribed); +} + + @end @interface MTRDeviceEncoderTests : XCTestCase diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h index 0cf5707c05c8b4..3c27a1fcc0102e 100644 --- a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h +++ b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h @@ -59,10 +59,31 @@ NS_ASSUME_NONNULL_BEGIN @end @interface MTRDevice (TestDebug) +typedef NS_ENUM(NSUInteger, MTRInternalDeviceState) { + // Unsubscribed means we do not have a subscription and are not trying to set one up. + MTRInternalDeviceStateUnsubscribed = 0, + // Subscribing means we are actively trying to establish our initial subscription (e.g. doing + // DNS-SD discovery, trying to establish CASE to the peer, getting priming reports, etc). + MTRInternalDeviceStateSubscribing = 1, + // InitialSubscriptionEstablished means we have at some point finished setting up a + // subscription. That subscription may have dropped since then, but if so it's the ReadClient's + // responsibility to re-establish it. + MTRInternalDeviceStateInitialSubscriptionEstablished = 2, + // Resubscribing means we had established a subscription, but then + // detected a subscription drop due to not receiving a report on time. This + // covers all the actions that happen when re-subscribing (discovery, CASE, + // getting priming reports, etc). + MTRInternalDeviceStateResubscribing = 3, + // LaterSubscriptionEstablished meant that we had a subscription drop and + // then re-created a subscription. + MTRInternalDeviceStateLaterSubscriptionEstablished = 4, +}; + - (void)unitTestInjectEventReport:(NSArray *> *)eventReport; - (void)unitTestInjectAttributeReport:(NSArray *> *)attributeReport fromSubscription:(BOOL)isFromSubscription; - (NSUInteger)unitTestAttributesReportedSinceLastCheck; - (void)unitTestClearClusterData; +- (MTRInternalDeviceState)_getInternalState; @end #endif From a9aa5306b9efd33de32ecb8a10c749b1d5cf7a47 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Wed, 22 May 2024 21:33:50 +0000 Subject: [PATCH 2/7] Restyled by whitespace --- src/darwin/Framework/CHIPTests/MTRDeviceTests.m | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index f997a1b3080009..d0a82fff36f033 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -3626,25 +3626,25 @@ - (void)test035_TestMTRDeviceSubscriptionNotEstablishedOverXPC xpcConnectBlock:^NSXPCConnection * _Nonnull { return nil; }]; - + __auto_type * device = [MTRDevice deviceWithNodeID:kDeviceId deviceController:remoteController]; dispatch_queue_t queue = dispatch_get_main_queue(); - + // We should not set up a subscription when creating a MTRDevice with a remote controller. XCTestExpectation * subscriptionExpectation = [self expectationWithDescription:@"Subscription has been set up"]; subscriptionExpectation.inverted = YES; - + __auto_type * delegate = [[MTRDeviceTestDelegate alloc] init]; - + XCTAssertTrue([device _getInternalState] == MTRInternalDeviceStateUnsubscribed); - + delegate.onAttributeDataReceived = ^(NSArray *> * attributeReport) { [subscriptionExpectation fulfill]; }; - + [device setDelegate:delegate queue:queue]; [self waitForExpectations:@[ subscriptionExpectation ] timeout:30]; - + XCTAssertTrue([device _getInternalState] == MTRInternalDeviceStateUnsubscribed); } From 5d18b880ef8e9e8815184bc739eaec94078cf4bc Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Wed, 22 May 2024 21:33:51 +0000 Subject: [PATCH 3/7] Restyled by clang-format --- src/darwin/Framework/CHIP/MTRDevice.mm | 3 ++- src/darwin/Framework/CHIPTests/MTRDeviceTests.m | 7 +++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index b6e569fc3d7655..239498de4ea6cd 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -690,7 +690,8 @@ - (void)setDelegate:(id)delegate queue:(dispatch_queue_t)queu MTR_LOG_INFO("%@ setDelegate %@", self, delegate); // We should not set up a subscription for device controllers over XPC. - BOOL setUpSubscription = ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class];; + BOOL setUpSubscription = ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class]; + ; // For unit testing only #ifdef DEBUG diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index d0a82fff36f033..645ecef786c2f3 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -3623,9 +3623,9 @@ - (void)test035_TestMTRDeviceSubscriptionNotEstablishedOverXPC static NSString * const MTRDeviceControllerId = @"MTRController"; __auto_type remoteController = [MTRDeviceController sharedControllerWithID:MTRDeviceControllerId - xpcConnectBlock:^NSXPCConnection * _Nonnull { - return nil; - }]; + xpcConnectBlock:^NSXPCConnection * _Nonnull { + return nil; + }]; __auto_type * device = [MTRDevice deviceWithNodeID:kDeviceId deviceController:remoteController]; dispatch_queue_t queue = dispatch_get_main_queue(); @@ -3648,7 +3648,6 @@ - (void)test035_TestMTRDeviceSubscriptionNotEstablishedOverXPC XCTAssertTrue([device _getInternalState] == MTRInternalDeviceStateUnsubscribed); } - @end @interface MTRDeviceEncoderTests : XCTestCase From 6c67002850f2b54eb262d524e2eab7713e1652d2 Mon Sep 17 00:00:00 2001 From: Nivedita Sarkar Date: Wed, 22 May 2024 14:36:46 -0700 Subject: [PATCH 4/7] Clean up --- src/darwin/Framework/CHIPTests/MTRDeviceTests.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index 645ecef786c2f3..622ce842e5eccd 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -3620,7 +3620,7 @@ - (void)test034_TestMTRDeviceHistoricalEvents - (void)test035_TestMTRDeviceSubscriptionNotEstablishedOverXPC { - static NSString * const MTRDeviceControllerId = @"MTRController"; + NSString * const MTRDeviceControllerId = @"MTRController"; __auto_type remoteController = [MTRDeviceController sharedControllerWithID:MTRDeviceControllerId xpcConnectBlock:^NSXPCConnection * _Nonnull { From aa96777fabc0dd797e9c068db9696bc973f71a48 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Wed, 22 May 2024 21:37:10 +0000 Subject: [PATCH 5/7] Restyled by clang-format --- src/darwin/Framework/CHIPTests/MTRDeviceTests.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index 622ce842e5eccd..26152705613790 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -3620,7 +3620,7 @@ - (void)test034_TestMTRDeviceHistoricalEvents - (void)test035_TestMTRDeviceSubscriptionNotEstablishedOverXPC { - NSString * const MTRDeviceControllerId = @"MTRController"; + NSString * const MTRDeviceControllerId = @"MTRController"; __auto_type remoteController = [MTRDeviceController sharedControllerWithID:MTRDeviceControllerId xpcConnectBlock:^NSXPCConnection * _Nonnull { From 3efd38a94012ce4b59df7d92bd8032d7c964603b Mon Sep 17 00:00:00 2001 From: Nivedita Sarkar Date: Wed, 22 May 2024 14:49:24 -0700 Subject: [PATCH 6/7] Add an API for checking if subscriptions are allowed and check that in _setUpSubscription --- src/darwin/Framework/CHIP/MTRDevice.mm | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 239498de4ea6cd..ed6f26cb5e0775 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -685,13 +685,16 @@ - (void)_setDSTOffsets:(NSArray #define MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MIN (10 * 60) // 10 minutes (for now) #define MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MAX (60 * 60) // 60 minutes +-(BOOL)_subscriptionsAllowed { + return ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class]; +} + - (void)setDelegate:(id)delegate queue:(dispatch_queue_t)queue { MTR_LOG_INFO("%@ setDelegate %@", self, delegate); // We should not set up a subscription for device controllers over XPC. - BOOL setUpSubscription = ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class]; - ; + BOOL setUpSubscription = [self _subscriptionsAllowed]; // For unit testing only #ifdef DEBUG @@ -1690,6 +1693,12 @@ - (void)_setupSubscription { os_unfair_lock_assert_owner(&self->_lock); + if (![self _subscriptionsAllowed]) + { + MTR_LOG("_setupSubscription: Subscriptions not allowed. Do not set up subscription"); + return; + } + #ifdef DEBUG id delegate = _weakDelegate.strongObject; Optional maxIntervalOverride; From 508ea6de5e1116f4ee768cdc700e1b5d3d62629a Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Wed, 22 May 2024 22:01:20 +0000 Subject: [PATCH 7/7] Restyled by clang-format --- src/darwin/Framework/CHIP/MTRDevice.mm | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index ed6f26cb5e0775..f93efe47609ece 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -685,7 +685,8 @@ - (void)_setDSTOffsets:(NSArray #define MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MIN (10 * 60) // 10 minutes (for now) #define MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MAX (60 * 60) // 60 minutes --(BOOL)_subscriptionsAllowed { +- (BOOL)_subscriptionsAllowed +{ return ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class]; } @@ -1693,8 +1694,7 @@ - (void)_setupSubscription { os_unfair_lock_assert_owner(&self->_lock); - if (![self _subscriptionsAllowed]) - { + if (![self _subscriptionsAllowed]) { MTR_LOG("_setupSubscription: Subscriptions not allowed. Do not set up subscription"); return; }