From 1e0c658a931eea4ca1a0fcbc84e7d39809184521 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 9 Apr 2024 16:37:13 -0400 Subject: [PATCH] Take CASE BUSY delay into account when scheduling retries in Matter.framework. --- src/darwin/Framework/CHIP/MTRBaseDevice.mm | 4 +-- .../Framework/CHIP/MTRCallbackBridgeBase.h | 6 ++--- src/darwin/Framework/CHIP/MTRDevice.mm | 27 ++++++++++++++----- .../CHIP/MTRDeviceConnectionBridge.h | 12 ++++++--- .../CHIP/MTRDeviceConnectionBridge.mm | 10 ++++--- .../Framework/CHIP/MTRDeviceController.mm | 12 ++++----- 6 files changed, 46 insertions(+), 25 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.mm b/src/darwin/Framework/CHIP/MTRBaseDevice.mm index cb93d2db5d0e5b..ccefbb18497414 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.mm +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.mm @@ -365,7 +365,7 @@ - (void)subscribeWithQueue:(dispatch_queue_t)queue [self.deviceController getSessionForNode:self.nodeID completion:^(ExchangeManager * _Nullable exchangeManager, const Optional & session, - NSError * _Nullable error) { + NSError * _Nullable error, NSNumber * _Nullable retryDelay) { if (error != nil) { dispatch_async(queue, ^{ errorHandler(error); @@ -1603,7 +1603,7 @@ - (void)subscribeToAttributePaths:(NSArray * _Nullabl [self.deviceController getSessionForNode:self.nodeID completion:^(ExchangeManager * _Nullable exchangeManager, const Optional & session, - NSError * _Nullable error) { + NSError * _Nullable error, NSNumber * _Nullable retryDelay) { if (error != nil) { dispatch_async(queue, ^{ reportHandler(nil, error); diff --git a/src/darwin/Framework/CHIP/MTRCallbackBridgeBase.h b/src/darwin/Framework/CHIP/MTRCallbackBridgeBase.h index d113b5dedc86a1..6631e2d132bb54 100644 --- a/src/darwin/Framework/CHIP/MTRCallbackBridgeBase.h +++ b/src/darwin/Framework/CHIP/MTRCallbackBridgeBase.h @@ -83,7 +83,7 @@ class MTRCallbackBridgeBase { [device.deviceController getSessionForCommissioneeDevice:device.nodeID completion:^(chip::Messaging::ExchangeManager * exchangeManager, - const chip::Optional & session, NSError * error) { + const chip::Optional & session, NSError * _Nullable error, NSNumber * _Nullable retryDelay) { MaybeDoAction(exchangeManager, session, error); }]; } @@ -93,8 +93,8 @@ class MTRCallbackBridgeBase { LogRequestStart(); [controller getSessionForNode:nodeID - completion:^(chip::Messaging::ExchangeManager * exchangeManager, - const chip::Optional & session, NSError * error) { + completion:^(chip::Messaging::ExchangeManager * _Nullable exchangeManager, + const chip::Optional & session, NSError * _Nullable error, NSNumber * _Nullable retryDelay) { MaybeDoAction(exchangeManager, session, error); }]; } diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 17916610a27688..a5ae3904156367 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -771,7 +771,7 @@ - (void)_handleResubscriptionNeeded [self _changeState:MTRDeviceStateUnknown]; } -- (void)_handleSubscriptionReset +- (void)_handleSubscriptionReset:(NSNumber * _Nullable)retryDelay { std::lock_guard lock(_lock); // if there is no delegate then also do not retry @@ -790,17 +790,29 @@ - (void)_handleSubscriptionReset self.reattemptingSubscription = YES; + NSTimeInterval secondsToWait; if (_lastSubscriptionAttemptWait < MTRDEVICE_SUBSCRIPTION_ATTEMPT_MIN_WAIT_SECONDS) { _lastSubscriptionAttemptWait = MTRDEVICE_SUBSCRIPTION_ATTEMPT_MIN_WAIT_SECONDS; + secondsToWait = _lastSubscriptionAttemptWait; + } else if (retryDelay != nil) { + // The device responded but is currently busy. Reset our backoff + // counter, so that we don't end up waiting for a long time if the next + // attempt fails for some reason, and retry after whatever time period + // the device told us to use. + _lastSubscriptionAttemptWait = 0; + secondsToWait = retryDelay.doubleValue; + MTR_LOG_INFO("%@ resetting resubscribe attempt counter, and delaying by the server-provided delay: %f", + self, secondsToWait); } else { _lastSubscriptionAttemptWait *= 2; if (_lastSubscriptionAttemptWait > MTRDEVICE_SUBSCRIPTION_ATTEMPT_MAX_WAIT_SECONDS) { _lastSubscriptionAttemptWait = MTRDEVICE_SUBSCRIPTION_ATTEMPT_MAX_WAIT_SECONDS; } + secondsToWait = _lastSubscriptionAttemptWait; } - MTR_LOG_DEFAULT("%@ scheduling to reattempt subscription in %u seconds", self, _lastSubscriptionAttemptWait); - dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t) (_lastSubscriptionAttemptWait * NSEC_PER_SEC)), self.queue, ^{ + MTR_LOG_DEFAULT("%@ scheduling to reattempt subscription in %f seconds", self, secondsToWait); + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t) (secondsToWait * NSEC_PER_SEC)), self.queue, ^{ os_unfair_lock_lock(&self->_lock); [self _reattemptSubscriptionNowIfNeeded]; os_unfair_lock_unlock(&self->_lock); @@ -1133,12 +1145,13 @@ - (void)_setupSubscription [_deviceController getSessionForNode:_nodeID.unsignedLongLongValue completion:^(chip::Messaging::ExchangeManager * _Nullable exchangeManager, - const chip::Optional & session, NSError * _Nullable error) { + const chip::Optional & session, NSError * _Nullable error, + NSNumber * _Nullable retryDelay) { if (error != nil) { MTR_LOG_ERROR("%@ getSessionForNode error %@", self, error); dispatch_async(self.queue, ^{ [self _handleSubscriptionError:error]; - [self _handleSubscriptionReset]; + [self _handleSubscriptionReset:retryDelay]; }); return; } @@ -1193,7 +1206,7 @@ - (void)_setupSubscription dispatch_async(self.queue, ^{ // OnDone - [self _handleSubscriptionReset]; + [self _handleSubscriptionReset:nil]; }); os_unfair_lock_unlock(&self->_lock); }, @@ -1300,7 +1313,7 @@ - (void)_setupSubscription MTR_LOG_ERROR("%@ SendAutoResubscribeRequest error %@", self, error); dispatch_async(self.queue, ^{ [self _handleSubscriptionError:error]; - [self _handleSubscriptionReset]; + [self _handleSubscriptionReset:nil]; }); return; diff --git a/src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.h b/src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.h index 01d248102b692d..e4d2dda652d0d0 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.h +++ b/src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.h @@ -19,6 +19,7 @@ #import +#include #include #include #include @@ -27,9 +28,12 @@ NS_ASSUME_NONNULL_BEGIN // Either exchangeManager will be non-nil and session will have a value, or -// error will be non-nil. +// error will be non-nil. If error is non-nil, retryDelay might be non-nil. In +// that case it will contain an NSTimeInterval indicating how long consumers +// should delay before trying again (e.g. based on the information communicated +// in a BUSY response). typedef void (^MTRInternalDeviceConnectionCallback)(chip::Messaging::ExchangeManager * _Nullable exchangeManager, - const chip::Optional & session, NSError * _Nullable error); + const chip::Optional & session, NSError * _Nullable error, NSNumber * _Nullable retryDelay); /** * Helper to establish or look up a CASE session and hand its session @@ -62,11 +66,11 @@ class MTRDeviceConnectionBridge : public chip::ReferenceCounted mOnConnected; - chip::Callback::Callback mOnConnectFailed; + chip::Callback::Callback mOnConnectFailed; static void OnConnected( void * context, chip::Messaging::ExchangeManager & exchangeMgr, const chip::SessionHandle & sessionHandle); - static void OnConnectionFailure(void * context, const chip::ScopedNodeId & peerId, CHIP_ERROR error); + static void OnConnectionFailure(void * context, const chip::OperationalSessionSetup::ConnnectionFailureInfo & failureInfo); }; NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.mm b/src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.mm index 490fe3e7bb4443..3fa1c45028adcd 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.mm @@ -23,13 +23,17 @@ void * context, chip::Messaging::ExchangeManager & exchangeMgr, const chip::SessionHandle & sessionHandle) { auto * object = static_cast(context); - object->mCompletionHandler(&exchangeMgr, chip::MakeOptional(*sessionHandle->AsSecureSession()), nil); + object->mCompletionHandler(&exchangeMgr, chip::MakeOptional(*sessionHandle->AsSecureSession()), nil, nil); object->Release(); } -void MTRDeviceConnectionBridge::OnConnectionFailure(void * context, const chip::ScopedNodeId & peerId, CHIP_ERROR error) +void MTRDeviceConnectionBridge::OnConnectionFailure(void * context, const chip::OperationalSessionSetup::ConnnectionFailureInfo & failureInfo) { + NSNumber * retryDelay; + if (failureInfo.requestedBusyDelay.HasValue()) { + retryDelay = @(static_cast(failureInfo.requestedBusyDelay.Value().count()) / MSEC_PER_SEC); + } auto * object = static_cast(context); - object->mCompletionHandler(nil, chip::NullOptional, [MTRError errorForCHIPErrorCode:error]); + object->mCompletionHandler(nil, chip::NullOptional, [MTRError errorForCHIPErrorCode:failureInfo.error], retryDelay); object->Release(); } diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 22d52cd5b4a684..cc9bc0d06f7c6c 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -1215,7 +1215,7 @@ - (void)getSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConn connectionBridge->connect(commissioner, nodeID); } errorHandler:^(NSError * error) { - completion(nullptr, chip::NullOptional, error); + completion(nullptr, chip::NullOptional, error, nil); }]; } @@ -1226,20 +1226,20 @@ - (void)getSessionForCommissioneeDevice:(chip::NodeId)deviceID completion:(MTRIn chip::CommissioneeDeviceProxy * deviceProxy; CHIP_ERROR err = commissioner->GetDeviceBeingCommissioned(deviceID, &deviceProxy); if (err != CHIP_NO_ERROR) { - completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:err]); + completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:err], nil); return; } chip::Optional session = deviceProxy->GetSecureSession(); if (!session.HasValue() || !session.Value()->AsSecureSession()->IsPASESession()) { - completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE]); + completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE], nil); return; } - completion(deviceProxy->GetExchangeManager(), session, nil); + completion(deviceProxy->GetExchangeManager(), session, nil, nil); } errorHandler:^(NSError * error) { - completion(nullptr, chip::NullOptional, error); + completion(nullptr, chip::NullOptional, error, nil); }]; } @@ -1619,7 +1619,7 @@ - (BOOL)getBaseDevice:(uint64_t)deviceID queue:(dispatch_queue_t)queue completio // that we are running. [self getSessionForNode:deviceID completion:^(chip::Messaging::ExchangeManager * _Nullable exchangeManager, - const chip::Optional & session, NSError * _Nullable error) { + const chip::Optional & session, NSError * _Nullable error, NSNumber * _Nullable retryDelay) { // Create an MTRBaseDevice for the node id involved, now that our // CASE session is primed. We don't actually care about the session // information here.