From 2a4227005254cd5c97615e6afabb02b5adff72b4 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Wed, 29 May 2024 09:51:01 -0700 Subject: [PATCH 1/2] [Darwin] MTRDeviceConnectivityMonitor stopMonitoring crash fix --- src/darwin/Framework/CHIP/MTRDevice.mm | 44 ++++++++++--------- .../CHIP/MTRDeviceConnectivityMonitor.mm | 9 +++- 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 4e916d14b101c8..3eb89f8f688de1 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -395,8 +395,8 @@ @implementation MTRDevice { NSMutableSet * _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 @@ - (void)setDelegate:(id)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"]; } inNanoseconds:0 description:@"MTRDevice setDelegate first subscription"]; } else { - [self _setupSubscription]; + [self _setupSubscriptionWithReason:@"delegate is set and subscription is needed"]; } } } @@ -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(); @@ -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; @@ -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); } } @@ -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]; } @@ -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); @@ -1235,17 +1235,17 @@ - (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(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)_handleSubscriptionReset:(NSNumber * _Nullable)retryDelay } } -- (void)_reattemptSubscriptionNowIfNeeded +- (void)_reattemptSubscriptionNowIfNeededWithReason:(NSString *)reason { os_unfair_lock_assert_owner(&self->_lock); if (!self.reattemptingSubscription) { @@ -1262,7 +1262,7 @@ - (void)_reattemptSubscriptionNowIfNeeded MTR_LOG("%@ reattempting subscription", self); self.reattemptingSubscription = NO; - [self _setupSubscription]; + [self _setupSubscriptionWithReason:reason]; } - (void)_handleUnsolicitedMessageFromPublisher @@ -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 @@ -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]; @@ -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; } @@ -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 * weakSelf = [MTRWeakReference weakReferenceWithObject:self]; dispatch_after(dispatch_time(DISPATCH_TIME_NOW, static_cast(kSecondsToWaitBeforeMarkingUnreachableAfterSettingUpSubscription) * static_cast(NSEC_PER_SEC)), self.queue, ^{ @@ -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]; } diff --git a/src/darwin/Framework/CHIP/MTRDeviceConnectivityMonitor.mm b/src/darwin/Framework/CHIP/MTRDeviceConnectivityMonitor.mm index 3df270be9ce62e..6a9ac601d41b6f 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceConnectivityMonitor.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceConnectivityMonitor.mm @@ -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) { MTR_LOG("MTRDeviceConnectivityMonitor: Closing shared resolver connection"); DNSServiceRefDeallocate(sSharedResolverConnection); sSharedResolverConnection = NULL; @@ -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]; }); From e4ee7ee3b2a2060ce820ea4cdbf6dda23b629896 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Wed, 29 May 2024 13:31:45 -0700 Subject: [PATCH 2/2] Update src/darwin/Framework/CHIP/MTRDevice.mm Co-authored-by: Boris Zbarsky --- src/darwin/Framework/CHIP/MTRDevice.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 3eb89f8f688de1..98af8fb4ab16c2 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -745,7 +745,7 @@ - (void)setDelegate:(id)delegate queue:(dispatch_queue_t)queu if ([self _deviceUsesThread]) { [self _scheduleSubscriptionPoolWork:^{ std::lock_guard lock(self->_lock); - [self _setupSubscriptionWithReason:@"delegate is set and subscription is scheduled"]; + [self _setupSubscriptionWithReason:@"delegate is set and scheduled subscription is happening"]; } inNanoseconds:0 description:@"MTRDevice setDelegate first subscription"]; } else { [self _setupSubscriptionWithReason:@"delegate is set and subscription is needed"];