From 13985787aea0d5f7cfbc89d579af2dc851f5fb8d Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Thu, 7 Dec 2023 20:30:41 -0800 Subject: [PATCH] [Darwin] MTRDevice should reset subscription retry backoff on triggering condition (#30885) --- .../CHIP/MTRBaseSubscriptionCallback.h | 7 ++++ .../CHIP/MTRBaseSubscriptionCallback.mm | 38 ++++++++++++++++++- src/darwin/Framework/CHIP/MTRDevice.mm | 6 +++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.h b/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.h index a64e8d1fe6d5cf..371bd8db3a4bd0 100644 --- a/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.h +++ b/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.h @@ -100,6 +100,9 @@ class MTRBaseSubscriptionCallback : public chip::app::ClusterStateCache::Callbac mClusterStateCache = std::move(aClusterStateCache); } + // Used to reset Resubscription backoff on events that indicate likely availability of device to come back online + void ResetResubscriptionBackoff() { mResubscriptionNumRetries = 0; } + protected: // Report an error, which may be due to issues in our own internal state or // due to the OnError callback happening. @@ -178,6 +181,10 @@ class MTRBaseSubscriptionCallback : public chip::app::ClusterStateCache::Callbac bool mHaveQueuedDeletion = false; OnDoneHandler _Nullable mOnDoneHandler = nil; dispatch_block_t mInterimReportBlock = nil; + + // Copied from ReadClient and customized for + uint32_t ComputeTimeTillNextSubscription(); + uint32_t mResubscriptionNumRetries = 0; }; NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.mm b/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.mm index e498a1682d52aa..079cefce5c9fc9 100644 --- a/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.mm +++ b/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.mm @@ -18,6 +18,7 @@ #import "MTRError_Internal.h" #import "MTRLogging_Internal.h" +#include #include using namespace chip; @@ -124,16 +125,49 @@ void MTRBaseSubscriptionCallback::OnSubscriptionEstablished(SubscriptionId aSubscriptionId) { + // ReadClient resets it at ProcessSubscribeResponse after calling OnSubscriptionEstablished, so this is equivalent + mResubscriptionNumRetries = 0; if (mSubscriptionEstablishedHandler) { auto subscriptionEstablishedHandler = mSubscriptionEstablishedHandler; subscriptionEstablishedHandler(); } } +uint32_t MTRBaseSubscriptionCallback::ComputeTimeTillNextSubscription() +{ + uint32_t maxWaitTimeInMsec = 0; + uint32_t waitTimeInMsec = 0; + uint32_t minWaitTimeInMsec = 0; + + if (mResubscriptionNumRetries <= CHIP_RESUBSCRIBE_MAX_FIBONACCI_STEP_INDEX) { + maxWaitTimeInMsec = GetFibonacciForIndex(mResubscriptionNumRetries) * CHIP_RESUBSCRIBE_WAIT_TIME_MULTIPLIER_MS; + } else { + maxWaitTimeInMsec = CHIP_RESUBSCRIBE_MAX_RETRY_WAIT_INTERVAL_MS; + } + + if (maxWaitTimeInMsec != 0) { + minWaitTimeInMsec = (CHIP_RESUBSCRIBE_MIN_WAIT_TIME_INTERVAL_PERCENT_PER_STEP * maxWaitTimeInMsec) / 100; + waitTimeInMsec = minWaitTimeInMsec + (Crypto::GetRandU32() % (maxWaitTimeInMsec - minWaitTimeInMsec)); + } + + return waitTimeInMsec; +} + CHIP_ERROR MTRBaseSubscriptionCallback::OnResubscriptionNeeded(ReadClient * apReadClient, CHIP_ERROR aTerminationCause) { - CHIP_ERROR err = ClusterStateCache::Callback::OnResubscriptionNeeded(apReadClient, aTerminationCause); - ReturnErrorOnFailure(err); + // No need to check ReadClient internal state is Idle because ReadClient only calls OnResubscriptionNeeded after calling ClearActiveSubscriptionState(), which sets the state to Idle. + + // This part is copied from ReadClient's DefaultResubscribePolicy: + auto timeTillNextResubscription = ComputeTimeTillNextSubscription(); + ChipLogProgress(DataManagement, + "Will try to resubscribe to %02x:" ChipLogFormatX64 " at retry index %" PRIu32 " after %" PRIu32 + "ms due to error %" CHIP_ERROR_FORMAT, + apReadClient->GetFabricIndex(), ChipLogValueX64(apReadClient->GetPeerNodeId()), mResubscriptionNumRetries, timeTillNextResubscription, + aTerminationCause.Format()); + ReturnErrorOnFailure(apReadClient->ScheduleResubscription(timeTillNextResubscription, NullOptional, aTerminationCause == CHIP_ERROR_TIMEOUT)); + + // Not as good a place to increment as when resubscription timer fires, but as is, this should be as good, because OnResubscriptionNeeded is only called from ReadClient's Close() while Idle, and nothing should cause this to happen + mResubscriptionNumRetries++; if (mResubscriptionCallback != nil) { auto callback = mResubscriptionCallback; diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index e05d4a75bcf4ab..1ce1ff183457fc 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -196,6 +196,7 @@ @interface MTRDevice () * an OnDone for that ReadClient. */ @property (nonatomic) ReadClient * currentReadClient; +@property (nonatomic) SubscriptionCallback * currentSubscriptionCallback; // valid when and only when currentReadClient is valid @end @@ -299,6 +300,7 @@ - (void)nodeMayBeAdvertisingOperational // nodeMayBeAdvertisingOperational is called we are running on the Matter // queue, and the ReadClient can't get destroyed while we are on that queue. ReadClient * readClientToResubscribe = nullptr; + SubscriptionCallback * subscriptionCallback = nullptr; os_unfair_lock_lock(&self->_lock); @@ -310,10 +312,12 @@ - (void)nodeMayBeAdvertisingOperational [self _reattemptSubscriptionNowIfNeeded]; } else { readClientToResubscribe = self->_currentReadClient; + subscriptionCallback = self->_currentSubscriptionCallback; } os_unfair_lock_unlock(&self->_lock); if (readClientToResubscribe) { + subscriptionCallback->ResetResubscriptionBackoff(); readClientToResubscribe->TriggerResubscribeIfScheduled("operational advertisement seen"); } } @@ -741,6 +745,7 @@ - (void)_setupSubscription // holding a dangling pointer. os_unfair_lock_lock(&self->_lock); self->_currentReadClient = nullptr; + self->_currentSubscriptionCallback = nullptr; os_unfair_lock_unlock(&self->_lock); dispatch_async(self.queue, ^{ // OnDone @@ -793,6 +798,7 @@ - (void)_setupSubscription // when OnDone is called. os_unfair_lock_lock(&self->_lock); self->_currentReadClient = readClient.get(); + self->_currentSubscriptionCallback = callback.get(); os_unfair_lock_unlock(&self->_lock); callback->AdoptReadClient(std::move(readClient)); callback->AdoptClusterStateCache(std::move(clusterStateCache));