Only update fetch throttle interval for server errors (#7901)

firebase · Apr 15, 2021 · a1732da · a1732da
1 parent 962407b
commit a1732da
Show file tree

Hide file tree

Showing 5 changed files with 87 additions and 6 deletions.
diff --git a/FirebaseRemoteConfig/CHANGELOG.md b/FirebaseRemoteConfig/CHANGELOG.md
@@ -1,3 +1,6 @@
+# Unreleased
+- [fixed] Fixed throttling issue when fetch fails due to no network. (#6628)
+
 # v7.10.0
 - [changed] Throw exception if projectID is missing from FirebaseOptions. (#7725)
 

diff --git a/FirebaseRemoteConfig/Sources/Private/RCNConfigSettings.h b/FirebaseRemoteConfig/Sources/Private/RCNConfigSettings.h
@@ -114,6 +114,10 @@
 /// @param fetchSuccess True if fetch was successful.
 - (void)updateMetadataWithFetchSuccessStatus:(BOOL)fetchSuccess;
 
+/// Increases the throttling time. Should only be called if the fetch error indicates a server
+/// issue.
+- (void)updateExponentialBackoffTime;
+
 /// Returns true if we are in exponential backoff mode and it is not yet the next request time.
 - (BOOL)shouldThrottle;
 

diff --git a/FirebaseRemoteConfig/Sources/RCNConfigFetch.m b/FirebaseRemoteConfig/Sources/RCNConfigFetch.m
@@ -387,6 +387,7 @@ - (void)fetchWithUserProperties:(NSDictionary *)userProperties
               statusCode == kRCNFetchResponseHTTPStatusCodeInternalError ||
               statusCode == kRCNFetchResponseHTTPStatusCodeServiceUnavailable ||
               statusCode == kRCNFetchResponseHTTPStatusCodeGatewayTimeout) {
+            [strongSelf->_settings updateExponentialBackoffTime];
             if ([strongSelf->_settings shouldThrottle]) {
               // Must set lastFetchStatus before FailReason.
               strongSelf->_settings.lastFetchStatus = FIRRemoteConfigFetchStatusThrottled;
@@ -404,8 +405,8 @@ - (void)fetchWithUserProperties:(NSDictionary *)userProperties
                                                 withStatus:strongSelf->_settings.lastFetchStatus
                                                  withError:error];
             }
-          }  // Response error code 429, 500, 503
-        }    // StatusCode != kRCNFetchResponseHTTPStatusCodeOK
+          }
+        }
         // Return back the received error.
         // Must set lastFetchStatus before setting Fetch Error.
         strongSelf->_settings.lastFetchStatus = FIRRemoteConfigFetchStatusFailure;

diff --git a/FirebaseRemoteConfig/Sources/RCNConfigSettings.m b/FirebaseRemoteConfig/Sources/RCNConfigSettings.m
@@ -234,10 +234,6 @@ - (void)updateExponentialBackoffTime {
 
 - (void)updateMetadataWithFetchSuccessStatus:(BOOL)fetchSuccess {
   FIRLogDebug(kFIRLoggerRemoteConfig, @"I-RCN000056", @"Updating metadata with fetch result.");
-  if (!fetchSuccess) {
-    [self updateExponentialBackoffTime];
-  }
-
   [self updateFetchTimeWithSuccessFetch:fetchSuccess];
   _lastFetchStatus =
       fetchSuccess ? FIRRemoteConfigFetchStatusSuccess : FIRRemoteConfigFetchStatusFailure;

diff --git a/FirebaseRemoteConfig/Tests/Unit/RCNRemoteConfigTest.m b/FirebaseRemoteConfig/Tests/Unit/RCNRemoteConfigTest.m
@@ -733,6 +733,83 @@ __unsafe_unretained void (^handler)(FIRRemoteConfigFetchStatus status,
                                }];
 }
 
