Adjust Storage Timeouts to GTM's retry interval (#6791)

Author: schmidt-sebastian

FirebaseStorage/CHANGELOG.md

@@ -5,6 +5,8 @@
   that contain the "+" sign.
 - [changed] Renamed `list(withMaxResults:)` to `list(maxResults:)` in the Swift
   API.
+- [fixed] Fixed an issue that caused longer than expected timeouts for users
+  that specified custom timeouts.
 
 # 3.8.1
 - [fixed] Fixed typo in doc comments (#6485).

FirebaseStorage/Sources/FIRStorage.m

@@ -41,6 +41,9 @@
 @interface FIRStorage () {
   /// Stored Auth reference, if it exists. This needs to be stored for `copyWithZone:`.
   id<FIRAuthInterop> _Nullable _auth;
+  NSTimeInterval _maxUploadRetryTime;
+  NSTimeInterval _maxDownloadRetryTime;
+  NSTimeInterval _maxOperationRetryTime;
 }
 @end
 
@@ -154,8 +157,14 @@ - (instancetype)initWithApp:(FIRApp *)app
     _dispatchQueue = dispatch_queue_create("com.google.firebase.storage", DISPATCH_QUEUE_SERIAL);
     _fetcherServiceForApp = [FIRStorage fetcherServiceForApp:_app bucket:bucket auth:auth];
     _maxDownloadRetryTime = 600.0;
+    _maxDownloadRetryInterval =
+        [FIRStorageUtils computeRetryIntervalFromRetryTime:_maxDownloadRetryTime];
     _maxOperationRetryTime = 120.0;
+    _maxOperationRetryInterval =
+        [FIRStorageUtils computeRetryIntervalFromRetryTime:_maxOperationRetryTime];
     _maxUploadRetryTime = 600.0;
+    _maxUploadRetryInterval =
+        [FIRStorageUtils computeRetryIntervalFromRetryTime:_maxUploadRetryTime];
   }
   return self;
 }
@@ -205,6 +214,50 @@ - (NSString *)description {
   return [NSString stringWithFormat:@"%@ %p: %@", [self class], self, _app];
 }
 
+#pragma mark - Retry time intervals
+
+- (void)setMaxUploadRetryTime:(NSTimeInterval)maxUploadRetryTime {
+  @synchronized(self) {
+    _maxUploadRetryTime = maxUploadRetryTime;
+    _maxUploadRetryInterval =
+        [FIRStorageUtils computeRetryIntervalFromRetryTime:maxUploadRetryTime];
+  }
+}
+
+- (NSTimeInterval)maxDownloadRetryTime {
+  @synchronized(self) {
+    return _maxDownloadRetryTime;
+  }
+}
+
+- (void)setMaxDownloadRetryTime:(NSTimeInterval)maxDownloadRetryTime {
+  @synchronized(self) {
+    _maxDownloadRetryTime = maxDownloadRetryTime;
+    _maxDownloadRetryInterval =
+        [FIRStorageUtils computeRetryIntervalFromRetryTime:maxDownloadRetryTime];
+  }
+}
+
+- (NSTimeInterval)maxUploadRetryTime {
+  @synchronized(self) {
+    return _maxUploadRetryTime;
+  }
+}
+
+- (void)setMaxOperationRetryTime:(NSTimeInterval)maxOperationRetryTime {
+  @synchronized(self) {
+    _maxOperationRetryTime = maxOperationRetryTime;
+    _maxOperationRetryInterval =
+        [FIRStorageUtils computeRetryIntervalFromRetryTime:maxOperationRetryTime];
+  }
+}
+
+- (NSTimeInterval)maxOperationRetryTime {
+  @synchronized(self) {
+    return _maxOperationRetryTime;
+  }
+}
+
 #pragma mark - Public methods
 
 - (FIRStorageReference *)reference {
@@ -261,4 +314,5 @@ + (void)enableBackgroundTasks:(BOOL)isEnabled {
   [NSException raise:NSGenericException format:@"getDownloadTasks not implemented"];
   return nil;
 }
+
 @end

FirebaseStorage/Sources/FIRStorageDownloadTask.m

@@ -18,6 +18,7 @@
 #import "FirebaseStorage/Sources/FIRStorageDownloadTask_Private.h"
 #import "FirebaseStorage/Sources/FIRStorageObservableTask_Private.h"
 #import "FirebaseStorage/Sources/FIRStorageTask_Private.h"
+#import "FirebaseStorage/Sources/FIRStorage_Private.h"
 
 @implementation FIRStorageDownloadTask
 
@@ -80,7 +81,7 @@ - (void)enqueueWithData:(nullable NSData *)resumeData {
       }
     }];
 
