Address follow up review comments for not setting up subscription for XPC controllers

Author: nivi-apple

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,16 +668,20 @@ - (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
 #ifdef DEBUG
     id testDelegate = delegate;
@@ -705,8 +690,6 @@ - (void)setDelegate:(id<MTRDeviceDelegate>)delegate queue:(dispatch_queue_t)queu
     }
 #endif
 
-    std::lock_guard lock(_lock);
-
     _weakDelegate = [MTRWeakReference weakReferenceWithObject:delegate];
     _delegateQueue = queue;
 
@@ -830,12 +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;
+    // setup. Based on whether subscriptions are allowed or not, return YES or NO.
+    return [self _subscriptionsAllowed];
 }
 
 // Notification that read-through was skipped for an attribute read.

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.
  */

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

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;