Skip to content

Commit

Permalink
Ensure we create a clean subscription on _deviceMayBeReachable. (#36842)
Browse files Browse the repository at this point in the history
Just clear out all existing subscription/session state when this API is called.
Callers should ensure there are no spurious calls.
  • Loading branch information
bzbarsky-apple authored Dec 14, 2024
1 parent b49b845 commit cfdaf79
Showing 1 changed file with 51 additions and 21 deletions.
72 changes: 51 additions & 21 deletions src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
Original file line number Diff line number Diff line change
Expand Up @@ -871,13 +871,7 @@ - (void)invalidate
// tear down the subscription. We will get no more callbacks from
// the subscription after this point.
std::lock_guard lock(self->_lock);
self->_currentReadClient = nullptr;
if (self->_currentSubscriptionCallback) {
delete self->_currentSubscriptionCallback;
}
self->_currentSubscriptionCallback = nullptr;

[self _changeInternalState:MTRInternalDeviceStateUnsubscribed];
[self _resetSubscription];
}
errorHandler:nil];

Expand Down Expand Up @@ -938,8 +932,8 @@ - (void)_triggerResubscribeWithReason:(NSString *)reason nodeLikelyReachable:(BO
}
readClientToResubscribe->TriggerResubscribeIfScheduled(reason.UTF8String);
} else if (_internalDeviceState == MTRInternalDeviceStateSubscribing && nodeLikelyReachable) {
// If we have reason to suspect that the node is now reachable and we havent established a
// CASE session yet, lets consider it to be stalled and invalidate the pairing session.
// 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.
[self._concreteController asyncGetCommissionerOnMatterQueue:^(Controller::DeviceCommissioner * commissioner) {
auto caseSessionMgr = commissioner->CASESessionMgr();
VerifyOrDie(caseSessionMgr != nullptr);
Expand Down Expand Up @@ -1164,7 +1158,7 @@ - (void)_handleSubscriptionError:(NSError *)error
[self _doHandleSubscriptionError:error];
}

- (void)_doHandleSubscriptionError:(NSError *)error
- (void)_doHandleSubscriptionError:(nullable NSError *)error
{
assertChipStackLockedByCurrentThread();

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

// Change our state before going async.
[self _changeState:MTRDeviceStateUnknown];
[self _changeInternalState:MTRInternalDeviceStateResubscribing];

// If we have never had a subscription established, stay in the Subscribing
// state; don't transition to Resubscribing just because our attempt at
// subscribing failed.
if (HadSubscriptionEstablishedOnce(self->_internalDeviceState)) {
[self _changeInternalState:MTRInternalDeviceStateResubscribing];
}

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

std::lock_guard lock(self->_lock);
self->_currentReadClient = nullptr;
if (self->_currentSubscriptionCallback) {
delete self->_currentSubscriptionCallback;
}
self->_currentSubscriptionCallback = nullptr;

[self _doHandleSubscriptionError:nil];
[self _resetSubscription];

// Use nil reset delay so that this keeps existing backoff timing
[self _doHandleSubscriptionReset:nil];
}
errorHandler:nil];
}

- (void)_resetSubscription
{
assertChipStackLockedByCurrentThread();
os_unfair_lock_assert_owner(&_lock);

_currentReadClient = nullptr;
if (_currentSubscriptionCallback) {
delete _currentSubscriptionCallback;
_currentSubscriptionCallback = nullptr;
}

[self _doHandleSubscriptionError:nil];
}

#ifdef DEBUG
- (void)unitTestResetSubscription
{
Expand Down Expand Up @@ -4322,11 +4332,31 @@ - (BOOL)_deviceHasActiveSubscription

- (void)_deviceMayBeReachable
{
MTR_LOG("%@ _deviceMayBeReachable called", self);
MTR_LOG("%@ _deviceMayBeReachable called, resetting subscription", self);
// TODO: This should only be allowed for thread devices
[_deviceController asyncDispatchToMatterQueue:^{
[self _triggerResubscribeWithReason:@"SPI client indicated the device may now be reachable"
nodeLikelyReachable:YES];
[self._concreteController asyncGetCommissionerOnMatterQueue:^(Controller::DeviceCommissioner * commissioner) {
// Reset all of our subscription/session state and re-establish it all
// from the start. Reset our subscription first, before tearing
// down the session, so we don't have to worry about the
// notifications from the latter coming through async and
// complicating the situation. Unfortunately, we do not want to
// hold the lock when destroying the session, just in case it still
// ends up calling into us somehow, so we have to break the work up
// into two separate locked sections...
{
std::lock_guard lock(self->_lock);
[self _clearSubscriptionPoolWork];
[self _resetSubscription];
}

auto caseSessionMgr = commissioner->CASESessionMgr();
VerifyOrDie(caseSessionMgr != nullptr);
caseSessionMgr->ReleaseSession(commissioner->GetPeerScopedId(self->_nodeID.unsignedLongLongValue));

std::lock_guard lock(self->_lock);
// Use _ensureSubscriptionForExistingDelegates so that the subscriptions
// will go through the pool as needed, not necessarily happen immediately.
[self _ensureSubscriptionForExistingDelegates:@"SPI client indicated the device may now be reachable"];
} errorHandler:nil];
}

Expand Down

0 comments on commit cfdaf79

Please sign in to comment.