Address follow up review comments for not setting up subscription for… (

#33573) * 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 <[email protected]> * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]> * Address more comments --------- Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Oct 15, 2024 · 3106797 · 3106797
1 parent fad2b7e
commit 3106797
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 54 deletions.
diff --git 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,26 +668,29 @@ - (void)_setDSTOffsets:(NSArray<MTRTimeSynchronizationClusterDSTOffsetStruct *>
 
 - (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];
 }
 
 - (void)setDelegate:(id<MTRDeviceDelegate>)delegate queue:(dispatch_queue_t)queue
 {
     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:)]) {
         setUpSubscription = [testDelegate unitTestShouldSetUpSubscriptionForDevice:self];
     }
 #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
@@ -30,6 +30,26 @@ typedef NSDictionary<NSString *, id> * 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
@@ -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<NSDictionary<NSString *, id> *> * 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
@@ -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<NSDictionary<NSString *, id> *> *)eventReport;
 - (void)unitTestInjectAttributeReport:(NSArray<NSDictionary<NSString *, id> *> *)attributeReport fromSubscription:(BOOL)isFromSubscription;
 - (NSUInteger)unitTestAttributesReportedSinceLastCheck;