From c3ef1102030ed50904ca878faa8d711f16ba16c2 Mon Sep 17 00:00:00 2001 From: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com> Date: Wed, 22 May 2024 16:03:59 -0700 Subject: [PATCH] =?UTF-8?q?Do=20not=20set=20up=20subscription=20when=20set?= =?UTF-8?q?ting=20a=20delegate=20for=20a=20MTRDevice=20wi=E2=80=A6=20(#335?= =?UTF-8?q?59)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * Restyled by whitespace * Restyled by clang-format * Clean up * Restyled by clang-format * Add an API for checking if subscriptions are allowed and check that in _setUpSubscription * Restyled by clang-format --------- Co-authored-by: Restyled.io --- src/darwin/Framework/CHIP/MTRDevice.mm | 21 ++++++++++++- .../Framework/CHIPTests/MTRDeviceTests.m | 30 +++++++++++++++++++ .../TestHelpers/MTRTestDeclarations.h | 21 +++++++++++++ 3 files changed, 71 insertions(+), 1 deletion(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 1c0fad30ecacc4..7cb816708aac49 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -685,11 +685,17 @@ - (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("%@ setDelegate %@", self, delegate); - BOOL setUpSubscription = YES; + // We should not set up a subscription for device controllers over XPC. + BOOL setUpSubscription = [self _subscriptionsAllowed]; // For unit testing only #ifdef DEBUG @@ -933,6 +939,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 @@ -1680,6 +1694,11 @@ - (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; diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index 1c2d5ddfe39a9a..26152705613790 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -3618,6 +3618,36 @@ - (void)test034_TestMTRDeviceHistoricalEvents XCTAssertTrue(eventReportsReceived > 0); } +- (void)test035_TestMTRDeviceSubscriptionNotEstablishedOverXPC +{ + 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