-    fetcher.maxRetryInterval = strongSelf.reference.storage.maxDownloadRetryTime;
+    fetcher.maxRetryInterval = strongSelf.reference.storage.maxDownloadRetryInterval;
 
     if (strongSelf->_fileURL) {
       // Handle file downloads

FirebaseStorage/Sources/FIRStorageTask.m

@@ -45,7 +45,7 @@ - (instancetype)initWithReference:(FIRStorageReference *)reference
     _reference = reference;
     _baseRequest = [FIRStorageUtils defaultRequestForPath:reference.path];
     _fetcherService = service;
-    _fetcherService.maxRetryInterval = _reference.storage.maxOperationRetryTime;
+    _fetcherService.maxRetryInterval = _reference.storage.maxOperationRetryInterval;
     _dispatchQueue = queue;
   }
   return self;

FirebaseStorage/Sources/FIRStorageUploadTask.m

@@ -19,6 +19,7 @@
 #import "FirebaseStorage/Sources/FIRStorageObservableTask_Private.h"
 #import "FirebaseStorage/Sources/FIRStorageTask_Private.h"
 #import "FirebaseStorage/Sources/FIRStorageUploadTask_Private.h"
+#import "FirebaseStorage/Sources/FIRStorage_Private.h"
 
 #if SWIFT_PACKAGE
 @import GTMSessionFetcherCore;
@@ -128,7 +129,7 @@ - (void)enqueue {
       uploadFetcher.comment = @"File UploadTask";
     }
 
