From 0740e59729b61d57e3e333aef55409252706cf57 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Fri, 16 May 2025 17:03:40 -0700 Subject: [PATCH 1/2] [Darwin] Unit test for #39003 --- .../Framework/CHIP/MTRDevice_Concrete.mm | 26 +++++++----- .../Framework/CHIPTests/MTRDeviceTests.m | 41 +++++++++++++++++++ .../TestHelpers/MTRDeviceTestDelegate.h | 1 + .../TestHelpers/MTRDeviceTestDelegate.m | 4 ++ .../TestHelpers/MTRTestDeclarations.h | 2 + 5 files changed, 64 insertions(+), 10 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm index f9c06a66ef2ebc..a99f5cfd5faaba 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm @@ -660,6 +660,13 @@ - (void)_notifyDelegateOfPrivateInternalPropertiesChanges }]; } +#ifdef DEBUG +- (void)unitTestSyncRunOnDeviceQueue:(dispatch_block_t)block +{ + dispatch_sync(self.queue, block); +} +#endif + #pragma mark - Time Synchronization - (void)_setTimeOnDevice @@ -1071,8 +1078,6 @@ - (void)_triggerResubscribeWithReason:(NSString *)reason nodeLikelyReachable:(BO } os_unfair_lock_unlock(&self->_lock); - BOOL resubscribeScheduled = NO; - if (readClientToResubscribe) { if (nodeLikelyReachable) { // If we have reason to suspect the node is now reachable, reset the @@ -1081,7 +1086,14 @@ - (void)_triggerResubscribeWithReason:(NSString *)reason nodeLikelyReachable:(BO // here (e.g. still booting up), but should try again reasonably quickly. subscriptionCallback->ResetResubscriptionBackoff(); } - resubscribeScheduled = readClientToResubscribe->TriggerResubscribeIfScheduled(reason.UTF8String); + + BOOL resubscribeScheduled = readClientToResubscribe->TriggerResubscribeIfScheduled(reason.UTF8String); + + // In the case no resubscribe was actually scheduled, remove this device from the subscription pool + if (!resubscribeScheduled) { + std::lock_guard lock(_lock); + [self _clearSubscriptionPoolWork]; + } } else if (((_internalDeviceState == MTRInternalDeviceStateSubscribing && !self.doingCASEAttemptForDeviceMayBeReachable) || shouldReattemptSubscription) && nodeLikelyReachable) { // If we have reason to suspect that the node is now reachable and we haven't established a // CASE session yet, let's consider it to be stalled and invalidate the pairing session. @@ -1106,13 +1118,7 @@ - (void)_triggerResubscribeWithReason:(NSString *)reason nodeLikelyReachable:(BO // and should be called after the above ReleaseSession call, to avoid churn. if (shouldReattemptSubscription) { std::lock_guard lock(_lock); - resubscribeScheduled = [self _reattemptSubscriptionNowIfNeededWithReason:reason]; - } - - // In the case no resubscribe was actually scheduled, remove this device from the subscription pool - if (!resubscribeScheduled) { - std::lock_guard lock(_lock); - [self _clearSubscriptionPoolWork]; + [self _reattemptSubscriptionNowIfNeededWithReason:reason]; } } diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index a3f9ed81a789c2..e6a43505c77c90 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -6180,6 +6180,47 @@ - (void)test047_DeviceMaybeReachableSetsUnreachable delegate.onReachable = nil; } +- (void)test048_MTRDeviceResubscribeOnSubscriptionPool +{ + __auto_type * device = [MTRDevice deviceWithNodeID:kDeviceId1 deviceController:sController]; + // dispatch_queue_t queue = dispatch_get_main_queue(); + dispatch_queue_t queue = dispatch_queue_create("subscription-pool-queue", DISPATCH_QUEUE_SERIAL); + + __auto_type * delegate = [[MTRDeviceTestDelegate alloc] init]; + delegate.pretendThreadEnabled = YES; + delegate.subscriptionMaxIntervalOverride = @(20); // larger than kTimeoutInSeconds so empty reports do not clear subscription pool sooner + + XCTestExpectation * subscriptionExpectation = [self expectationWithDescription:@"Subscription work completed"]; + delegate.onReportEnd = ^{ + [subscriptionExpectation fulfill]; + }; + + [device setDelegate:delegate queue:queue]; + + [self waitForExpectations:@[ subscriptionExpectation ] timeout:60]; + + // Wait for subscription report stuff to clear and _handleSubscriptionEstablished asynced to device queue + [sController syncRunOnWorkQueue:^{ + ; + } error:nil]; + + // Wait for _handleSubscriptionEstablished to finish removing subscription work from pool + [device unitTestSyncRunOnDeviceQueue:^{ + ; + }]; + + // Now we can set up waiting for onSubscriptionPoolWorkComplete from the test + XCTestExpectation * subscriptionPoolWorkCompleteForTriggerTestExpectation = [self expectationWithDescription:@"_triggerResubscribeWithReason work completed"]; + delegate.onSubscriptionPoolWorkComplete = ^{ + [subscriptionPoolWorkCompleteForTriggerTestExpectation fulfill]; + }; + + // Now that subscription is established and live, ReadClient->mIsResubscriptionScheduled should be false, and _handleResubscriptionNeededWithDelayOnDeviceQueue can simulate the code path that leads to ReadClient->TriggerResubscribeIfScheduled() returning false, and exercise the edge case + [device _handleResubscriptionNeededWithDelayOnDeviceQueue:@(0)]; + + [self waitForExpectations:@[ subscriptionPoolWorkCompleteForTriggerTestExpectation ] timeout:kTimeoutInSeconds]; +} + @end @interface MTRDeviceEncoderTests : XCTestCase diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRDeviceTestDelegate.h b/src/darwin/Framework/CHIPTests/TestHelpers/MTRDeviceTestDelegate.h index f1eb607e0e2dc4..502e590118056f 100644 --- a/src/darwin/Framework/CHIPTests/TestHelpers/MTRDeviceTestDelegate.h +++ b/src/darwin/Framework/CHIPTests/TestHelpers/MTRDeviceTestDelegate.h @@ -39,6 +39,7 @@ typedef void (^MTRDeviceTestDelegateDataHandler)(NSArray *)arrayOfNumbersFromAttributeValue:(MTRDeviceDataValueDictionary)dataDictionary; - (void)setStorageBehaviorConfiguration:(MTRDeviceStorageBehaviorConfiguration *)storageBehaviorConfiguration; - (void)_deviceMayBeReachable; +- (void)_handleResubscriptionNeededWithDelayOnDeviceQueue:(NSNumber *)resubscriptionDelayMs; @property (nonatomic, readonly, nullable) NSNumber * highestObservedEventNumber; @property (nonatomic, readonly) MTRAsyncWorkQueue * asyncWorkQueue; @@ -101,6 +102,7 @@ NS_ASSUME_NONNULL_BEGIN - (NSSet *)unitTestGetPersistedClusters; - (BOOL)unitTestClusterHasBeenPersisted:(MTRClusterPath *)path; - (NSUInteger)unitTestAttributeCount; +- (void)unitTestSyncRunOnDeviceQueue:(dispatch_block_t)block; @end #endif From d568ca88141d972e6a1b85116a5ae71a464ace4e Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Fri, 16 May 2025 18:46:16 -0700 Subject: [PATCH 2/2] Update src/darwin/Framework/CHIPTests/MTRDeviceTests.m Co-authored-by: Kiel Oleson --- src/darwin/Framework/CHIPTests/MTRDeviceTests.m | 1 - 1 file changed, 1 deletion(-) diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index e6a43505c77c90..e1fbb6529f1677 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -6183,7 +6183,6 @@ - (void)test047_DeviceMaybeReachableSetsUnreachable - (void)test048_MTRDeviceResubscribeOnSubscriptionPool { __auto_type * device = [MTRDevice deviceWithNodeID:kDeviceId1 deviceController:sController]; - // dispatch_queue_t queue = dispatch_get_main_queue(); dispatch_queue_t queue = dispatch_queue_create("subscription-pool-queue", DISPATCH_QUEUE_SERIAL); __auto_type * delegate = [[MTRDeviceTestDelegate alloc] init];