From 413b47b2b86e184777c65c08d1f2809bc0fcd47f Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Mon, 5 Aug 2024 11:24:44 -0700 Subject: [PATCH 1/3] [Darwin] MTRDevice subscription estalished handler should change state before async --- src/darwin/Framework/CHIP/MTRDevice.mm | 34 +++++++++++++++++--------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index b8ef8694825e1f..ee8438f2ecf7bf 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -1193,23 +1193,20 @@ - (void)_handleSubscriptionEstablished { os_unfair_lock_lock(&self->_lock); - // We have completed the subscription work - remove from the subscription pool. - [self _clearSubscriptionPoolWork]; - - // reset subscription attempt wait time when subscription succeeds - _lastSubscriptionAttemptWait = 0; - if (HadSubscriptionEstablishedOnce(_internalDeviceState)) { - [self _changeInternalState:MTRInternalDeviceStateLaterSubscriptionEstablished]; - } else { - MATTER_LOG_METRIC_END(kMetricMTRDeviceInitialSubscriptionSetup, CHIP_NO_ERROR); - [self _changeInternalState:MTRInternalDeviceStateInitialSubscriptionEstablished]; + // If subscription had reset since this handler was scheduled, do not ececute "established" logic below + if (!HaveSubscriptionEstablishedRightNow(_internalDeviceState)) { + MTR_LOG("%@ _handleSubscriptionEstablished run with internal state %lu - skipping subscription establishment logic", self, static_cast(_internalDeviceState)); + return; } - [self _changeState:MTRDeviceStateReachable]; + // We have completed the subscription work - remove from the subscription pool. + [self _clearSubscriptionPoolWork]; // No need to monitor connectivity after subscription establishment [self _stopConnectivityMonitoring]; + _lastSubscriptionAttemptWait = 0; + auto initialSubscribeStart = _initialSubscribeStart; // We no longer need to track subscribe latency for this device. _initialSubscribeStart = nil; @@ -2476,6 +2473,21 @@ - (void)_setupSubscriptionWithReason:(NSString *)reason }, ^(void) { MTR_LOG("%@ got subscription established", self); + std::lock_guard lock(self->_lock); + + // First synchronously change state + + // reset subscription attempt wait time when subscription succeeds + if (HadSubscriptionEstablishedOnce(self->_internalDeviceState)) { + [self _changeInternalState:MTRInternalDeviceStateLaterSubscriptionEstablished]; + } else { + MATTER_LOG_METRIC_END(kMetricMTRDeviceInitialSubscriptionSetup, CHIP_NO_ERROR); + [self _changeInternalState:MTRInternalDeviceStateInitialSubscriptionEstablished]; + } + + [self _changeState:MTRDeviceStateReachable]; + + // Then async work that shouldn't be performed on the matter queue dispatch_async(self.queue, ^{ // OnSubscriptionEstablished [self _handleSubscriptionEstablished]; From 80469f653823dd84769cd8edac0bd3f885e27b05 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Tue, 6 Aug 2024 07:35:03 -0700 Subject: [PATCH 2/3] Update src/darwin/Framework/CHIP/MTRDevice.mm Co-authored-by: Kiel Oleson --- 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 ee8438f2ecf7bf..0f4dde0a5887da 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -1193,7 +1193,7 @@ - (void)_handleSubscriptionEstablished { os_unfair_lock_lock(&self->_lock); - // If subscription had reset since this handler was scheduled, do not ececute "established" logic below + // If subscription had reset since this handler was scheduled, do not execute "established" logic below if (!HaveSubscriptionEstablishedRightNow(_internalDeviceState)) { MTR_LOG("%@ _handleSubscriptionEstablished run with internal state %lu - skipping subscription establishment logic", self, static_cast(_internalDeviceState)); return; From 9fb264a6f680d1446ee0614164a5ad8d0b75d58f Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Tue, 6 Aug 2024 07:37:28 -0700 Subject: [PATCH 3/3] Addressed PR review - moved code comment to correct place --- src/darwin/Framework/CHIP/MTRDevice.mm | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 0f4dde0a5887da..07b695cd3c80d4 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -1205,6 +1205,7 @@ - (void)_handleSubscriptionEstablished // No need to monitor connectivity after subscription establishment [self _stopConnectivityMonitoring]; + // reset subscription attempt wait time when subscription succeeds _lastSubscriptionAttemptWait = 0; auto initialSubscribeStart = _initialSubscribeStart; @@ -2476,8 +2477,6 @@ - (void)_setupSubscriptionWithReason:(NSString *)reason std::lock_guard lock(self->_lock); // First synchronously change state - - // reset subscription attempt wait time when subscription succeeds if (HadSubscriptionEstablishedOnce(self->_internalDeviceState)) { [self _changeInternalState:MTRInternalDeviceStateLaterSubscriptionEstablished]; } else {