Skip to content

Commit

Permalink
Take CASE BUSY delay into account when scheduling retries in Matter.f…
Browse files Browse the repository at this point in the history
…ramework. (project-chip#32920)
  • Loading branch information
bzbarsky-apple authored Apr 10, 2024
1 parent e05d08e commit ee6c9e9
Show file tree
Hide file tree
Showing 6 changed files with 46 additions and 25 deletions.
4 changes: 2 additions & 2 deletions src/darwin/Framework/CHIP/MTRBaseDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ - (void)subscribeWithQueue:(dispatch_queue_t)queue

[self.deviceController getSessionForNode:self.nodeID
completion:^(ExchangeManager * _Nullable exchangeManager, const Optional<SessionHandle> & session,
NSError * _Nullable error) {
NSError * _Nullable error, NSNumber * _Nullable retryDelay) {
if (error != nil) {
dispatch_async(queue, ^{
errorHandler(error);
Expand Down Expand Up @@ -1603,7 +1603,7 @@ - (void)subscribeToAttributePaths:(NSArray<MTRAttributeRequestPath *> * _Nullabl
[self.deviceController
getSessionForNode:self.nodeID
completion:^(ExchangeManager * _Nullable exchangeManager, const Optional<SessionHandle> & session,
NSError * _Nullable error) {
NSError * _Nullable error, NSNumber * _Nullable retryDelay) {
if (error != nil) {
dispatch_async(queue, ^{
reportHandler(nil, error);
Expand Down
6 changes: 3 additions & 3 deletions src/darwin/Framework/CHIP/MTRCallbackBridgeBase.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ class MTRCallbackBridgeBase {

[device.deviceController getSessionForCommissioneeDevice:device.nodeID
completion:^(chip::Messaging::ExchangeManager * exchangeManager,
const chip::Optional<chip::SessionHandle> & session, NSError * error) {
const chip::Optional<chip::SessionHandle> & session, NSError * _Nullable error, NSNumber * _Nullable retryDelay) {
MaybeDoAction(exchangeManager, session, error);
}];
}
Expand All @@ -93,8 +93,8 @@ class MTRCallbackBridgeBase {
LogRequestStart();

[controller getSessionForNode:nodeID
completion:^(chip::Messaging::ExchangeManager * exchangeManager,
const chip::Optional<chip::SessionHandle> & session, NSError * error) {
completion:^(chip::Messaging::ExchangeManager * _Nullable exchangeManager,
const chip::Optional<chip::SessionHandle> & session, NSError * _Nullable error, NSNumber * _Nullable retryDelay) {
MaybeDoAction(exchangeManager, session, error);
}];
}
Expand Down
27 changes: 20 additions & 7 deletions src/darwin/Framework/CHIP/MTRDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
Expand Down Expand Up @@ -1133,12 +1145,13 @@ - (void)_setupSubscription
[_deviceController
getSessionForNode:_nodeID.unsignedLongLongValue
completion:^(chip::Messaging::ExchangeManager * _Nullable exchangeManager,
const chip::Optional<chip::SessionHandle> & session, NSError * _Nullable error) {
const chip::Optional<chip::SessionHandle> & 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;
}
Expand Down Expand Up @@ -1193,7 +1206,7 @@ - (void)_setupSubscription

dispatch_async(self.queue, ^{
// OnDone
[self _handleSubscriptionReset];
[self _handleSubscriptionReset:nil];
});
os_unfair_lock_unlock(&self->_lock);
},
Expand Down Expand Up @@ -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;
Expand Down
12 changes: 8 additions & 4 deletions src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#import <Foundation/Foundation.h>

#include <app/OperationalSessionSetup.h>
#include <controller/CHIPDeviceController.h>
#include <lib/core/ReferenceCounted.h>
#include <messaging/ExchangeMgr.h>
Expand All @@ -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<chip::SessionHandle> & session, NSError * _Nullable error);
const chip::Optional<chip::SessionHandle> & session, NSError * _Nullable error, NSNumber * _Nullable retryDelay);

/**
* Helper to establish or look up a CASE session and hand its session
Expand Down Expand Up @@ -62,11 +66,11 @@ class MTRDeviceConnectionBridge : public chip::ReferenceCounted<MTRDeviceConnect
private:
MTRInternalDeviceConnectionCallback mCompletionHandler;
chip::Callback::Callback<chip::OnDeviceConnected> mOnConnected;
chip::Callback::Callback<chip::OnDeviceConnectionFailure> mOnConnectFailed;
chip::Callback::Callback<chip::OperationalSessionSetup::OnSetupFailure> 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
10 changes: 7 additions & 3 deletions src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.mm
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,17 @@
void * context, chip::Messaging::ExchangeManager & exchangeMgr, const chip::SessionHandle & sessionHandle)
{
auto * object = static_cast<MTRDeviceConnectionBridge *>(context);
object->mCompletionHandler(&exchangeMgr, chip::MakeOptional<chip::SessionHandle>(*sessionHandle->AsSecureSession()), nil);
object->mCompletionHandler(&exchangeMgr, chip::MakeOptional<chip::SessionHandle>(*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<double>(failureInfo.requestedBusyDelay.Value().count()) / MSEC_PER_SEC);
}
auto * object = static_cast<MTRDeviceConnectionBridge *>(context);
object->mCompletionHandler(nil, chip::NullOptional, [MTRError errorForCHIPErrorCode:error]);
object->mCompletionHandler(nil, chip::NullOptional, [MTRError errorForCHIPErrorCode:failureInfo.error], retryDelay);
object->Release();
}
12 changes: 6 additions & 6 deletions src/darwin/Framework/CHIP/MTRDeviceController.mm
Original file line number Diff line number Diff line change
Expand Up @@ -1205,7 +1205,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);
}];
}

Expand All @@ -1216,20 +1216,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<chip::SessionHandle> 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);
}];
}

Expand Down Expand Up @@ -1609,7 +1609,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<chip::SessionHandle> & session, NSError * _Nullable error) {
const chip::Optional<chip::SessionHandle> & 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.
Expand Down

0 comments on commit ee6c9e9

Please sign in to comment.