Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Darwin] MTRDeviceConnectivityMonitor stopMonitoring crash fix #33666

Merged
merged 2 commits into from
May 30, 2024
Merged

[Darwin] MTRDeviceConnectivityMonitor stopMonitoring crash fix #33666

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a42270
[Darwin] MTRDeviceConnectivityMonitor stopMonitoring crash fix
jtung-apple May 29, 2024
e4ee7ee
Update src/darwin/Framework/CHIP/MTRDevice.mm
jtung-apple May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 23 additions & 21 deletions src/darwin/Framework/CHIP/MTRDevice.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,8 +395,8 @@ @implementation MTRDevice {
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;

Expand Down Expand Up @@ -745,10 +745,10 @@ - (void)setDelegate:(id<MTRDeviceDelegate>)delegate queue:(dispatch_queue_t)queu
if ([self _deviceUsesThread]) {
[self _scheduleSubscriptionPoolWork:^{
std::lock_guard lock(self->_lock);
[self _setupSubscription];
[self _setupSubscriptionWithReason:@"delegate is set and subscription is scheduled"];
jtung-apple marked this conversation as resolved.
Show resolved Hide resolved
} inNanoseconds:0 description:@"MTRDevice setDelegate first subscription"];
} else {
[self _setupSubscription];
[self _setupSubscriptionWithReason:@"delegate is set and subscription is needed"];
jtung-apple marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down Expand Up @@ -788,14 +788,14 @@ - (void)nodeMayBeAdvertisingOperational

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();

Expand All @@ -814,7 +814,7 @@ - (void)_triggerResubscribeWithReason:(const char *)reason nodeLikelyReachable:(
// 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;
Expand All @@ -829,7 +829,7 @@ - (void)_triggerResubscribeWithReason:(const char *)reason nodeLikelyReachable:(
// here (e.g. still booting up), but should try again reasonably quickly.
subscriptionCallback->ResetResubscriptionBackoff();
}
readClientToResubscribe->TriggerResubscribeIfScheduled(reason);
readClientToResubscribe->TriggerResubscribeIfScheduled(reason.UTF8String);
}
}

Expand Down Expand Up @@ -893,7 +893,7 @@ - (void)_readThroughSkipped
// 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];
}
Expand Down Expand Up @@ -1160,7 +1160,7 @@ - (void)_handleResubscriptionNeededWithDelay:(NSNumber *)resubscriptionDelayMs
// 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);
Expand Down Expand Up @@ -1235,25 +1235,25 @@ - (void)_handleSubscriptionReset:(NSNumber * _Nullable)retryDelay
// 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
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, resubscriptionDelayNs), self.queue, resubscriptionBlock);
}
}

- (void)_reattemptSubscriptionNowIfNeeded
- (void)_reattemptSubscriptionNowIfNeededWithReason:(NSString *)reason
{
os_unfair_lock_assert_owner(&self->_lock);
if (!self.reattemptingSubscription) {
Expand All @@ -1262,7 +1262,7 @@ - (void)_reattemptSubscriptionNowIfNeeded

MTR_LOG("%@ reattempting subscription", self);
self.reattemptingSubscription = NO;
[self _setupSubscription];
[self _setupSubscriptionWithReason:reason];
}

- (void)_handleUnsolicitedMessageFromPublisher
Expand All @@ -1284,8 +1284,8 @@ - (void)_handleUnsolicitedMessageFromPublisher
// 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
Expand Down Expand Up @@ -1941,7 +1941,7 @@ - (void)_setupConnectivityMonitoring
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];
Expand All @@ -1959,12 +1959,12 @@ - (void)_stopConnectivityMonitoring
}

// 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;
}

Expand All @@ -1986,6 +1986,8 @@ - (void)_setupSubscription

[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, ^{
Expand Down Expand Up @@ -3512,7 +3514,7 @@ - (void)_deviceMayBeReachable

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];
}

Expand Down
9 changes: 7 additions & 2 deletions src/darwin/Framework/CHIP/MTRDeviceConnectivityMonitor.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ - (void)_stopMonitoring
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, kSharedConnectionLingerIntervalSeconds * NSEC_PER_SEC), sSharedResolverQueue, ^{
std::lock_guard lock(sConnectivityMonitorLock);

if (!sConnectivityMonitorCount) {
if (!sConnectivityMonitorCount && sSharedResolverConnection) {
jtung-apple marked this conversation as resolved.
Show resolved Hide resolved
MTR_LOG("MTRDeviceConnectivityMonitor: Closing shared resolver connection");
DNSServiceRefDeallocate(sSharedResolverConnection);
sSharedResolverConnection = NULL;
Expand All @@ -271,9 +271,14 @@ - (void)_stopMonitoring

- (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];
});
Expand Down
Loading