[Darwin] MTRDeviceConnectivityMonitor stopMonitoring crash fix (#33666)
* [Darwin] MTRDeviceConnectivityMonitor stopMonitoring crash fix
* Update src/darwin/Framework/CHIP/MTRDevice.mm
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
---------
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm
index 4e916d1..98af8fb 100644
--- a/src/darwin/Framework/CHIP/MTRDevice.mm
+++ b/src/darwin/Framework/CHIP/MTRDevice.mm
@@ -395,8 +395,8 @@
NSMutableSet<MTRClusterPath *> * _persistedClusters;
// When we last failed to subscribe to the device (either via
- // _setupSubscription or via the auto-resubscribe behavior of the
- // ReadClient). Nil if we have had no such failures.
+ // _setupSubscriptionWithReason or via the auto-resubscribe behavior
+ // of the ReadClient). Nil if we have had no such failures.
NSDate * _Nullable _lastSubscriptionFailureTime;
MTRDeviceConnectivityMonitor * _connectivityMonitor;
@@ -745,10 +745,10 @@
if ([self _deviceUsesThread]) {
[self _scheduleSubscriptionPoolWork:^{
std::lock_guard lock(self->_lock);
- [self _setupSubscription];
+ [self _setupSubscriptionWithReason:@"delegate is set and scheduled subscription is happening"];
} inNanoseconds:0 description:@"MTRDevice setDelegate first subscription"];
} else {
- [self _setupSubscription];
+ [self _setupSubscriptionWithReason:@"delegate is set and subscription is needed"];
}
}
}
@@ -788,14 +788,14 @@
MTR_LOG("%@ saw new operational advertisement", self);
- [self _triggerResubscribeWithReason:"operational advertisement seen"
+ [self _triggerResubscribeWithReason:@"operational advertisement seen"
nodeLikelyReachable:YES];
}
// Trigger a resubscribe as needed. nodeLikelyReachable should be YES if we
// have reason to suspect the node is now reachable, NO if we have no idea
// whether it might be.
-- (void)_triggerResubscribeWithReason:(const char *)reason nodeLikelyReachable:(BOOL)nodeLikelyReachable
+- (void)_triggerResubscribeWithReason:(NSString *)reason nodeLikelyReachable:(BOOL)nodeLikelyReachable
{
assertChipStackLockedByCurrentThread();
@@ -814,7 +814,7 @@
// establish a CASE session. And at that point, our subscription will
// trigger the state change as needed.
if (self.reattemptingSubscription) {
- [self _reattemptSubscriptionNowIfNeeded];
+ [self _reattemptSubscriptionNowIfNeededWithReason:reason];
} else {
readClientToResubscribe = self->_currentReadClient;
subscriptionCallback = self->_currentSubscriptionCallback;
@@ -829,7 +829,7 @@
// here (e.g. still booting up), but should try again reasonably quickly.
subscriptionCallback->ResetResubscriptionBackoff();
}
- readClientToResubscribe->TriggerResubscribeIfScheduled(reason);
+ readClientToResubscribe->TriggerResubscribeIfScheduled(reason.UTF8String);
}
}
@@ -893,7 +893,7 @@
// ReadClient in there. If the dispatch fails, that's fine; it means our
// controller has shut down, so nothing to be done.
[_deviceController asyncDispatchToMatterQueue:^{
- [self _triggerResubscribeWithReason:"read-through skipped while not subscribed" nodeLikelyReachable:NO];
+ [self _triggerResubscribeWithReason:@"read-through skipped while not subscribed" nodeLikelyReachable:NO];
}
errorHandler:nil];
}
@@ -1160,7 +1160,7 @@
// this block is run -- if other triggering events had happened, this would become a no-op.
auto resubscriptionBlock = ^{
[self->_deviceController asyncDispatchToMatterQueue:^{
- [self _triggerResubscribeWithReason:"ResubscriptionNeeded timer fired" nodeLikelyReachable:NO];
+ [self _triggerResubscribeWithReason:@"ResubscriptionNeeded timer fired" nodeLikelyReachable:NO];
} 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);
@@ -1235,17 +1235,17 @@
// If we started subscription or session establishment but failed, remove item from the subscription pool so we can re-queue.
[self _clearSubscriptionPoolWork];
- // Call _reattemptSubscriptionNowIfNeeded when timer fires - if subscription is
+ // Call _reattemptSubscriptionNowIfNeededWithReason when timer fires - if subscription is
// in a better state at that time this will be a no-op.
auto resubscriptionBlock = ^{
os_unfair_lock_lock(&self->_lock);
- [self _reattemptSubscriptionNowIfNeeded];
+ [self _reattemptSubscriptionNowIfNeededWithReason:@"got subscription reset"];
os_unfair_lock_unlock(&self->_lock);
};
int64_t resubscriptionDelayNs = static_cast<int64_t>(secondsToWait * NSEC_PER_SEC);
if ([self _deviceUsesThread]) {
- // For Thread-enabled devices, schedule the _reattemptSubscriptionNowIfNeeded call to run in the subscription pool
+ // For Thread-enabled devices, schedule the _reattemptSubscriptionNowIfNeededWithReason call to run in the subscription pool
[self _scheduleSubscriptionPoolWork:resubscriptionBlock inNanoseconds:resubscriptionDelayNs description:@"MTRDevice resubscription"];
} else {
// For non-Thread-enabled devices, just call the resubscription block after the specified time
@@ -1253,7 +1253,7 @@
}
}
-- (void)_reattemptSubscriptionNowIfNeeded
+- (void)_reattemptSubscriptionNowIfNeededWithReason:(NSString *)reason
{
os_unfair_lock_assert_owner(&self->_lock);
if (!self.reattemptingSubscription) {
@@ -1262,7 +1262,7 @@
MTR_LOG("%@ reattempting subscription", self);
self.reattemptingSubscription = NO;
- [self _setupSubscription];
+ [self _setupSubscriptionWithReason:reason];
}
- (void)_handleUnsolicitedMessageFromPublisher
@@ -1284,8 +1284,8 @@
// reestablishment, this starts the attempt right away
// TODO: This doesn't really make sense. If we _don't_ have a live
// ReadClient how did we get this notification and if we _do_ have an active
- // ReadClient, this call or _setupSubscription would be no-ops.
- [self _reattemptSubscriptionNowIfNeeded];
+ // ReadClient, this call or _setupSubscriptionWithReason would be no-ops.
+ [self _reattemptSubscriptionNowIfNeededWithReason:@"got unsolicited message from publisher"];
}
- (void)_markDeviceAsUnreachableIfNeverSubscribed
@@ -1941,7 +1941,7 @@
self->_connectivityMonitor = [[MTRDeviceConnectivityMonitor alloc] initWithCompressedFabricID:compressedFabricID nodeID:self.nodeID];
[self->_connectivityMonitor startMonitoringWithHandler:^{
[self->_deviceController asyncDispatchToMatterQueue:^{
- [self _triggerResubscribeWithReason:"device connectivity changed" nodeLikelyReachable:YES];
+ [self _triggerResubscribeWithReason:@"device connectivity changed" nodeLikelyReachable:YES];
}
errorHandler:nil];
} queue:self.queue];
@@ -1959,12 +1959,12 @@
}
// assume lock is held
-- (void)_setupSubscription
+- (void)_setupSubscriptionWithReason:(NSString *)reason
{
os_unfair_lock_assert_owner(&self->_lock);
if (![self _subscriptionsAllowed]) {
- MTR_LOG("_setupSubscription: Subscriptions not allowed. Do not set up subscription");
+ MTR_LOG("%@ _setupSubscription: Subscriptions not allowed. Do not set up subscription", self);
return;
}
@@ -1986,6 +1986,8 @@
[self _changeInternalState:MTRInternalDeviceStateSubscribing];
+ MTR_LOG("%@ setting up subscription with reason: %@", self, reason);
+
// Set up a timer to mark as not reachable if it takes too long to set up a subscription
MTRWeakReference<MTRDevice *> * weakSelf = [MTRWeakReference weakReferenceWithObject:self];
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, static_cast<int64_t>(kSecondsToWaitBeforeMarkingUnreachableAfterSettingUpSubscription) * static_cast<int64_t>(NSEC_PER_SEC)), self.queue, ^{
@@ -3512,7 +3514,7 @@
MTR_LOG("%@ _deviceMayBeReachable called", self);
- [self _triggerResubscribeWithReason:"SPI client indicated the device may now be reachable"
+ [self _triggerResubscribeWithReason:@"SPI client indicated the device may now be reachable"
nodeLikelyReachable:YES];
}
diff --git a/src/darwin/Framework/CHIP/MTRDeviceConnectivityMonitor.mm b/src/darwin/Framework/CHIP/MTRDeviceConnectivityMonitor.mm
index 3df270b..6a9ac60 100644
--- a/src/darwin/Framework/CHIP/MTRDeviceConnectivityMonitor.mm
+++ b/src/darwin/Framework/CHIP/MTRDeviceConnectivityMonitor.mm
@@ -258,7 +258,7 @@
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, kSharedConnectionLingerIntervalSeconds * NSEC_PER_SEC), sSharedResolverQueue, ^{
std::lock_guard lock(sConnectivityMonitorLock);
- if (!sConnectivityMonitorCount) {
+ if (!sConnectivityMonitorCount && sSharedResolverConnection) {
MTR_LOG("MTRDeviceConnectivityMonitor: Closing shared resolver connection");
DNSServiceRefDeallocate(sSharedResolverConnection);
sSharedResolverConnection = NULL;
@@ -271,9 +271,14 @@
- (void)stopMonitoring
{
+ MTR_LOG("%@ stop connectivity monitoring for %@", self, self->_instanceName);
+ std::lock_guard lock(sConnectivityMonitorLock);
+ if (!sSharedResolverConnection || !sSharedResolverQueue) {
+ MTR_LOG("%@ shared resolver connection already stopped - nothing to do", self);
+ }
+
// DNSServiceRefDeallocate must be called on the same queue set on the shared connection.
dispatch_async(sSharedResolverQueue, ^{
- MTR_LOG("%@ stop connectivity monitoring for %@", self, self->_instanceName);
std::lock_guard lock(sConnectivityMonitorLock);
[self _stopMonitoring];
});