+- (void)testFetchFailedNoNetworkErrorDoesNotThrottle {
+  RCNConfigContent *configContent = [[RCNConfigContent alloc] initWithDBManager:_DBManager];
+  NSString *currentAppName = RCNTestsDefaultFIRAppName;
+  FIROptions *currentOptions = [self firstAppOptions];
+  NSString *currentNamespace = RCNTestsFIRNamespace;
+  NSString *fullyQualifiedNamespace =
+      [NSString stringWithFormat:@"%@:%@", currentNamespace, currentAppName];
+
+  RCNUserDefaultsManager *userDefaultsManager =
+      [[RCNUserDefaultsManager alloc] initWithAppName:currentAppName
+                                             bundleID:[NSBundle mainBundle].bundleIdentifier
+                                            namespace:fullyQualifiedNamespace];
+  userDefaultsManager.lastFetchTime = 0;
+
+  FIRRemoteConfig *config = OCMPartialMock([[FIRRemoteConfig alloc] initWithAppName:currentAppName
+                                                                         FIROptions:currentOptions
+                                                                          namespace:currentNamespace
+                                                                          DBManager:_DBManager
+                                                                      configContent:configContent
+                                                                          analytics:nil]);
+  RCNConfigSettings *settings =
+      [[RCNConfigSettings alloc] initWithDatabaseManager:_DBManager
+                                               namespace:fullyQualifiedNamespace
+                                         firebaseAppName:currentAppName
+                                             googleAppID:currentOptions.googleAppID];
+  dispatch_queue_t queue = dispatch_queue_create(
+      [[NSString stringWithFormat:@"testqueue"] cStringUsingEncoding:NSUTF8StringEncoding],
+      DISPATCH_QUEUE_SERIAL);
+  RCNConfigFetch *configFetch =
+      OCMPartialMock([[RCNConfigFetch alloc] initWithContent:configContent
+                                                   DBManager:_DBManager
+                                                    settings:settings
+                                                   analytics:nil
+                                                  experiment:nil
+                                                       queue:queue
+                                                   namespace:fullyQualifiedNamespace
+                                                     options:currentOptions]);
+
+  OCMStub([configFetch fetchConfigWithExpirationDuration:43200 completionHandler:OCMOCK_ANY])
+      .andDo(^(NSInvocation *invocation) {
+        __unsafe_unretained void (^handler)(FIRRemoteConfigFetchStatus status,
+                                            NSError *_Nullable error) = nil;
+        [invocation getArgument:&handler atIndex:3];
+        [configFetch fetchWithUserProperties:[[NSDictionary alloc] init] completionHandler:handler];
+      });
+  _responseData[0] = [NSJSONSerialization dataWithJSONObject:@{} options:0 error:nil];
+
+  // A no network error is accompanied with an HTTP status code of 0.
+  _URLResponse[0] =
+      [[NSHTTPURLResponse alloc] initWithURL:[NSURL URLWithString:@"https://firebase.com"]
+                                  statusCode:0
+                                 HTTPVersion:nil
+                                headerFields:@{@"etag" : @"etag1"}];
+  [config updateWithNewInstancesForConfigFetch:configFetch
+                                 configContent:configContent
+                                configSettings:settings
+                              configExperiment:nil];
+
+  XCTestExpectation *expectation =
+      [self expectationWithDescription:@"Network error doesn't increase throttle interval"];
+  XCTAssertEqual(config.lastFetchStatus, FIRRemoteConfigFetchStatusNoFetchYet);
+
+  FIRRemoteConfigFetchCompletion fetchCompletion =
+      ^void(FIRRemoteConfigFetchStatus status, NSError *error) {
+        XCTAssertEqual(config.lastFetchStatus, FIRRemoteConfigFetchStatusFailure);
+        XCTAssertEqual(settings.exponentialBackoffRetryInterval, 0);
+        [expectation fulfill];
+      };
+
+  [config fetchWithExpirationDuration:43200 completionHandler:fetchCompletion];
+
+  [self waitForExpectationsWithTimeout:_expectationTimeout
+                               handler:^(NSError *error) {
+                                 XCTAssertNil(error);
+                               }];
+}
+
 // Activate should return false if a fetch response returns 200 with NO_CHANGE as the response body.
 - (void)testActivateOnFetchNoChangeStatus {
   // Override the setup values to return back an error status.