MTRDevice should retry commands if it gets a BUSY response. (#29830)

Since this can delay command invocation, accounts for that in the timed invoke case.
project-chip · Oct 18, 2023 · 752046c · 752046c
1 parent 4c205a2
commit 752046c
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 7 deletions.
diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm
@@ -1112,6 +1112,11 @@ - (void)_invokeCommandWithEndpointID:(NSNumber *)endpointID
  serverSideProcessingTimeout = [serverSideProcessingTimeout copy];
  timeout = [timeout copy];
 
+ NSDate * cutoffTime;
+ if (timeout) {
+ cutoffTime = [NSDate dateWithTimeIntervalSinceNow:(timeout.doubleValue / 1000)];
+ }
+
  uint64_t expectedValueID = 0;
  NSMutableArray<MTRAttributePath *> * attributePaths = nil;
  if (expectedValues) {
@@ -1130,26 +1135,51 @@ - (void)_invokeCommandWithEndpointID:(NSNumber *)endpointID
  *stop = YES;
  }];
  [workItem setReadyHandler:^(MTRDevice * device, NSInteger retryCount, MTRAsyncWorkCompletionBlock workCompletion) {
+ auto workDone = ^(NSArray<NSDictionary<NSString *, id> *> * _Nullable values, NSError * _Nullable error) {
+ dispatch_async(queue, ^{
+ completion(values, error);
+ });
+ if (error && expectedValues) {
+ [self removeExpectedValuesForAttributePaths:attributePaths expectedValueID:expectedValueID];
+ }
+ workCompletion(MTRAsyncWorkComplete);
+ };
+
+ NSNumber * timedInvokeTimeout = nil;
+ if (timeout) {
+ auto * now = [NSDate now];
+ if ([now compare:cutoffTime] == NSOrderedDescending) {
+ // Our timed invoke timeout has expired already. Command
+ // was queued for too long. Do not send it out.
+ workDone(nil, [MTRError errorForIMStatusCode:Status::Timeout]);
+ return;
+ }
+
+ // Recompute the actual timeout left, accounting for time spent
+ // in our queuing and retries.
+ timedInvokeTimeout = @([cutoffTime timeIntervalSinceDate:now] * 1000);
+ }
  MTRBaseDevice * baseDevice = [self newBaseDevice];
  [baseDevice
  _invokeCommandWithEndpointID:endpointID
  clusterID:clusterID
  commandID:commandID
  commandFields:commandFields
- timedInvokeTimeout:timeout
+ timedInvokeTimeout:timedInvokeTimeout
  serverSideProcessingTimeout:serverSideProcessingTimeout
  queue:self.queue
  completion:^(NSArray<NSDictionary<NSString *, id> *> * _Nullable values, NSError * _Nullable error) {
  // Log the data at the INFO level (not usually persisted permanently),
  // but make sure we log the work completion at the DEFAULT level.
  MTR_LOG_INFO("Invoke work item [%llu] received command response: %@ error: %@", workItemID, values, error);
- dispatch_async(queue, ^{
-  completion(values, error);
- });
- if (error && expectedValues) {
- [self removeExpectedValuesForAttributePaths:attributePaths expectedValueID:expectedValueID];
+ // TODO: This 5-retry cap is very arbitrary.
+ // TODO: Should there be some sort of backoff here?
+ if (error != nil && error.domain == MTRInteractionErrorDomain && error.code == MTRInteractionErrorCodeBusy && retryCount < 5) {
+  workCompletion(MTRAsyncWorkNeedsRetry);
+ return;
  }
- workCompletion(MTRAsyncWorkComplete);
+
+ workDone(values, error);
  }];
  }];
  [_asyncWorkQueue enqueueWorkItem:workItem descriptionWithFormat:@"invoke %@ %@ %@", endpointID, clusterID, commandID];

diff --git a/src/darwin/Framework/CHIP/MTRError.mm b/src/darwin/Framework/CHIP/MTRError.mm
@@ -232,6 +232,11 @@ + (NSError *)errorForIMStatus:(const chip::app::StatusIB &)status
  return [NSError errorWithDomain:MTRInteractionErrorDomain code:chip::to_underlying(status.mStatus) userInfo:userInfo];
 }
 
++ (NSError *)errorForIMStatusCode:(chip::Protocols::InteractionModel::Status)status
+{
+ return [self errorForIMStatus:chip::app::StatusIB(status)];
+}
+
 + (CHIP_ERROR)errorToCHIPErrorCode:(NSError * _Nullable)error
 {
  if (error == nil) {

diff --git a/src/darwin/Framework/CHIP/MTRError_Internal.h b/src/darwin/Framework/CHIP/MTRError_Internal.h
@@ -21,13 +21,15 @@
 
 #include <app/MessageDef/StatusIB.h>
 #include <lib/core/CHIPError.h>
+#include <protocols/interaction_model/StatusCode.h>
 
 NS_ASSUME_NONNULL_BEGIN
 
 MTR_HIDDEN
 @interface MTRError : NSObject
 + (NSError * _Nullable)errorForCHIPErrorCode:(CHIP_ERROR)errorCode;
 + (NSError * _Nullable)errorForIMStatus:(const chip::app::StatusIB &)status;
++ (NSError * _Nullable)errorForIMStatusCode:(chip::Protocols::InteractionModel::Status)status;
 + (CHIP_ERROR)errorToCHIPErrorCode:(NSError * _Nullable)error;
 @end