Implemented exponential backoff with query (#6653)

.changeset/large-lamps-reflect.md

@@ -0,0 +1,6 @@
+---
+"@firebase/storage": patch
+---
+
+Fixed bug where upload status wasn't being checked after an upload failure.
+Implemented exponential backoff and max retry strategy.

common/api-review/storage.api.md

@@ -83,7 +83,7 @@ export class _FirebaseStorageImpl implements FirebaseStorage {
     // Warning: (ae-forgotten-export) The symbol "Request" needs to be exported by the entry point index.d.ts
     //
     // (undocumented)
-    _makeRequest<I extends ConnectionType, O>(requestInfo: RequestInfo_2<I, O>, requestFactory: () => Connection<I>, authToken: string | null, appCheckToken: string | null): Request_2<O>;
+    _makeRequest<I extends ConnectionType, O>(requestInfo: RequestInfo_2<I, O>, requestFactory: () => Connection<I>, authToken: string | null, appCheckToken: string | null, retry?: boolean): Request_2<O>;
     // (undocumented)
     makeRequestWithTokens<I extends ConnectionType, O>(requestInfo: RequestInfo_2<I, O>, requestFactory: () => Connection<I>): Promise<O>;
     _makeStorageReference(loc: _Location): _Reference;
@@ -319,11 +319,13 @@ export class _UploadTask {
     _blob: _FbsBlob;
     cancel(): boolean;
     catch<T>(onRejected: (p1: StorageError_2) => T | Promise<T>): Promise<T>;
+    // (undocumented)
+    isExponentialBackoffExpired(): boolean;
     // Warning: (ae-forgotten-export) The symbol "Metadata" needs to be exported by the entry point index.d.ts
     _metadata: Metadata | null;
     // Warning: (ae-forgotten-export) The symbol "Unsubscribe" needs to be exported by the entry point index.d.ts
     // Warning: (ae-forgotten-export) The symbol "Subscribe" needs to be exported by the entry point index.d.ts
-    on(type: _TaskEvent, nextOrObserver?: StorageObserver<UploadTaskSnapshot> | null | ((snapshot: UploadTaskSnapshot) => unknown), error?: ((a: StorageError_2) => unknown) | null, completed?: Unsubscribe_2 | null): Unsubscribe_2 | Subscribe_2<UploadTaskSnapshot>;
+    on(type: _TaskEvent, nextOrObserver?: StorageObserver<UploadTaskSnapshot> | null | ((snapshot: UploadTaskSnapshot) => unknown), error?: ((a: StorageError_2) => unknown) | null, completed?: CompleteFn | null): Unsubscribe_2 | Subscribe_2<UploadTaskSnapshot>;
     pause(): boolean;
     resume(): boolean;
     get snapshot(): UploadTaskSnapshot;

packages/storage/src/implementation/backoff.ts

@@ -24,15 +24,24 @@ type id = (p1: boolean) => void;
 export { id };
 
 /**
- * @param f May be invoked
- *     before the function returns.
- * @param callback Get all the arguments passed to the function
- *     passed to f, including the initial boolean.
+ * Accepts a callback for an action to perform (`doRequest`),
+ * and then a callback for when the backoff has completed (`backoffCompleteCb`).
+ * The callback sent to start requires an argument to call (`onRequestComplete`).
+ * When `start` calls `doRequest`, it passes a callback for when the request has
+ * completed, `onRequestComplete`. Based on this, the backoff continues, with
+ * another call to `doRequest` and the above loop continues until the timeout
+ * is hit, or a successful response occurs.
+ * @description
+ * @param doRequest Callback to perform request
+ * @param backoffCompleteCb Callback to call when backoff has been completed
  */
 export function start(
-  f: (p1: (success: boolean) => void, canceled: boolean) => void,
+  doRequest: (
+    onRequestComplete: (success: boolean) => void,
+    canceled: boolean
+  ) => void,
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  callback: (...args: any[]) => unknown,
+  backoffCompleteCb: (...args: any[]) => unknown,
   timeout: number
 ): id {
   // TODO(andysoto): make this code cleaner (probably refactor into an actual
@@ -55,14 +64,14 @@ export function start(
   function triggerCallback(...args: any[]): void {
     if (!triggeredCallback) {
       triggeredCallback = true;
-      callback.apply(null, args);
+      backoffCompleteCb.apply(null, args);
     }
   }
 
   function callWithDelay(millis: number): void {
     retryTimeoutId = setTimeout(() => {
       retryTimeoutId = null;
-      f(handler, canceled());
+      doRequest(responseHandler, canceled());
     }, millis);
   }
 
@@ -72,7 +81,7 @@ export function start(
     }
   }
 
-  function handler(success: boolean, ...args: any[]): void {
+  function responseHandler(success: boolean, ...args: any[]): void {
     if (triggeredCallback) {
       clearGlobalTimeout();
       return;

packages/storage/src/implementation/constants.ts

@@ -42,6 +42,11 @@ export const DEFAULT_MAX_OPERATION_RETRY_TIME = 2 * 60 * 1000;
  */
 export const DEFAULT_MAX_UPLOAD_RETRY_TIME = 10 * 60 * 1000;
 
+/**
+ * 1 second
+ */
+export const DEFAULT_MIN_SLEEP_TIME_MILLIS = 1000;
+
 /**
  * This is the value of Number.MIN_SAFE_INTEGER, which is not well supported
  * enough for us to use it directly.

packages/storage/src/implementation/error.ts

@@ -34,8 +34,9 @@ export class StorageError extends FirebaseError {
    * @param code - A StorageErrorCode string to be prefixed with 'storage/' and
    *  added to the end of the message.
    * @param message  - Error message.
+   * @param status_ - Corresponding HTTP Status Code
    */
-  constructor(code: StorageErrorCode, message: string) {
+  constructor(code: StorageErrorCode, message: string, private status_ = 0) {
     super(
       prependCode(code),
       `Firebase Storage: ${message} (${prependCode(code)})`
@@ -46,6 +47,14 @@ export class StorageError extends FirebaseError {
     Object.setPrototypeOf(this, StorageError.prototype);
   }
 
+  get status(): number {
+    return this.status_;
+  }
+
+  set status(status: number) {
+    this.status_ = status;
+  }
+
   /**
    * Compares a StorageErrorCode against this error's code, filtering out the prefix.
    */

packages/storage/src/implementation/request.ts

@@ -26,6 +26,7 @@ import { ErrorHandler, RequestHandler, RequestInfo } from './requestinfo';
 import { isJustDef } from './type';
 import { makeQueryString } from './url';
 import { Connection, ErrorCode, Headers, ConnectionType } from './connection';
+import { isRetryStatusCode } from './utils';
 
 export interface Request<T> {
   getPromise(): Promise<T>;
@@ -69,7 +70,8 @@ class NetworkRequest<I extends ConnectionType, O> implements Request<O> {
     private errorCallback_: ErrorHandler | null,
     private timeout_: number,
     private progressCallback_: ((p1: number, p2: number) => void) | null,
-    private connectionFactory_: () => Connection<I>
+    private connectionFactory_: () => Connection<I>,
+    private retry = true
   ) {
     this.promise_ = new Promise((resolve, reject) => {
       this.resolve_ = resolve as (value?: O | PromiseLike<O>) => void;
@@ -93,16 +95,15 @@ class NetworkRequest<I extends ConnectionType, O> implements Request<O> {
       const connection = this.connectionFactory_();
       this.pendingConnection_ = connection;
 
-      const progressListener: (progressEvent: ProgressEvent) => void =
-        progressEvent => {
-          const loaded = progressEvent.loaded;
-          const total = progressEvent.lengthComputable
-            ? progressEvent.total
-            : -1;
-          if (this.progressCallback_ !== null) {
-            this.progressCallback_(loaded, total);
-          }
-        };
+      const progressListener: (
+        progressEvent: ProgressEvent
+      ) => void = progressEvent => {
+        const loaded = progressEvent.loaded;
+        const total = progressEvent.lengthComputable ? progressEvent.total : -1;
+        if (this.progressCallback_ !== null) {
+          this.progressCallback_(loaded, total);
+        }
+      };
       if (this.progressCallback_ !== null) {
         connection.addUploadProgressListener(progressListener);
       }
@@ -118,7 +119,11 @@ class NetworkRequest<I extends ConnectionType, O> implements Request<O> {
           this.pendingConnection_ = null;
           const hitServer = connection.getErrorCode() === ErrorCode.NO_ERROR;
           const status = connection.getStatus();
-          if (!hitServer || this.isRetryStatusCode_(status)) {
+          if (
+            (!hitServer ||
+              isRetryStatusCode(status, this.additionalRetryCodes_)) &&
+            this.retry
+          ) {
             const wasCanceled = connection.getErrorCode() === ErrorCode.ABORT;
             backoffCallback(
               false,
@@ -196,22 +201,6 @@ class NetworkRequest<I extends ConnectionType, O> implements Request<O> {
       this.pendingConnection_.abort();
     }
   }
-
-  private isRetryStatusCode_(status: number): boolean {
-    // The codes for which to retry came from this page:
-    // https://cloud.google.com/storage/docs/exponential-backoff
-    const isFiveHundredCode = status >= 500 && status < 600;
-    const extraRetryCodes = [
-      // Request Timeout: web server didn't receive full request in time.
-      408,
-      // Too Many Requests: you're getting rate-limited, basically.
-      429
-    ];
-    const isExtraRetryCode = extraRetryCodes.indexOf(status) !== -1;
-    const isRequestSpecificRetryCode =
-      this.additionalRetryCodes_.indexOf(status) !== -1;
-    return isFiveHundredCode || isExtraRetryCode || isRequestSpecificRetryCode;
-  }
 }
 
 /**
@@ -271,7 +260,8 @@ export function makeRequest<I extends ConnectionType, O>(
   authToken: string | null,
   appCheckToken: string | null,
   requestFactory: () => Connection<I>,
-  firebaseVersion?: string
+  firebaseVersion?: string,
+  retry = true
 ): Request<O> {
   const queryPart = makeQueryString(requestInfo.urlParams);
   const url = requestInfo.url + queryPart;
@@ -291,6 +281,7 @@ export function makeRequest<I extends ConnectionType, O>(
     requestInfo.errorHandler,
     requestInfo.timeout,
     requestInfo.progressCallback,
-    requestFactory
+    requestFactory,
+    retry
   );
 }

packages/storage/src/implementation/requests.ts

@@ -104,7 +104,7 @@ export function sharedErrorHandler(
     xhr: Connection<ConnectionType>,
     err: StorageError
   ): StorageError {
-    let newErr;
+    let newErr: StorageError;
     if (xhr.getStatus() === 401) {
       if (
         // This exact message string is the only consistent part of the
@@ -126,6 +126,7 @@ export function sharedErrorHandler(
         }
       }
     }
+    newErr.status = xhr.getStatus();
     newErr.serverResponse = err.serverResponse;
     return newErr;
   }
@@ -534,8 +535,14 @@ export function continueResumableUpload(
   }
   const startByte = status_.current;
   const endByte = startByte + bytesToUpload;
-  const uploadCommand =
-    bytesToUpload === bytesLeft ? 'upload, finalize' : 'upload';
+  let uploadCommand = '';
+  if (bytesToUpload === 0) {
+    uploadCommand = 'finalize';
+  } else if (bytesLeft === bytesToUpload) {
+    uploadCommand = 'upload, finalize';
+  } else {
+    uploadCommand = 'upload';
+  }
   const headers = {
     'X-Goog-Upload-Command': uploadCommand,
     'X-Goog-Upload-Offset': `${status_.current}`

packages/storage/src/implementation/utils.ts

@@ -0,0 +1,40 @@
+/**
+ * @license
+ * Copyright 2022 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * Checks the status code to see if the action should be retried.
+ *
+ * @param status Current HTTP status code returned by server.
+ * @param additionalRetryCodes additional retry codes to check against
+ */
+export function isRetryStatusCode(
+  status: number,
+  additionalRetryCodes: number[]
+): boolean {
+  // The codes for which to retry came from this page:
+  // https://cloud.google.com/storage/docs/exponential-backoff
+  const isFiveHundredCode = status >= 500 && status < 600;
+  const extraRetryCodes = [
+    // Request Timeout: web server didn't receive full request in time.
+    408,
+    // Too Many Requests: you're getting rate-limited, basically.
+    429
+  ];
+  const isExtraRetryCode = extraRetryCodes.indexOf(status) !== -1;
+  const isAdditionalRetryCode = additionalRetryCodes.indexOf(status) !== -1;
+  return isFiveHundredCode || isExtraRetryCode || isAdditionalRetryCode;
+}

packages/storage/src/service.ts

@@ -302,7 +302,8 @@ export class FirebaseStorageImpl implements FirebaseStorage {
     requestInfo: RequestInfo<I, O>,
     requestFactory: () => Connection<I>,
     authToken: string | null,
-    appCheckToken: string | null
+    appCheckToken: string | null,
+    retry = true
   ): Request<O> {
     if (!this._deleted) {
       const request = makeRequest(
@@ -311,7 +312,8 @@ export class FirebaseStorageImpl implements FirebaseStorage {
         authToken,
         appCheckToken,
         requestFactory,
-        this._firebaseVersion
+        this._firebaseVersion,
+        retry
       );
       this._requests.add(request);
       // Request removes itself from set when complete.