From 3106797971377f4effc5ef9af90272ef17680c42 Mon Sep 17 00:00:00 2001 From: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com> Date: Thu, 23 May 2024 14:40:55 -0700 Subject: [PATCH] =?UTF-8?q?Address=20follow=20up=20review=20comments=20for?= =?UTF-8?q?=20not=20setting=20up=20subscription=20for=E2=80=A6=20(#33573)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Address follow up review comments for not setting up subscription for XPC controllers * Restyled by whitespace * Apply suggestions from code review Co-authored-by: Boris Zbarsky * Apply suggestions from code review Co-authored-by: Boris Zbarsky * Address more comments --------- Co-authored-by: Restyled.io Co-authored-by: Boris Zbarsky --- src/darwin/Framework/CHIP/MTRDevice.mm | 39 +++++-------------- .../Framework/CHIP/MTRDevice_Internal.h | 20 ++++++++++ .../Framework/CHIPTests/MTRDeviceTests.m | 7 ++-- .../TestHelpers/MTRTestDeclarations.h | 20 ---------- 4 files changed, 32 insertions(+), 54 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 7cb816708aac49..01f56756942b16 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -141,25 +141,6 @@ - (id)strongObject } // anonymous namespace #pragma mark - MTRDevice -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, -}; // Utility methods for working with MTRInternalDeviceState, located near the // enum so it's easier to notice that they need to stay in sync. @@ -687,6 +668,9 @@ - (void)_setDSTOffsets:(NSArray - (BOOL)_subscriptionsAllowed { + os_unfair_lock_assert_owner(&self->_lock); + + // We should not allow a subscription for device controllers over XPC. return ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class]; } @@ -694,10 +678,12 @@ - (void)setDelegate:(id)delegate queue:(dispatch_queue_t)queu { MTR_LOG("%@ setDelegate %@", self, delegate); - // We should not set up a subscription for device controllers over XPC. + std::lock_guard lock(_lock); + BOOL setUpSubscription = [self _subscriptionsAllowed]; - // For unit testing only + // For unit testing only. If this ever changes to not being for unit testing purposes, + // we would need to move the code outside of where we acquire the lock above. #ifdef DEBUG id testDelegate = delegate; if ([testDelegate respondsToSelector:@selector(unitTestShouldSetUpSubscriptionForDevice:)]) { @@ -705,8 +691,6 @@ - (void)setDelegate:(id)delegate queue:(dispatch_queue_t)queu } #endif - std::lock_guard lock(_lock); - _weakDelegate = [MTRWeakReference weakReferenceWithObject:delegate]; _delegateQueue = queue; @@ -829,13 +813,8 @@ - (BOOL)_subscriptionAbleToReport } #endif - // Unfortunately, we currently have no subscriptions over our hacked-up XPC - // setup. Try to detect that situation. - if ([_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class]) { - return NO; - } - - return YES; + // Subscriptions are not able to report if they are not allowed. + return [self _subscriptionsAllowed]; } // Notification that read-through was skipped for an attribute read. diff --git a/src/darwin/Framework/CHIP/MTRDevice_Internal.h b/src/darwin/Framework/CHIP/MTRDevice_Internal.h index 04313a6d555db2..f5383f301fe4af 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDevice_Internal.h @@ -30,6 +30,26 @@ typedef NSDictionary * MTRDeviceDataValueDictionary; typedef void (^MTRDevicePerformAsyncBlock)(MTRBaseDevice * baseDevice); +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, +}; + /** * Information about a cluster: data version and known attribute values. */ diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index 26152705613790..38b31569831d48 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -3630,22 +3630,21 @@ - (void)test035_TestMTRDeviceSubscriptionNotEstablishedOverXPC __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); + XCTAssertEqual([device _getInternalState], MTRInternalDeviceStateUnsubscribed); delegate.onAttributeDataReceived = ^(NSArray *> * attributeReport) { [subscriptionExpectation fulfill]; }; [device setDelegate:delegate queue:queue]; - [self waitForExpectations:@[ subscriptionExpectation ] timeout:30]; + [self waitForExpectations:@[ subscriptionExpectation ] timeout:5]; - XCTAssertTrue([device _getInternalState] == MTRInternalDeviceStateUnsubscribed); + XCTAssertEqual([device _getInternalState], MTRInternalDeviceStateUnsubscribed); } @end diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h index 3c27a1fcc0102e..c8174fd97b1b3f 100644 --- a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h +++ b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h @@ -59,26 +59,6 @@ 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;