[Storage] Cleared timeouts upon pause/resume of uploads (#6667)

Author: maneesht

.changeset/great-houses-trade.md

@@ -0,0 +1,5 @@
+---
+"@firebase/storage": patch
+---
+
+Cleared retry timeouts when uploads are paused/canceled

packages/storage/src/task.ts

@@ -94,6 +94,7 @@ export class UploadTask {
   private _metadataErrorHandler: (p1: StorageError) => void;
   private _resolve?: (p1: UploadTaskSnapshot) => void = undefined;
   private _reject?: (p1: StorageError) => void = undefined;
+  private pendingTimeout?: ReturnType<typeof setTimeout>;
   private _promise: Promise<UploadTaskSnapshot>;
 
   private sleepTime: number;
@@ -191,7 +192,8 @@ export class UploadTask {
             // Happens if we miss the metadata on upload completion.
             this._fetchMetadata();
           } else {
-            setTimeout(() => {
+            this.pendingTimeout = setTimeout(() => {
+              this.pendingTimeout = undefined;
               this._continueUpload();
             }, this.sleepTime);
           }
@@ -405,20 +407,17 @@ export class UploadTask {
     }
     switch (state) {
       case InternalTaskState.CANCELING:
+      case InternalTaskState.PAUSING:
         // TODO(andysoto):
         // assert(this.state_ === InternalTaskState.RUNNING ||
         //        this.state_ === InternalTaskState.PAUSING);
         this._state = state;
         if (this._request !== undefined) {
           this._request.cancel();
-        }
-        break;
-      case InternalTaskState.PAUSING:
-        // TODO(andysoto):
-        // assert(this.state_ === InternalTaskState.RUNNING);
-        this._state = state;
-        if (this._request !== undefined) {
-          this._request.cancel();
+        } else if (this.pendingTimeout) {
+          clearTimeout(this.pendingTimeout);
+          this.pendingTimeout = undefined;
+          this.completeTransitions_();
         }
         break;
       case InternalTaskState.RUNNING:

packages/storage/test/unit/task.test.ts

@@ -14,7 +14,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import { assert, expect } from 'chai';
+import { assert, expect, use } from 'chai';
 import { FbsBlob } from '../../src/implementation/blob';
 import { Location } from '../../src/implementation/location';
 import { Unsubscribe } from '../../src/implementation/observer';
@@ -31,8 +31,13 @@ import {
 } from './testshared';
 import { injectTestConnection } from '../../src/platform/connection';
 import { Deferred } from '@firebase/util';
-import { retryLimitExceeded } from '../../src/implementation/error';
+import { canceled, retryLimitExceeded } from '../../src/implementation/error';
 import { SinonFakeTimers, useFakeTimers } from 'sinon';
+import * as sinon from 'sinon';
+import sinonChai from 'sinon-chai';
+import { DEFAULT_MAX_UPLOAD_RETRY_TIME } from '../../src/implementation/constants';
+
+use(sinonChai);
 
 const testLocation = new Location('bucket', 'object');
 const smallBlob = new FbsBlob(new Uint8Array([97]));
@@ -361,7 +366,7 @@ describe('Firebase Storage > Upload Task', () => {
   function handleStateChange(
     requestHandler: RequestHandler,
     blob: FbsBlob
-  ): Promise<TotalState> {
+  ): { taskPromise: Promise<TotalState>; task: UploadTask } {
     const storageService = storageServiceWithHandler(requestHandler);
     const task = new UploadTask(
       new Reference(storageService, testLocation),
@@ -410,7 +415,7 @@ describe('Firebase Storage > Upload Task', () => {
       }
     );
 
-    return deferred.promise;
+    return { taskPromise: deferred.promise, task };
   }
 
   it('Calls callback sequences for small uploads correctly', () => {
@@ -422,13 +427,13 @@ describe('Firebase Storage > Upload Task', () => {
   it('properly times out if large blobs returns a 503 when finalizing', async () => {
     clock = useFakeTimers();
     // Kick off upload
-    const promise = handleStateChange(
+    const { taskPromise } = handleStateChange(
       fake503ForFinalizeServerHandler(),
       bigBlob
     );
     // Run all timers
     await clock.runAllAsync();
-    const { events, progress } = await promise;
+    const { events, progress } = await taskPromise;
     expect(events.length).to.equal(2);
     expect(events[0]).to.deep.equal({ type: 'resume' });
     expect(events[1].type).to.deep.equal('error');
@@ -460,10 +465,13 @@ describe('Firebase Storage > Upload Task', () => {
   it('properly times out if large blobs returns a 503 when uploading', async () => {
     clock = useFakeTimers();
     // Kick off upload
-    const promise = handleStateChange(fake503ForUploadServerHandler(), bigBlob);
+    const { taskPromise } = handleStateChange(
+      fake503ForUploadServerHandler(),
+      bigBlob
+    );
     // Run all timers
     await clock.runAllAsync();
-    const { events, progress } = await promise;
+    const { events, progress } = await taskPromise;
     expect(events.length).to.equal(2);
     expect(events[0]).to.deep.equal({ type: 'resume' });
     expect(events[1].type).to.deep.equal('error');
@@ -478,13 +486,122 @@ describe('Firebase Storage > Upload Task', () => {
     });
     clock.restore();
   });
+
+  /**
+   * Starts upload, finds the first instance of an exponential backoff, and resolves `readyToCancel` when done.
+   * @returns readyToCancel, taskPromise and task
+   */
+  function resumeCancelSetup(): {
+    readyToCancel: Promise<null>;
+    taskPromise: Promise<TotalState>;
+    task: UploadTask;
+  } {
+    clock = useFakeTimers();
+    const fakeSetTimeout = clock.setTimeout;
+
+    let gotFirstEvent = false;
+
+    const stub = sinon.stub(global, 'setTimeout');
+
+    // Function that notifies when we are in the middle of an exponential backoff
+    const readyToCancel = new Promise<null>(resolve => {
+      stub.callsFake((fn, timeout) => {
+        // @ts-ignore The types for `stub.callsFake` is incompatible with types of `clock.setTimeout`
+        const res = fakeSetTimeout(fn, timeout);
+        if (timeout !== DEFAULT_MAX_UPLOAD_RETRY_TIME) {
+          if (!gotFirstEvent || timeout === 0) {
+            clock.tick(timeout as number);
+          } else {
+            // If the timeout isn't 0 and it isn't the max upload retry time, it's most likely due to exponential backoff.
+            resolve(null);
+          }
+        }
+        return res;
+      });
+    });
+    readyToCancel.then(
+      () => stub.restore(),
+      () => stub.restore()
+    );
+    return {
+      ...handleStateChange(
+        fake503ForUploadServerHandler(undefined, () => (gotFirstEvent = true)),
+        bigBlob
+      ),
+      readyToCancel
+    };
+  }
+  it('properly errors with a cancel StorageError if a pending timeout remains', async () => {
+    // Kick off upload
+    const { readyToCancel, taskPromise: promise, task } = resumeCancelSetup();
+
+    await readyToCancel;
+    task.cancel();
+
+    const { events, progress } = await promise;
+    expect(events.length).to.equal(2);
+    expect(events[0]).to.deep.equal({ type: 'resume' });
+    expect(events[1].type).to.deep.equal('error');
+    const canceledError = canceled();
+    expect(events[1].data!.name).to.deep.equal(canceledError.name);
+    expect(events[1].data!.message).to.deep.equal(canceledError.message);
+    const blobSize = bigBlob.size();
+    expect(progress.length).to.equal(1);
+    expect(progress[0]).to.deep.equal({
+      bytesTransferred: 0,
+      totalBytes: blobSize
+    });
+    expect(clock.countTimers()).to.eq(0);
+    clock.restore();
+  });
+  it('properly errors with a pause StorageError if a pending timeout remains', async () => {
+    // Kick off upload
+    const { readyToCancel, taskPromise: promise, task } = resumeCancelSetup();
+
+    await readyToCancel;
+
+    task.pause();
+    expect(clock.countTimers()).to.eq(0);
+    task.resume();
+    await clock.runAllAsync();
+
+    // Run all timers
+    const { events, progress } = await promise;
+    expect(events.length).to.equal(4);
+    expect(events[0]).to.deep.equal({ type: 'resume' });
+    expect(events[1]).to.deep.equal({ type: 'pause' });
+    expect(events[2]).to.deep.equal({ type: 'resume' });
+    expect(events[3].type).to.deep.equal('error');
+    const retryError = retryLimitExceeded();
+    expect(events[3].data!.name).to.deep.equal(retryError.name);
+    expect(events[3].data!.message).to.deep.equal(retryError.message);
+    const blobSize = bigBlob.size();
+    expect(progress.length).to.equal(3);
+    expect(progress[0]).to.deep.equal({
+      bytesTransferred: 0,
+      totalBytes: blobSize
+    });
+    expect(progress[1]).to.deep.equal({
+      bytesTransferred: 0,
+      totalBytes: blobSize
+    });
+    expect(progress[2]).to.deep.equal({
+      bytesTransferred: 0,
+      totalBytes: blobSize
+    });
+    expect(clock.countTimers()).to.eq(0);
+    clock.restore();
+  });
   it('tests if small requests that respond with 500 retry correctly', async () => {
     clock = useFakeTimers();
     // Kick off upload
-    const promise = handleStateChange(fakeOneShot503ServerHandler(), smallBlob);
+    const { taskPromise } = handleStateChange(
+      fakeOneShot503ServerHandler(),
+      smallBlob
+    );
     // Run all timers
     await clock.runAllAsync();
-    const { events, progress } = await promise;
+    const { events, progress } = await taskPromise;
     expect(events.length).to.equal(2);
     expect(events[0]).to.deep.equal({ type: 'resume' });
     expect(events[1].type).to.deep.equal('error');

packages/storage/test/unit/testshared.ts

@@ -356,7 +356,7 @@ export function fakeServerHandler(
 
 /**
  * Responds with a 503 for finalize.
- * @param fakeMetadata metadata to respond with for query
+ * @param fakeMetadata metadata to respond with for finalize
  * @returns a handler for requests
  */
 export function fake503ForFinalizeServerHandler(
@@ -459,7 +459,8 @@ export function fake503ForFinalizeServerHandler(
  * @returns a handler for requests
  */
 export function fake503ForUploadServerHandler(
-  fakeMetadata: Partial<Metadata> = defaultFakeMetadata
+  fakeMetadata: Partial<Metadata> = defaultFakeMetadata,
+  cb?: () => void
 ): RequestHandler {
   const stats: {
     [num: number]: {
@@ -536,6 +537,9 @@ export function fake503ForUploadServerHandler(
     const isUpload = commands.indexOf('upload') !== -1;
 
     if (isUpload) {
+      if (cb) {
+        cb();
+      }
       return {
         status: 503,
         body: JSON.stringify(fakeMetadata),