Skip to content

Commit cfdaf79

Browse files
Ensure we create a clean subscription on _deviceMayBeReachable. (project-chip#36842)
Just clear out all existing subscription/session state when this API is called. Callers should ensure there are no spurious calls.
1 parent b49b845 commit cfdaf79

File tree

1 file changed

+51
-21
lines changed

1 file changed

+51
-21
lines changed

src/darwin/Framework/CHIP/MTRDevice_Concrete.mm

+51-21
Original file line numberDiff line numberDiff line change
@@ -871,13 +871,7 @@ - (void)invalidate
871871
// tear down the subscription. We will get no more callbacks from
872872
// the subscription after this point.
873873
std::lock_guard lock(self->_lock);
874-
self->_currentReadClient = nullptr;
875-
if (self->_currentSubscriptionCallback) {
876-
delete self->_currentSubscriptionCallback;
877-
}
878-
self->_currentSubscriptionCallback = nullptr;
879-
880-
[self _changeInternalState:MTRInternalDeviceStateUnsubscribed];
874+
[self _resetSubscription];
881875
}
882876
errorHandler:nil];
883877

@@ -938,8 +932,8 @@ - (void)_triggerResubscribeWithReason:(NSString *)reason nodeLikelyReachable:(BO
938932
}
939933
readClientToResubscribe->TriggerResubscribeIfScheduled(reason.UTF8String);
940934
} else if (_internalDeviceState == MTRInternalDeviceStateSubscribing && nodeLikelyReachable) {
941-
// If we have reason to suspect that the node is now reachable and we havent established a
942-
// CASE session yet, lets consider it to be stalled and invalidate the pairing session.
935+
// If we have reason to suspect that the node is now reachable and we haven't established a
936+
// CASE session yet, let's consider it to be stalled and invalidate the pairing session.
943937
[self._concreteController asyncGetCommissionerOnMatterQueue:^(Controller::DeviceCommissioner * commissioner) {
944938
auto caseSessionMgr = commissioner->CASESessionMgr();
945939
VerifyOrDie(caseSessionMgr != nullptr);
@@ -1164,7 +1158,7 @@ - (void)_handleSubscriptionError:(NSError *)error
11641158
[self _doHandleSubscriptionError:error];
11651159
}
11661160

1167-
- (void)_doHandleSubscriptionError:(NSError *)error
1161+
- (void)_doHandleSubscriptionError:(nullable NSError *)error
11681162
{
11691163
assertChipStackLockedByCurrentThread();
11701164

@@ -1305,7 +1299,13 @@ - (void)_handleResubscriptionNeededWithDelay:(NSNumber *)resubscriptionDelayMs
13051299

13061300
// Change our state before going async.
13071301
[self _changeState:MTRDeviceStateUnknown];
1308-
[self _changeInternalState:MTRInternalDeviceStateResubscribing];
1302+
1303+
// If we have never had a subscription established, stay in the Subscribing
1304+
// state; don't transition to Resubscribing just because our attempt at
1305+
// subscribing failed.
1306+
if (HadSubscriptionEstablishedOnce(self->_internalDeviceState)) {
1307+
[self _changeInternalState:MTRInternalDeviceStateResubscribing];
1308+
}
13091309

13101310
dispatch_async(self.queue, ^{
13111311
[self _handleResubscriptionNeededWithDelayOnDeviceQueue:resubscriptionDelayMs];
@@ -2444,19 +2444,29 @@ - (void)_resetSubscriptionWithReasonString:(NSString *)reasonString
24442444
MTR_LOG("%@ subscription reset disconnecting ReadClient and SubscriptionCallback", self);
24452445

24462446
std::lock_guard lock(self->_lock);
2447-
self->_currentReadClient = nullptr;
2448-
if (self->_currentSubscriptionCallback) {
2449-
delete self->_currentSubscriptionCallback;
2450-
}
2451-
self->_currentSubscriptionCallback = nullptr;
24522447

2453-
[self _doHandleSubscriptionError:nil];
2448+
[self _resetSubscription];
2449+
24542450
// Use nil reset delay so that this keeps existing backoff timing
24552451
[self _doHandleSubscriptionReset:nil];
24562452
}
24572453
errorHandler:nil];
24582454
}
24592455

2456+
- (void)_resetSubscription
2457+
{
2458+
assertChipStackLockedByCurrentThread();
2459+
os_unfair_lock_assert_owner(&_lock);
2460+
2461+
_currentReadClient = nullptr;
2462+
if (_currentSubscriptionCallback) {
2463+
delete _currentSubscriptionCallback;
2464+
_currentSubscriptionCallback = nullptr;
2465+
}
2466+
2467+
[self _doHandleSubscriptionError:nil];
2468+
}
2469+
24602470
#ifdef DEBUG
24612471
- (void)unitTestResetSubscription
24622472
{
@@ -4322,11 +4332,31 @@ - (BOOL)_deviceHasActiveSubscription
43224332

43234333
- (void)_deviceMayBeReachable
43244334
{
4325-
MTR_LOG("%@ _deviceMayBeReachable called", self);
4335+
MTR_LOG("%@ _deviceMayBeReachable called, resetting subscription", self);
43264336
// TODO: This should only be allowed for thread devices
4327-
[_deviceController asyncDispatchToMatterQueue:^{
4328-
[self _triggerResubscribeWithReason:@"SPI client indicated the device may now be reachable"
4329-
nodeLikelyReachable:YES];
4337+
[self._concreteController asyncGetCommissionerOnMatterQueue:^(Controller::DeviceCommissioner * commissioner) {
4338+
// Reset all of our subscription/session state and re-establish it all
4339+
// from the start. Reset our subscription first, before tearing
4340+
// down the session, so we don't have to worry about the
4341+
// notifications from the latter coming through async and
4342+
// complicating the situation. Unfortunately, we do not want to
4343+
// hold the lock when destroying the session, just in case it still
4344+
// ends up calling into us somehow, so we have to break the work up
4345+
// into two separate locked sections...
4346+
{
4347+
std::lock_guard lock(self->_lock);
4348+
[self _clearSubscriptionPoolWork];
4349+
[self _resetSubscription];
4350+
}
4351+
4352+
auto caseSessionMgr = commissioner->CASESessionMgr();
4353+
VerifyOrDie(caseSessionMgr != nullptr);
4354+
caseSessionMgr->ReleaseSession(commissioner->GetPeerScopedId(self->_nodeID.unsignedLongLongValue));
4355+
4356+
std::lock_guard lock(self->_lock);
4357+
// Use _ensureSubscriptionForExistingDelegates so that the subscriptions
4358+
// will go through the pool as needed, not necessarily happen immediately.
4359+
[self _ensureSubscriptionForExistingDelegates:@"SPI client indicated the device may now be reachable"];
43304360
} errorHandler:nil];
43314361
}
43324362

0 commit comments

Comments
 (0)