-    uploadFetcher.maxRetryInterval = self.reference.storage.maxUploadRetryTime;
+    uploadFetcher.maxRetryInterval = self.reference.storage.maxUploadRetryInterval;
 
     [uploadFetcher setSendProgressBlock:^(int64_t bytesSent, int64_t totalBytesSent,
                                           int64_t totalBytesExpectedToSend) {

FirebaseStorage/Sources/FIRStorageUtils.h

@@ -84,6 +84,16 @@ NS_ASSUME_NONNULL_BEGIN
  */
 + (NSError *)storageErrorWithDescription:(NSString *)description code:(NSInteger)code;
 
+/**
+ * Performs a crude translation of the user provided timeouts to the retry intervals that
+ * GTMSessionFetcher accepts. GTMSessionFetcher times out operations if the time between individual
+ * retry attempts exceed a certain threshold, while our API contract looks at the total observed
+ * time of the operation (i.e. the sum of all retries).
+ * @param retryTime A timeout that caps the sum of all retry attempts
+ * @return A timeout that caps the timeout of the last retry attempt
+ */
++ (NSTimeInterval)computeRetryIntervalFromRetryTime:(NSTimeInterval)retryTime;
+
 @end
 
 @interface NSDictionary (FIRStorageNSDictionaryJSONHelpers)

FirebaseStorage/Sources/FIRStorageUtils.m

@@ -134,6 +134,23 @@ + (NSError *)storageErrorWithDescription:(NSString *)description code:(NSInteger
                          userInfo:@{NSLocalizedDescriptionKey : description}];
 }
 
++ (NSTimeInterval)computeRetryIntervalFromRetryTime:(NSTimeInterval)retryTime {
+  // GTMSessionFetcher's retry starts at 1 second and then doubles every time. We use this
+  // information to compute a best-effort estimate of what to translate the user provided retry
+  // time into.
+
+  // Note that this is the same as 2 << (log2(retryTime) - 1), but deemed more readable.
+  NSTimeInterval lastInterval = 1.0;
+  NSTimeInterval sumOfAllIntervals = 1.0;
+
+  while (sumOfAllIntervals < retryTime) {
+    lastInterval *= 2;
+    sumOfAllIntervals += lastInterval;
+  }
+
+  return lastInterval;
+}
+
 @end
 
 @implementation NSDictionary (FIRStorageNSDictionaryJSONHelpers)

FirebaseStorage/Sources/FIRStorage_Private.h

@@ -29,6 +29,27 @@ NS_ASSUME_NONNULL_BEGIN
 
 @property(strong, nonatomic) NSString *storageBucket;
 
+/**
+ * Maximum time between retry attempts for uploads.
+ *
+ * This is used by GTMSessionFetcher and translated from the user provided `maxUploadRetryTime`.
+ */
+@property(assign, nonatomic) NSTimeInterval maxUploadRetryInterval;
+
+/**
+ * Maximum time between retry attempts for downloads.
+ *
+ * This is used by GTMSessionFetcher and translated from the user provided `maxDownloadRetryTime`.
+ */
+@property(assign, nonatomic) NSTimeInterval maxDownloadRetryInterval;
+
+/**
+ * Maximum time between retry attempts for any operation that is not an upload or download.
+ *
+ * This is used by GTMSessionFetcher and translated from the user provided `maxOperationRetryTime`.
+ */
+@property(assign, nonatomic) NSTimeInterval maxOperationRetryInterval;
+
 /**
  * Enables/disables GTMSessionFetcher HTTP logging
  * @param isLoggingEnabled Boolean passed through to enable/disable GTMSessionFetcher logging

FirebaseStorage/Tests/Unit/FIRStorageUtilsTests.m

@@ -122,4 +122,24 @@ - (void)testEncodedURLForNoPath {
   XCTAssertEqualObjects(encodedURL, @"/v0/b/bucket/o");
 }
 
+- (void)testTranslateRetryTime {
+  // The 1st retry attempt runs after 1 second.
+  // The 2nd retry attempt is delayed by 2 seconds (3s total)
+  // The 3rd retry attempt is delayed by 4 seconds (7s total)
+  // The 4th retry attempt is delayed by 8 seconds (15s total)
+  // The 5th retry attempt is delayed by 16 seconds (31s total)
+  // The 6th retry attempt is delayed by 32 seconds (63s total)
+  // Thus, we should exit just between the 5th and 6th retry attempt and cut off before 32s.
+  XCTAssertEqual(32.0, [FIRStorageUtils computeRetryIntervalFromRetryTime:60.0]);
+
+  XCTAssertEqual(1.0, [FIRStorageUtils computeRetryIntervalFromRetryTime:1.0]);
+  XCTAssertEqual(2.0, [FIRStorageUtils computeRetryIntervalFromRetryTime:2.0]);
+  XCTAssertEqual(4.0, [FIRStorageUtils computeRetryIntervalFromRetryTime:4.0]);
+  XCTAssertEqual(8.0, [FIRStorageUtils computeRetryIntervalFromRetryTime:10.0]);
+  XCTAssertEqual(16.0, [FIRStorageUtils computeRetryIntervalFromRetryTime:20.0]);
+  XCTAssertEqual(16.0, [FIRStorageUtils computeRetryIntervalFromRetryTime:30.0]);
+  XCTAssertEqual(32.0, [FIRStorageUtils computeRetryIntervalFromRetryTime:40.0]);
+  XCTAssertEqual(32.0, [FIRStorageUtils computeRetryIntervalFromRetryTime:50.0]);
+}
+
 @end