From cbf92b7eb8ee199a8b41f1bc86bdb931c6fec85c Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Thu, 5 Dec 2024 17:19:33 -0800 Subject: [PATCH] [Darwin] MTRDevice can get stuck using subscription pool if not reset properly (#36741) * [Darwin] MTRDevice can get stuck using subscription pool if not reset properly * Restyled --- .../Framework/CHIP/MTRDevice_Concrete.mm | 22 ++++- .../CHIPTests/MTRPerControllerStorageTests.m | 93 ++++++++++++++++--- 2 files changed, 98 insertions(+), 17 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm index 0fa798e8fa3d79..4eca852a692e49 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm @@ -821,7 +821,12 @@ - (void)_ensureSubscriptionForExistingDelegates:(NSString *)reason [self->_deviceController asyncDispatchToMatterQueue:^{ std::lock_guard lock(self->_lock); [self _setupSubscriptionWithReason:[NSString stringWithFormat:@"%@ and scheduled subscription is happening", reason]]; - } errorHandler:nil]; + } errorHandler:^(NSError * _Nonnull error) { + // If controller is not running, clear work item from the subscription queue + MTR_LOG_ERROR("%@ could not dispatch to matter queue for resubscription - error %@", self, error); + std::lock_guard lock(self->_lock); + [self _clearSubscriptionPoolWork]; + }]; } inNanoseconds:0 description:@"MTRDevice setDelegate first subscription"]; } else { [_deviceController asyncDispatchToMatterQueue:^{ @@ -850,6 +855,10 @@ - (void)invalidate // attempt, since we now have no delegate. _reattemptingSubscription = NO; + // Clear subscription pool work item if it's in progress, to avoid forever + // taking up a slot in the controller's work queue. + [self _clearSubscriptionPoolWork]; + [_deviceController asyncDispatchToMatterQueue:^{ MTR_LOG("%@ invalidate disconnecting ReadClient and SubscriptionCallback", self); @@ -1380,6 +1389,7 @@ - (void)_doHandleSubscriptionReset:(NSNumber * _Nullable)retryDelay if (self.suspended) { MTR_LOG("%@ ignoring expected subscription reset on controller suspend", self); + [self _clearSubscriptionPoolWork]; return; } @@ -1397,6 +1407,7 @@ - (void)_doHandleSubscriptionReset:(NSNumber * _Nullable)retryDelay if (![self _delegateExists]) { // NOTE: Do not log anything here: we have been invalidated, and the // Matter stack might already be torn down. + [self _clearSubscriptionPoolWork]; return; } @@ -1445,7 +1456,12 @@ - (void)_doHandleSubscriptionReset:(NSNumber * _Nullable)retryDelay std::lock_guard lock(self->_lock); [self _reattemptSubscriptionNowIfNeededWithReason:@"got subscription reset"]; } - errorHandler:nil]; + errorHandler:^(NSError * _Nonnull error) { + // If controller is not running, clear work item from the subscription queue + MTR_LOG_ERROR("%@ could not dispatch to matter queue for resubscription - error %@", self, error); + std::lock_guard lock(self->_lock); + [self _clearSubscriptionPoolWork]; + }]; }; int64_t resubscriptionDelayNs = static_cast(secondsToWait * NSEC_PER_SEC); @@ -2456,6 +2472,7 @@ - (void)_setupSubscriptionWithReason:(NSString *)reason if (![self _subscriptionsAllowed]) { MTR_LOG("%@ _setupSubscription: Subscriptions not allowed. Do not set up subscription (reason: %@)", self, reason); + [self _clearSubscriptionPoolWork]; return; } @@ -2475,6 +2492,7 @@ - (void)_setupSubscriptionWithReason:(NSString *)reason // for now just subscribe once if (!NeedToStartSubscriptionSetup(_internalDeviceState)) { MTR_LOG("%@ setupSubscription: no need to subscribe due to internal state %lu (reason: %@)", self, static_cast(_internalDeviceState), reason); + [self _clearSubscriptionPoolWork]; return; } diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m index 3d91a64712e32f..8270fe8593c98b 100644 --- a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m +++ b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m @@ -2641,7 +2641,7 @@ - (void)doTestSubscriptionPoolWithSize:(NSInteger)subscriptionPoolSize deviceOnb XCTAssertEqualObjects(controller.controllerNodeID, nodeID); - NSArray * orderedDeviceIDs = @[ @(101), @(102), @(103), @(104), @(105) ]; + NSArray * orderedDeviceIDs = [deviceOnboardingPayloads allKeys]; // Commission 5 devices for (NSNumber * deviceID in orderedDeviceIDs) { @@ -2669,21 +2669,16 @@ - (void)doTestSubscriptionPoolWithSize:(NSInteger)subscriptionPoolSize deviceOnb // Set up expectations and delegates - NSDictionary * subscriptionExpectations = @{ - @(101) : [self expectationWithDescription:@"Subscription 1 has been set up"], - @(102) : [self expectationWithDescription:@"Subscription 2 has been set up"], - @(103) : [self expectationWithDescription:@"Subscription 3 has been set up"], - @(104) : [self expectationWithDescription:@"Subscription 4 has been set up"], - @(105) : [self expectationWithDescription:@"Subscription 5 has been set up"], - }; + NSMutableDictionary * subscriptionExpectations = [NSMutableDictionary dictionary]; + for (NSNumber * deviceID in orderedDeviceIDs) { + NSString * expectationDescription = [NSString stringWithFormat:@"Subscription 1 has been set up %@", deviceID]; + subscriptionExpectations[deviceID] = [self expectationWithDescription:expectationDescription]; + } - NSDictionary * deviceDelegates = @{ - @(101) : [[MTRDeviceTestDelegate alloc] init], - @(102) : [[MTRDeviceTestDelegate alloc] init], - @(103) : [[MTRDeviceTestDelegate alloc] init], - @(104) : [[MTRDeviceTestDelegate alloc] init], - @(105) : [[MTRDeviceTestDelegate alloc] init], - }; + NSMutableDictionary * deviceDelegates = [NSMutableDictionary dictionary]; + for (NSNumber * deviceID in orderedDeviceIDs) { + deviceDelegates[deviceID] = [[MTRDeviceTestDelegate alloc] init]; + } // Test with counters __block os_unfair_lock counterLock = OS_UNFAIR_LOCK_INIT; @@ -2786,6 +2781,74 @@ - (void)testSubscriptionPool [self doTestSubscriptionPoolWithSize:2 deviceOnboardingPayloads:deviceOnboardingPayloads]; } +- (void)testSubscriptionPoolManyDevices +{ + // QRCodes generated for discriminators 1111~1150 and passcodes 1001~1050 + NSDictionary * deviceOnboardingPayloads = @{ + @(101) : @"MT:00000I9K17U0D900000", + @(102) : @"MT:0000000000BED900000", + @(103) : @"MT:000008C801TRD900000", + @(104) : @"MT:00000GOG0293E900000", + @(105) : @"MT:00000O-O03RGE900000", + @(106) : @"MT:00000WAX047UE900000", + @(107) : @"MT:000002N315P5F900000", + @(108) : @"MT:00000AZB165JF900000", + @(109) : @"MT:00000I9K17NWF900000", + @(110) : @"MT:000000000048G900000", + @(111) : @"MT:000008C801MLG900000", + @(112) : @"MT:00000GOG022ZG900000", + @(113) : @"MT:00000O-O03KAH900000", + @(114) : @"MT:00000WAX040OH900000", + @(115) : @"MT:000002N315I.H900000", + @(116) : @"MT:00000AZB16-CI900000", + @(117) : @"MT:00000I9K17GQI900000", + @(118) : @"MT:0000000000Z1J900000", + @(119) : @"MT:000008C801FFJ900000", + @(120) : @"MT:00000GOG02XSJ900000", + @(121) : @"MT:00000O-O03D4K900000", + @(122) : @"MT:00000WAX04VHK900000", + @(123) : @"MT:000002N315BVK900000", + @(124) : @"MT:00000AZB16T6L900000", + @(125) : @"MT:00000I9K179KL900000", + @(126) : @"MT:0000000000SXL900000", + @(127) : @"MT:000008C80189M900000", + @(128) : @"MT:00000GOG02QMM900000", + @(129) : @"MT:00000O-O036-M900000", + @(130) : @"MT:00000WAX04OBN900000", + @(131) : @"MT:000002N3154PN900000", + @(132) : @"MT:00000AZB16M0O900000", + @(133) : @"MT:00000I9K172EO900000", + @(134) : @"MT:0000000000LRO900000", + @(135) : @"MT:000008C80113P900000", + @(136) : @"MT:00000GOG02JGP900000", + @(137) : @"MT:00000O-O03.TP900000", + @(138) : @"MT:00000WAX04H5Q900000", + @(139) : @"MT:000002N315ZIQ900000", + @(140) : @"MT:00000AZB16FWQ900000", + @(141) : @"MT:00000I9K17X7R900000", + @(142) : @"MT:0000000000ELR900000", + @(143) : @"MT:000008C801WYR900000", + @(144) : @"MT:00000GOG02CAS900000", + @(145) : @"MT:00000O-O03UNS900000", + @(146) : @"MT:00000WAX04A.S900000", + @(147) : @"MT:000002N315SCT900000", + @(148) : @"MT:00000AZB168QT900000", + @(149) : @"MT:00000I9K17Q1U900000", + @(150) : @"MT:00000000007FU900000", + }; + + // Start our helper apps. + __auto_type * sortedKeys = [[deviceOnboardingPayloads allKeys] sortedArrayUsingSelector:@selector(compare:)]; + for (NSNumber * deviceID in sortedKeys) { + BOOL started = [self startAppWithName:@"all-clusters" + arguments:@[] + payload:deviceOnboardingPayloads[deviceID]]; + XCTAssertTrue(started); + } + + [self doTestSubscriptionPoolWithSize:3 deviceOnboardingPayloads:deviceOnboardingPayloads]; +} + - (MTRDevice *)getMTRDevice:(NSNumber *)deviceID { __auto_type * factory = [MTRDeviceControllerFactory sharedInstance];