Skip to content

Commit

Permalink
[Darwin] MTRDevice can get stuck using subscription pool if not reset…
Browse files Browse the repository at this point in the history 
… properly (project-chip#36741)

* [Darwin] MTRDevice can get stuck using subscription pool if not reset properly

* Restyled
  • Loading branch information
@jtung-apple
jtung-apple authored Dec 6, 2024
1 parent fa9d005 commit cbf92b7
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 17 deletions.
22 changes: 20 additions & 2 deletions src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:^{
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand Down Expand Up @@ -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<int64_t>(secondsToWait * NSEC_PER_SEC);
Expand Down Expand Up @@ -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;
}

Expand All @@ -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<unsigned long>(_internalDeviceState), reason);
[self _clearSubscriptionPoolWork];
return;
}

Expand Down
93 changes: 78 additions & 15 deletions src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2641,7 +2641,7 @@ - (void)doTestSubscriptionPoolWithSize:(NSInteger)subscriptionPoolSize deviceOnb

XCTAssertEqualObjects(controller.controllerNodeID, nodeID);

NSArray<NSNumber *> * orderedDeviceIDs = @[ @(101), @(102), @(103), @(104), @(105) ];
NSArray<NSNumber *> * orderedDeviceIDs = [deviceOnboardingPayloads allKeys];

// Commission 5 devices
for (NSNumber * deviceID in orderedDeviceIDs) {
Expand Down Expand Up @@ -2669,21 +2669,16 @@ - (void)doTestSubscriptionPoolWithSize:(NSInteger)subscriptionPoolSize deviceOnb

// Set up expectations and delegates

NSDictionary<NSNumber *, XCTestExpectation *> * 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<NSNumber *, XCTestExpectation *> * 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<NSNumber *, MTRDeviceTestDelegate *> * deviceDelegates = @{
@(101) : [[MTRDeviceTestDelegate alloc] init],
@(102) : [[MTRDeviceTestDelegate alloc] init],
@(103) : [[MTRDeviceTestDelegate alloc] init],
@(104) : [[MTRDeviceTestDelegate alloc] init],
@(105) : [[MTRDeviceTestDelegate alloc] init],
};
NSMutableDictionary<NSNumber *, MTRDeviceTestDelegate *> * 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;
Expand Down Expand Up @@ -2786,6 +2781,74 @@ - (void)testSubscriptionPool
[self doTestSubscriptionPoolWithSize:2 deviceOnboardingPayloads:deviceOnboardingPayloads];
}

- (void)testSubscriptionPoolManyDevices
{
// QRCodes generated for discriminators 1111~1150 and passcodes 1001~1050
NSDictionary<NSNumber *, NSString *> * 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];
Expand Down

0 comments on commit cbf92b7

Please sign in to comment.