From d8bb500bf3a8c5332cb565e58664c7f6dedd0a7d Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Mon, 9 Jan 2023 16:15:13 -0800 Subject: [PATCH 01/15] Fixed issue where pause didn't work --- .../storage/src/implementation/request.ts | 6 ++-- .../test/integration/integration.test.ts | 29 +++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/packages/storage/src/implementation/request.ts b/packages/storage/src/implementation/request.ts index 572562d1528..fae46d7a5ab 100644 --- a/packages/storage/src/implementation/request.ts +++ b/packages/storage/src/implementation/request.ts @@ -120,9 +120,9 @@ class NetworkRequest implements Request { const hitServer = connection.getErrorCode() === ErrorCode.NO_ERROR; const status = connection.getStatus(); if ( - (!hitServer || - isRetryStatusCode(status, this.additionalRetryCodes_)) && - this.retry + !hitServer || + (isRetryStatusCode(status, this.additionalRetryCodes_) && + this.retry) ) { const wasCanceled = connection.getErrorCode() === ErrorCode.ABORT; backoffCallback( diff --git a/packages/storage/test/integration/integration.test.ts b/packages/storage/test/integration/integration.test.ts index 27d285b2709..7ecb37f45d5 100644 --- a/packages/storage/test/integration/integration.test.ts +++ b/packages/storage/test/integration/integration.test.ts @@ -36,6 +36,8 @@ import { import { use, expect } from 'chai'; import chaiAsPromised from 'chai-as-promised'; import * as types from '../../src/public-types'; +import { waitFor } from '../../../database/test/helpers/util'; +import { Deferred } from '@firebase/util'; use(chaiAsPromised); @@ -187,4 +189,31 @@ describe('FirebaseStorage Exp', () => { expect(listResult.items.map(v => v.name)).to.have.members(['a', 'b']); expect(listResult.prefixes.map(v => v.name)).to.have.members(['c']); }); + + it('can pause uploads without an error', async () => { + const referenceA = ref(storage, 'public/exp-upload/a'); + const bytesToUpload = new ArrayBuffer(1024 * 1024); + const task = uploadBytesResumable(referenceA, bytesToUpload); + const failureDeferred = new Deferred(); + let hasPaused = false; + task.on( + 'state_changed', + snapshot => { + const p = [snapshot.bytesTransferred, snapshot.totalBytes]; + if (snapshot.bytesTransferred > 0 && !hasPaused) { + console.log('pausing'); + task.pause(); + hasPaused = true; + } + }, + () => { + failureDeferred.reject('Failed to upload file'); + } + ); + await Promise.race([failureDeferred.promise, waitFor(5000)]); + task.resume(); + await task; + const bytes = await getBytes(referenceA); + expect(bytes).to.deep.eq(bytesToUpload); + }); }); From 90cce8618d77034684e38fab842151aa66d54ebc Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Mon, 9 Jan 2023 16:16:29 -0800 Subject: [PATCH 02/15] Removed console log --- packages/storage/test/integration/integration.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/storage/test/integration/integration.test.ts b/packages/storage/test/integration/integration.test.ts index 7ecb37f45d5..b6e21609bb9 100644 --- a/packages/storage/test/integration/integration.test.ts +++ b/packages/storage/test/integration/integration.test.ts @@ -201,7 +201,6 @@ describe('FirebaseStorage Exp', () => { snapshot => { const p = [snapshot.bytesTransferred, snapshot.totalBytes]; if (snapshot.bytesTransferred > 0 && !hasPaused) { - console.log('pausing'); task.pause(); hasPaused = true; } From 95be1ef5c44e98d8896df8c40fa671813203ac9a Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Mon, 9 Jan 2023 16:27:25 -0800 Subject: [PATCH 03/15] Create eleven-cycles-hang.md --- .changeset/eleven-cycles-hang.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/eleven-cycles-hang.md diff --git a/.changeset/eleven-cycles-hang.md b/.changeset/eleven-cycles-hang.md new file mode 100644 index 00000000000..49d15a764e5 --- /dev/null +++ b/.changeset/eleven-cycles-hang.md @@ -0,0 +1,5 @@ +--- +"@firebase/storage": patch +--- + +Fixed issue where pause throws an error From 11c164e2524d8442f3e205ee5ba5df2e3988a6d6 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Tue, 10 Jan 2023 14:29:39 -0800 Subject: [PATCH 04/15] WIP --- config/.eslintrc.js | 8 +- .../storage/src/implementation/request.ts | 9 +- packages/storage/src/task.ts | 4 + .../test/integration/integration.test.ts | 7 +- packages/storage/test/unit/connection.ts | 2 +- packages/storage/test/unit/task.test.ts | 133 ++++++++++++++++++ packages/storage/test/unit/testshared.ts | 21 ++- 7 files changed, 170 insertions(+), 14 deletions(-) diff --git a/config/.eslintrc.js b/config/.eslintrc.js index 04de0f9d10c..63c579ccd33 100644 --- a/config/.eslintrc.js +++ b/config/.eslintrc.js @@ -98,10 +98,10 @@ module.exports = { 'object': 'it', 'property': 'skip' }, - { - 'object': 'it', - 'property': 'only' - }, + // { + // 'object': 'it', + // 'property': 'only' + // }, { 'object': 'describe', 'property': 'skip' diff --git a/packages/storage/src/implementation/request.ts b/packages/storage/src/implementation/request.ts index fae46d7a5ab..02d1478ab45 100644 --- a/packages/storage/src/implementation/request.ts +++ b/packages/storage/src/implementation/request.ts @@ -84,6 +84,7 @@ class NetworkRequest implements Request { * Actually starts the retry loop. */ private start_(): void { + console.log('start'); const doTheRequest: ( backoffCallback: (success: boolean, ...p2: unknown[]) => void, canceled: boolean @@ -118,11 +119,13 @@ class NetworkRequest implements Request { } this.pendingConnection_ = null; const hitServer = connection.getErrorCode() === ErrorCode.NO_ERROR; + console.log(connection.getErrorCode()); + console.log(connection.getStatus()); const status = connection.getStatus(); if ( - !hitServer || - (isRetryStatusCode(status, this.additionalRetryCodes_) && - this.retry) + (!hitServer || + isRetryStatusCode(status, this.additionalRetryCodes_))&& + this.retry ) { const wasCanceled = connection.getErrorCode() === ErrorCode.ABORT; backoffCallback( diff --git a/packages/storage/src/task.ts b/packages/storage/src/task.ts index 617fce08d26..faf34985b29 100644 --- a/packages/storage/src/task.ts +++ b/packages/storage/src/task.ts @@ -230,6 +230,7 @@ export class UploadTask { // TODO(andysoto): assert false private _createResumable(): void { + console.log('creating resumable'); this._resolveToken((authToken, appCheckToken) => { const requestInfo = createResumableUpload( this._ref.storage, @@ -245,7 +246,9 @@ export class UploadTask { appCheckToken ); this._request = createRequest; + console.log(this._request); createRequest.getPromise().then((url: string) => { + console.log('clearing from createRequest'); this._request = undefined; this._uploadUrl = url; this._needToFetchStatus = false; @@ -412,6 +415,7 @@ export class UploadTask { // assert(this.state_ === InternalTaskState.RUNNING || // this.state_ === InternalTaskState.PAUSING); this._state = state; + console.log(this._request); if (this._request !== undefined) { this._request.cancel(); } else if (this.pendingTimeout) { diff --git a/packages/storage/test/integration/integration.test.ts b/packages/storage/test/integration/integration.test.ts index b6e21609bb9..df6622b5878 100644 --- a/packages/storage/test/integration/integration.test.ts +++ b/packages/storage/test/integration/integration.test.ts @@ -36,7 +36,6 @@ import { import { use, expect } from 'chai'; import chaiAsPromised from 'chai-as-promised'; import * as types from '../../src/public-types'; -import { waitFor } from '../../../database/test/helpers/util'; import { Deferred } from '@firebase/util'; use(chaiAsPromised); @@ -199,7 +198,6 @@ describe('FirebaseStorage Exp', () => { task.on( 'state_changed', snapshot => { - const p = [snapshot.bytesTransferred, snapshot.totalBytes]; if (snapshot.bytesTransferred > 0 && !hasPaused) { task.pause(); hasPaused = true; @@ -209,7 +207,10 @@ describe('FirebaseStorage Exp', () => { failureDeferred.reject('Failed to upload file'); } ); - await Promise.race([failureDeferred.promise, waitFor(5000)]); + await Promise.race([ + failureDeferred.promise, + new Promise(resolve => setTimeout(resolve, 4000)) + ]); task.resume(); await task; const bytes = await getBytes(referenceA); diff --git a/packages/storage/test/unit/connection.ts b/packages/storage/test/unit/connection.ts index 018f65f3671..6b800a17f91 100644 --- a/packages/storage/test/unit/connection.ts +++ b/packages/storage/test/unit/connection.ts @@ -117,7 +117,7 @@ export class TestingConnection implements Connection { abort(): void { this.state = State.START; - this.errorCode = ErrorCode.NO_ERROR; + this.errorCode = ErrorCode.ABORT; this.resolve(); } diff --git a/packages/storage/test/unit/task.test.ts b/packages/storage/test/unit/task.test.ts index 18da6d83800..c03514c2c42 100644 --- a/packages/storage/test/unit/task.test.ts +++ b/packages/storage/test/unit/task.test.ts @@ -344,6 +344,135 @@ describe('Firebase Storage > Upload Task', () => { return promise; } + +async function runProgressPauseTest(blob: FbsBlob): Promise { + const storageService = storageServiceWithHandler(fakeServerHandler(), true); + const task = new UploadTask( + new Reference(storageService, testLocation), + blob + ); + + // eslint-disable-next-line @typescript-eslint/ban-types + let resolve: Function, reject: Function; + const promise = new Promise((innerResolve, innerReject) => { + resolve = innerResolve; + reject = innerReject; + }); + + // Assert functions throw Errors, which means if we're not in the right stack + // the error might not get reported properly. This function wraps existing + // assert functions, returning a new function that calls reject with any + // caught errors. This should make sure errors are reported properly. + function promiseAssertWrapper(func: T): T { + function wrapped(..._args: any[]): void { + try { + (func as any as (...args: any[]) => void).apply(null, _args); + } catch (e) { + reject(e); + // also throw to further unwind the stack + throw e; + } + } + return wrapped as any as T; + } + + const fixedAssertEquals = promiseAssertWrapper(assert.equal); + const fixedAssertFalse = promiseAssertWrapper(assert.isFalse); + const fixedAssertTrue = promiseAssertWrapper(assert.isTrue); + // const fixedAssertFail = promiseAssertWrapper(assert.fail); + const pausedDeferred = new Deferred(); + + const events: string[] = []; + const progress: number[][] = []; + let complete = 0; + let hasPaused = false; + function addCallbacks(task: UploadTask): void { + let lastState: string; + task.on( + TaskEvent.STATE_CHANGED, + snapshot => { + fixedAssertEquals(complete, 0); + + const state = snapshot.state; + if (lastState !== TaskState.RUNNING && state === TaskState.RUNNING) { + events.push('resume'); + } else if ( + lastState !== TaskState.PAUSED && + state === TaskState.PAUSED + ) { + events.push('pause'); + } + + const p = [snapshot.bytesTransferred, snapshot.totalBytes]; + if(snapshot.bytesTransferred > 0 && !hasPaused) { + task.pause(); + pausedDeferred.resolve(); + hasPaused = true; + } + progress.push(p); + + lastState = state; + }, + () => { + console.log('Failed to Upload'); + reject('Failed to Upload'); + }, + () => { + events.push('complete'); + complete++; + } + ); + } + + addCallbacks(task); + + let completeTriggered = false; + + task.on(TaskEvent.STATE_CHANGED, undefined, undefined, () => { + console.log(events); + fixedAssertFalse(completeTriggered); + completeTriggered = true; + + fixedAssertEquals(events.length, 4); + fixedAssertEquals(events[0], 'resume'); + fixedAssertEquals(events[1], 'pause'); + fixedAssertEquals(events[2], 'resume'); + fixedAssertEquals(events[3], 'complete'); + + fixedAssertEquals(complete, 1); + + let increasing = true; + let allTotalsTheSame = true; + for (let i = 0; i < progress.length - 1; i++) { + increasing = increasing && progress[i][0] <= progress[i + 1][0]; + allTotalsTheSame = + allTotalsTheSame && progress[i][1] === progress[i + 1][1]; + } + + let lastIsAll = false; + if (progress.length > 0) { + const last = progress[progress.length - 1]; + lastIsAll = last[0] === last[1]; + } else { + lastIsAll = true; + } + + fixedAssertTrue(increasing); + fixedAssertTrue(allTotalsTheSame); + fixedAssertTrue(lastIsAll); + console.log('RESOLVING'); + resolve(null); + }); + await pausedDeferred.promise; + await new Promise(resolve => + setTimeout(() => { + task.resume(); + resolve(null); + }, 0) + ); + + return promise; + } enum StateType { RESUME = 'resume', PAUSE = 'pause', @@ -593,6 +722,10 @@ describe('Firebase Storage > Upload Task', () => { expect(clock.countTimers()).to.eq(0); clock.restore(); }); + it.only('does not error after the first progress is uploaded', async () => { + // Kick off upload + await runProgressPauseTest(bigBlob); + }); it('tests if small requests that respond with 500 retry correctly', async () => { clock = useFakeTimers(); // Kick off upload diff --git a/packages/storage/test/unit/testshared.ts b/packages/storage/test/unit/testshared.ts index 502c2e507d2..3c28d7e3bde 100644 --- a/packages/storage/test/unit/testshared.ts +++ b/packages/storage/test/unit/testshared.ts @@ -205,7 +205,8 @@ export type RequestHandler = ( ) => Response; export function storageServiceWithHandler( - handler: RequestHandler + handler: RequestHandler, + delay = false ): FirebaseStorageImpl { function newSend( connection: TestingConnection, @@ -215,11 +216,20 @@ export function storageServiceWithHandler( headers?: Headers ): void { const response = handler(url, method, body, headers); - connection.simulateResponse( + if(delay) { +setTimeout(() => connection.simulateResponse( + response.status, + response.body, + response.headers + ), 2000); + } else { +connection.simulateResponse( response.status, response.body, response.headers ); + }; + } injectTestConnection(() => newTestConnection(newSend)); @@ -321,11 +331,13 @@ export function fakeServerHandler( if (isUpload) { const offset = +headers['X-Goog-Upload-Offset']; if (offset !== stat.currentSize) { + console.log('wrong offset', { offset, stat }); return { status: 400, body: 'Uploading at wrong offset', headers: {} }; } stat.currentSize += contentLength; if (stat.currentSize > stat.finalSize) { + console.log('too many bytes'); return { status: 400, body: 'Too many bytes', headers: {} }; } else if (!isFinalize) { return { status: 200, body: '', headers: statusHeaders('active') }; @@ -341,6 +353,7 @@ export function fakeServerHandler( headers: statusHeaders('final') }; } else { + console.log('wrong number of bytes'); return { status: 400, body: 'finalize without the right # of bytes', @@ -349,9 +362,11 @@ export function fakeServerHandler( } } + console.log('fallback'); return { status: 400, body: '', headers: {} }; } - return handler; + return handler; + } /** From 7bf749ccb2a82d3840d3d3b68a0b88d301a40cfa Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 11 Jan 2023 09:44:08 -0800 Subject: [PATCH 05/15] Added unit test. Stil WIP --- .../storage/src/implementation/request.ts | 7 ++-- packages/storage/src/task.ts | 5 +-- packages/storage/test/unit/connection.ts | 8 ++--- packages/storage/test/unit/task.test.ts | 32 +++++++++---------- packages/storage/test/unit/testshared.ts | 25 +++++++-------- 5 files changed, 33 insertions(+), 44 deletions(-) diff --git a/packages/storage/src/implementation/request.ts b/packages/storage/src/implementation/request.ts index 02d1478ab45..5a8ec8a4bd2 100644 --- a/packages/storage/src/implementation/request.ts +++ b/packages/storage/src/implementation/request.ts @@ -84,7 +84,6 @@ class NetworkRequest implements Request { * Actually starts the retry loop. */ private start_(): void { - console.log('start'); const doTheRequest: ( backoffCallback: (success: boolean, ...p2: unknown[]) => void, canceled: boolean @@ -119,13 +118,11 @@ class NetworkRequest implements Request { } this.pendingConnection_ = null; const hitServer = connection.getErrorCode() === ErrorCode.NO_ERROR; - console.log(connection.getErrorCode()); - console.log(connection.getStatus()); const status = connection.getStatus(); if ( (!hitServer || - isRetryStatusCode(status, this.additionalRetryCodes_))&& - this.retry + isRetryStatusCode(status, this.additionalRetryCodes_)&& + this.retry) ) { const wasCanceled = connection.getErrorCode() === ErrorCode.ABORT; backoffCallback( diff --git a/packages/storage/src/task.ts b/packages/storage/src/task.ts index faf34985b29..c6a159526b1 100644 --- a/packages/storage/src/task.ts +++ b/packages/storage/src/task.ts @@ -230,7 +230,6 @@ export class UploadTask { // TODO(andysoto): assert false private _createResumable(): void { - console.log('creating resumable'); this._resolveToken((authToken, appCheckToken) => { const requestInfo = createResumableUpload( this._ref.storage, @@ -246,9 +245,7 @@ export class UploadTask { appCheckToken ); this._request = createRequest; - console.log(this._request); createRequest.getPromise().then((url: string) => { - console.log('clearing from createRequest'); this._request = undefined; this._uploadUrl = url; this._needToFetchStatus = false; @@ -405,6 +402,7 @@ export class UploadTask { } private _transition(state: InternalTaskState): void { + console.log(`transitioning from ${this._state} to ${state}`); if (this._state === state) { return; } @@ -415,7 +413,6 @@ export class UploadTask { // assert(this.state_ === InternalTaskState.RUNNING || // this.state_ === InternalTaskState.PAUSING); this._state = state; - console.log(this._request); if (this._request !== undefined) { this._request.cancel(); } else if (this.pendingTimeout) { diff --git a/packages/storage/test/unit/connection.ts b/packages/storage/test/unit/connection.ts index 6b800a17f91..6ac855ac496 100644 --- a/packages/storage/test/unit/connection.ts +++ b/packages/storage/test/unit/connection.ts @@ -81,10 +81,10 @@ export class TestingConnection implements Connection { headers: { [key: string]: string } ): void { if (this.state !== State.SENT) { - throw new StorageError( - StorageErrorCode.UNKNOWN, - "Can't simulate response before send/more than once" - ); + // throw new StorageError( + // StorageErrorCode.UNKNOWN, + // "Can't simulate response before send/more than once" + // ); } this.status = status; diff --git a/packages/storage/test/unit/task.test.ts b/packages/storage/test/unit/task.test.ts index c03514c2c42..726a20ea973 100644 --- a/packages/storage/test/unit/task.test.ts +++ b/packages/storage/test/unit/task.test.ts @@ -341,12 +341,22 @@ describe('Firebase Storage > Upload Task', () => { }); task.pause(); - return promise; } async function runProgressPauseTest(blob: FbsBlob): Promise { - const storageService = storageServiceWithHandler(fakeServerHandler(), true); + + const pausedDeferred = new Deferred(); + let callbackCount = 0; + function callback() { + if(callbackCount++ == 1) { + pausedDeferred.resolve(); + task.pause(); + return false; + } + return true; + } + const storageService = storageServiceWithHandler(fakeServerHandler(), callback ); const task = new UploadTask( new Reference(storageService, testLocation), blob @@ -380,7 +390,6 @@ async function runProgressPauseTest(blob: FbsBlob): Promise { const fixedAssertFalse = promiseAssertWrapper(assert.isFalse); const fixedAssertTrue = promiseAssertWrapper(assert.isTrue); // const fixedAssertFail = promiseAssertWrapper(assert.fail); - const pausedDeferred = new Deferred(); const events: string[] = []; const progress: number[][] = []; @@ -404,11 +413,7 @@ async function runProgressPauseTest(blob: FbsBlob): Promise { } const p = [snapshot.bytesTransferred, snapshot.totalBytes]; - if(snapshot.bytesTransferred > 0 && !hasPaused) { - task.pause(); - pausedDeferred.resolve(); - hasPaused = true; - } + progress.push(p); lastState = state; @@ -429,7 +434,6 @@ async function runProgressPauseTest(blob: FbsBlob): Promise { let completeTriggered = false; task.on(TaskEvent.STATE_CHANGED, undefined, undefined, () => { - console.log(events); fixedAssertFalse(completeTriggered); completeTriggered = true; @@ -460,17 +464,11 @@ async function runProgressPauseTest(blob: FbsBlob): Promise { fixedAssertTrue(increasing); fixedAssertTrue(allTotalsTheSame); fixedAssertTrue(lastIsAll); - console.log('RESOLVING'); resolve(null); }); await pausedDeferred.promise; - await new Promise(resolve => - setTimeout(() => { - task.resume(); - resolve(null); - }, 0) - ); - + await new Promise(resolve => setTimeout(resolve, 1000)); + task.resume(); return promise; } enum StateType { diff --git a/packages/storage/test/unit/testshared.ts b/packages/storage/test/unit/testshared.ts index 3c28d7e3bde..94118c12b57 100644 --- a/packages/storage/test/unit/testshared.ts +++ b/packages/storage/test/unit/testshared.ts @@ -206,7 +206,7 @@ export type RequestHandler = ( export function storageServiceWithHandler( handler: RequestHandler, - delay = false + callback?: Function ): FirebaseStorageImpl { function newSend( connection: TestingConnection, @@ -216,19 +216,16 @@ export function storageServiceWithHandler( headers?: Headers ): void { const response = handler(url, method, body, headers); - if(delay) { -setTimeout(() => connection.simulateResponse( - response.status, - response.body, - response.headers - ), 2000); - } else { +if(!callback || callback()) { + connection.simulateResponse( response.status, response.body, response.headers ); - }; +} else { + callback(); +} } @@ -241,7 +238,8 @@ connection.simulateResponse( } export function fakeServerHandler( - fakeMetadata: Partial = defaultFakeMetadata + fakeMetadata: Partial = defaultFakeMetadata, + cb?: Function ): RequestHandler { const stats: { [num: number]: { @@ -271,6 +269,9 @@ export function fakeServerHandler( content = content || ''; headers = headers || {}; + if(cb) { + cb(); + } if (headers['X-Goog-Upload-Protocol'] === 'multipart') { return { status: 200, @@ -331,13 +332,11 @@ export function fakeServerHandler( if (isUpload) { const offset = +headers['X-Goog-Upload-Offset']; if (offset !== stat.currentSize) { - console.log('wrong offset', { offset, stat }); return { status: 400, body: 'Uploading at wrong offset', headers: {} }; } stat.currentSize += contentLength; if (stat.currentSize > stat.finalSize) { - console.log('too many bytes'); return { status: 400, body: 'Too many bytes', headers: {} }; } else if (!isFinalize) { return { status: 200, body: '', headers: statusHeaders('active') }; @@ -353,7 +352,6 @@ export function fakeServerHandler( headers: statusHeaders('final') }; } else { - console.log('wrong number of bytes'); return { status: 400, body: 'finalize without the right # of bytes', @@ -362,7 +360,6 @@ export function fakeServerHandler( } } - console.log('fallback'); return { status: 400, body: '', headers: {} }; } return handler; From 6b7698b0d63a3717806399660cb0323bcb589096 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 11 Jan 2023 10:32:30 -0800 Subject: [PATCH 06/15] Cleaned up tests a bit --- packages/storage/test/unit/connection.ts | 8 ++++---- packages/storage/test/unit/task.test.ts | 9 +++++---- packages/storage/test/unit/testshared.ts | 20 ++++---------------- 3 files changed, 13 insertions(+), 24 deletions(-) diff --git a/packages/storage/test/unit/connection.ts b/packages/storage/test/unit/connection.ts index 6ac855ac496..6b800a17f91 100644 --- a/packages/storage/test/unit/connection.ts +++ b/packages/storage/test/unit/connection.ts @@ -81,10 +81,10 @@ export class TestingConnection implements Connection { headers: { [key: string]: string } ): void { if (this.state !== State.SENT) { - // throw new StorageError( - // StorageErrorCode.UNKNOWN, - // "Can't simulate response before send/more than once" - // ); + throw new StorageError( + StorageErrorCode.UNKNOWN, + "Can't simulate response before send/more than once" + ); } this.status = status; diff --git a/packages/storage/test/unit/task.test.ts b/packages/storage/test/unit/task.test.ts index 726a20ea973..b9ac3806205 100644 --- a/packages/storage/test/unit/task.test.ts +++ b/packages/storage/test/unit/task.test.ts @@ -344,11 +344,14 @@ describe('Firebase Storage > Upload Task', () => { return promise; } +// This is to test to make sure that when you pause in the middle of a request that you do not get an error async function runProgressPauseTest(blob: FbsBlob): Promise { const pausedDeferred = new Deferred(); let callbackCount = 0; - function callback() { + // Usually the first request is to create the resumable upload and the second is to upload. + // Upload requests are not retryable, and this callback is to make sure we pause before the response comes back. + function shouldRespondCallback() { if(callbackCount++ == 1) { pausedDeferred.resolve(); task.pause(); @@ -356,7 +359,7 @@ async function runProgressPauseTest(blob: FbsBlob): Promise { } return true; } - const storageService = storageServiceWithHandler(fakeServerHandler(), callback ); + const storageService = storageServiceWithHandler(fakeServerHandler(), shouldRespondCallback ); const task = new UploadTask( new Reference(storageService, testLocation), blob @@ -389,12 +392,10 @@ async function runProgressPauseTest(blob: FbsBlob): Promise { const fixedAssertEquals = promiseAssertWrapper(assert.equal); const fixedAssertFalse = promiseAssertWrapper(assert.isFalse); const fixedAssertTrue = promiseAssertWrapper(assert.isTrue); - // const fixedAssertFail = promiseAssertWrapper(assert.fail); const events: string[] = []; const progress: number[][] = []; let complete = 0; - let hasPaused = false; function addCallbacks(task: UploadTask): void { let lastState: string; task.on( diff --git a/packages/storage/test/unit/testshared.ts b/packages/storage/test/unit/testshared.ts index 94118c12b57..9924f5c2d1c 100644 --- a/packages/storage/test/unit/testshared.ts +++ b/packages/storage/test/unit/testshared.ts @@ -206,7 +206,7 @@ export type RequestHandler = ( export function storageServiceWithHandler( handler: RequestHandler, - callback?: Function + shouldResponseCb?: Function ): FirebaseStorageImpl { function newSend( connection: TestingConnection, @@ -216,17 +216,9 @@ export function storageServiceWithHandler( headers?: Headers ): void { const response = handler(url, method, body, headers); -if(!callback || callback()) { - -connection.simulateResponse( - response.status, - response.body, - response.headers - ); -} else { - callback(); -} - + if (!shouldResponseCb || shouldResponseCb()) { + connection.simulateResponse(response.status, response.body, response.headers); + } } injectTestConnection(() => newTestConnection(newSend)); @@ -239,7 +231,6 @@ connection.simulateResponse( export function fakeServerHandler( fakeMetadata: Partial = defaultFakeMetadata, - cb?: Function ): RequestHandler { const stats: { [num: number]: { @@ -269,9 +260,6 @@ export function fakeServerHandler( content = content || ''; headers = headers || {}; - if(cb) { - cb(); - } if (headers['X-Goog-Upload-Protocol'] === 'multipart') { return { status: 200, From cb37e844611a3e332ac09538db7975c2187efbf0 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 11 Jan 2023 10:33:01 -0800 Subject: [PATCH 07/15] Formatting --- packages/storage/src/implementation/request.ts | 4 ++-- packages/storage/test/unit/task.test.ts | 16 +++++++++------- packages/storage/test/unit/testshared.ts | 11 +++++++---- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/packages/storage/src/implementation/request.ts b/packages/storage/src/implementation/request.ts index 5a8ec8a4bd2..fae46d7a5ab 100644 --- a/packages/storage/src/implementation/request.ts +++ b/packages/storage/src/implementation/request.ts @@ -120,8 +120,8 @@ class NetworkRequest implements Request { const hitServer = connection.getErrorCode() === ErrorCode.NO_ERROR; const status = connection.getStatus(); if ( - (!hitServer || - isRetryStatusCode(status, this.additionalRetryCodes_)&& + !hitServer || + (isRetryStatusCode(status, this.additionalRetryCodes_) && this.retry) ) { const wasCanceled = connection.getErrorCode() === ErrorCode.ABORT; diff --git a/packages/storage/test/unit/task.test.ts b/packages/storage/test/unit/task.test.ts index b9ac3806205..4fc2e1dcc71 100644 --- a/packages/storage/test/unit/task.test.ts +++ b/packages/storage/test/unit/task.test.ts @@ -344,22 +344,24 @@ describe('Firebase Storage > Upload Task', () => { return promise; } -// This is to test to make sure that when you pause in the middle of a request that you do not get an error -async function runProgressPauseTest(blob: FbsBlob): Promise { - + // This is to test to make sure that when you pause in the middle of a request that you do not get an error + async function runProgressPauseTest(blob: FbsBlob): Promise { const pausedDeferred = new Deferred(); let callbackCount = 0; // Usually the first request is to create the resumable upload and the second is to upload. // Upload requests are not retryable, and this callback is to make sure we pause before the response comes back. function shouldRespondCallback() { - if(callbackCount++ == 1) { + if (callbackCount++ == 1) { pausedDeferred.resolve(); task.pause(); return false; } - return true; + return true; } - const storageService = storageServiceWithHandler(fakeServerHandler(), shouldRespondCallback ); + const storageService = storageServiceWithHandler( + fakeServerHandler(), + shouldRespondCallback + ); const task = new UploadTask( new Reference(storageService, testLocation), blob @@ -414,7 +416,7 @@ async function runProgressPauseTest(blob: FbsBlob): Promise { } const p = [snapshot.bytesTransferred, snapshot.totalBytes]; - + progress.push(p); lastState = state; diff --git a/packages/storage/test/unit/testshared.ts b/packages/storage/test/unit/testshared.ts index 9924f5c2d1c..6302e21a490 100644 --- a/packages/storage/test/unit/testshared.ts +++ b/packages/storage/test/unit/testshared.ts @@ -217,7 +217,11 @@ export function storageServiceWithHandler( ): void { const response = handler(url, method, body, headers); if (!shouldResponseCb || shouldResponseCb()) { - connection.simulateResponse(response.status, response.body, response.headers); + connection.simulateResponse( + response.status, + response.body, + response.headers + ); } } @@ -230,7 +234,7 @@ export function storageServiceWithHandler( } export function fakeServerHandler( - fakeMetadata: Partial = defaultFakeMetadata, + fakeMetadata: Partial = defaultFakeMetadata ): RequestHandler { const stats: { [num: number]: { @@ -350,8 +354,7 @@ export function fakeServerHandler( return { status: 400, body: '', headers: {} }; } - return handler; - + return handler; } /** From 8ce7a460ef00ea3383d6e332762a766d9d34816d Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 11 Jan 2023 10:37:45 -0800 Subject: [PATCH 08/15] More cleanpu --- .changeset/eleven-cycles-hang.md | 2 +- config/.eslintrc.js | 8 ++++---- packages/storage/src/task.ts | 1 - packages/storage/test/unit/task.test.ts | 3 ++- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.changeset/eleven-cycles-hang.md b/.changeset/eleven-cycles-hang.md index 49d15a764e5..92ee8444407 100644 --- a/.changeset/eleven-cycles-hang.md +++ b/.changeset/eleven-cycles-hang.md @@ -2,4 +2,4 @@ "@firebase/storage": patch --- -Fixed issue where pause throws an error +Fixed issue where pause throws an error when a request is in flight. diff --git a/config/.eslintrc.js b/config/.eslintrc.js index 63c579ccd33..04de0f9d10c 100644 --- a/config/.eslintrc.js +++ b/config/.eslintrc.js @@ -98,10 +98,10 @@ module.exports = { 'object': 'it', 'property': 'skip' }, - // { - // 'object': 'it', - // 'property': 'only' - // }, + { + 'object': 'it', + 'property': 'only' + }, { 'object': 'describe', 'property': 'skip' diff --git a/packages/storage/src/task.ts b/packages/storage/src/task.ts index c6a159526b1..617fce08d26 100644 --- a/packages/storage/src/task.ts +++ b/packages/storage/src/task.ts @@ -402,7 +402,6 @@ export class UploadTask { } private _transition(state: InternalTaskState): void { - console.log(`transitioning from ${this._state} to ${state}`); if (this._state === state) { return; } diff --git a/packages/storage/test/unit/task.test.ts b/packages/storage/test/unit/task.test.ts index 4fc2e1dcc71..aea8f9975d4 100644 --- a/packages/storage/test/unit/task.test.ts +++ b/packages/storage/test/unit/task.test.ts @@ -470,6 +470,7 @@ describe('Firebase Storage > Upload Task', () => { resolve(null); }); await pausedDeferred.promise; + // Need to wait until the state has settled [i.e. from pausing to pause] and allow for any potential errors to propagate. await new Promise(resolve => setTimeout(resolve, 1000)); task.resume(); return promise; @@ -723,7 +724,7 @@ describe('Firebase Storage > Upload Task', () => { expect(clock.countTimers()).to.eq(0); clock.restore(); }); - it.only('does not error after the first progress is uploaded', async () => { + it('does not error after the first progress is uploaded', async () => { // Kick off upload await runProgressPauseTest(bigBlob); }); From e172faf799457df7daaa9b336c4c8c354803f1e1 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 11 Jan 2023 10:45:12 -0800 Subject: [PATCH 09/15] Fixed linting --- packages/storage/test/unit/task.test.ts | 2 +- packages/storage/test/unit/testshared.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/storage/test/unit/task.test.ts b/packages/storage/test/unit/task.test.ts index aea8f9975d4..f9f3935f8a3 100644 --- a/packages/storage/test/unit/task.test.ts +++ b/packages/storage/test/unit/task.test.ts @@ -351,7 +351,7 @@ describe('Firebase Storage > Upload Task', () => { // Usually the first request is to create the resumable upload and the second is to upload. // Upload requests are not retryable, and this callback is to make sure we pause before the response comes back. function shouldRespondCallback() { - if (callbackCount++ == 1) { + if (callbackCount++ === 1) { pausedDeferred.resolve(); task.pause(); return false; diff --git a/packages/storage/test/unit/testshared.ts b/packages/storage/test/unit/testshared.ts index 6302e21a490..c71c4eae2e7 100644 --- a/packages/storage/test/unit/testshared.ts +++ b/packages/storage/test/unit/testshared.ts @@ -206,7 +206,7 @@ export type RequestHandler = ( export function storageServiceWithHandler( handler: RequestHandler, - shouldResponseCb?: Function + shouldResponseCb?: () => boolean ): FirebaseStorageImpl { function newSend( connection: TestingConnection, From b98a71d67d9ba618445879956debae1441bc9df1 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 11 Jan 2023 10:58:57 -0800 Subject: [PATCH 10/15] Cleaned up naming --- packages/storage/test/unit/task.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/storage/test/unit/task.test.ts b/packages/storage/test/unit/task.test.ts index f9f3935f8a3..3be07f8db4d 100644 --- a/packages/storage/test/unit/task.test.ts +++ b/packages/storage/test/unit/task.test.ts @@ -422,7 +422,6 @@ describe('Firebase Storage > Upload Task', () => { lastState = state; }, () => { - console.log('Failed to Upload'); reject('Failed to Upload'); }, () => { @@ -724,7 +723,7 @@ describe('Firebase Storage > Upload Task', () => { expect(clock.countTimers()).to.eq(0); clock.restore(); }); - it('does not error after the first progress is uploaded', async () => { + it('does not error when pausing inflight request', async () => { // Kick off upload await runProgressPauseTest(bigBlob); }); From 9148efe468554d9f42ad01e10776daa1ff150577 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 11 Jan 2023 11:03:41 -0800 Subject: [PATCH 11/15] Fixed linting --- packages/storage/test/unit/task.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/storage/test/unit/task.test.ts b/packages/storage/test/unit/task.test.ts index 3be07f8db4d..84e1c44a850 100644 --- a/packages/storage/test/unit/task.test.ts +++ b/packages/storage/test/unit/task.test.ts @@ -350,7 +350,7 @@ describe('Firebase Storage > Upload Task', () => { let callbackCount = 0; // Usually the first request is to create the resumable upload and the second is to upload. // Upload requests are not retryable, and this callback is to make sure we pause before the response comes back. - function shouldRespondCallback() { + function shouldRespondCallback(): boolean { if (callbackCount++ === 1) { pausedDeferred.resolve(); task.pause(); From 70ee6c159496b6785cc0990b49fcaa7c76c8abcb Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 11 Jan 2023 13:15:40 -0800 Subject: [PATCH 12/15] Replaced timeout --- packages/storage/src/implementation/request.ts | 6 +++--- packages/storage/test/unit/task.test.ts | 17 ++++++++++------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/packages/storage/src/implementation/request.ts b/packages/storage/src/implementation/request.ts index fae46d7a5ab..15129af91bd 100644 --- a/packages/storage/src/implementation/request.ts +++ b/packages/storage/src/implementation/request.ts @@ -120,9 +120,9 @@ class NetworkRequest implements Request { const hitServer = connection.getErrorCode() === ErrorCode.NO_ERROR; const status = connection.getStatus(); if ( - !hitServer || - (isRetryStatusCode(status, this.additionalRetryCodes_) && - this.retry) + (!hitServer || + isRetryStatusCode(status, this.additionalRetryCodes_)) && + this.retry ) { const wasCanceled = connection.getErrorCode() === ErrorCode.ABORT; backoffCallback( diff --git a/packages/storage/test/unit/task.test.ts b/packages/storage/test/unit/task.test.ts index 84e1c44a850..715a12c0482 100644 --- a/packages/storage/test/unit/task.test.ts +++ b/packages/storage/test/unit/task.test.ts @@ -346,13 +346,11 @@ describe('Firebase Storage > Upload Task', () => { // This is to test to make sure that when you pause in the middle of a request that you do not get an error async function runProgressPauseTest(blob: FbsBlob): Promise { - const pausedDeferred = new Deferred(); let callbackCount = 0; // Usually the first request is to create the resumable upload and the second is to upload. // Upload requests are not retryable, and this callback is to make sure we pause before the response comes back. function shouldRespondCallback(): boolean { if (callbackCount++ === 1) { - pausedDeferred.resolve(); task.pause(); return false; } @@ -384,6 +382,7 @@ describe('Firebase Storage > Upload Task', () => { (func as any as (...args: any[]) => void).apply(null, _args); } catch (e) { reject(e); + pausedStateCompleted.reject(e); // also throw to further unwind the stack throw e; } @@ -394,9 +393,12 @@ describe('Firebase Storage > Upload Task', () => { const fixedAssertEquals = promiseAssertWrapper(assert.equal); const fixedAssertFalse = promiseAssertWrapper(assert.isFalse); const fixedAssertTrue = promiseAssertWrapper(assert.isTrue); + const fixedAssertFail = promiseAssertWrapper(assert.fail); const events: string[] = []; const progress: number[][] = []; + // Promise for when we are finally in the pause state + const pausedStateCompleted = new Deferred(); let complete = 0; function addCallbacks(task: UploadTask): void { let lastState: string; @@ -412,6 +414,7 @@ describe('Firebase Storage > Upload Task', () => { lastState !== TaskState.PAUSED && state === TaskState.PAUSED ) { + pausedStateCompleted.resolve(); events.push('pause'); } @@ -422,7 +425,7 @@ describe('Firebase Storage > Upload Task', () => { lastState = state; }, () => { - reject('Failed to Upload'); + fixedAssertFail('Failed to upload'); }, () => { events.push('complete'); @@ -435,6 +438,8 @@ describe('Firebase Storage > Upload Task', () => { let completeTriggered = false; + // We should clean this up and just add all callbacks in one function call. + // Keeps track of all events that were logged before and asserts on them. task.on(TaskEvent.STATE_CHANGED, undefined, undefined, () => { fixedAssertFalse(completeTriggered); completeTriggered = true; @@ -468,9 +473,7 @@ describe('Firebase Storage > Upload Task', () => { fixedAssertTrue(lastIsAll); resolve(null); }); - await pausedDeferred.promise; - // Need to wait until the state has settled [i.e. from pausing to pause] and allow for any potential errors to propagate. - await new Promise(resolve => setTimeout(resolve, 1000)); + await pausedStateCompleted.promise; task.resume(); return promise; } @@ -723,7 +726,7 @@ describe('Firebase Storage > Upload Task', () => { expect(clock.countTimers()).to.eq(0); clock.restore(); }); - it('does not error when pausing inflight request', async () => { + it.only('does not error when pausing inflight request', async () => { // Kick off upload await runProgressPauseTest(bigBlob); }); From 2c4bed98aa138da5e52cae77be57e6ad1ba3bdae Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 11 Jan 2023 13:17:45 -0800 Subject: [PATCH 13/15] Fixed bug again --- packages/storage/src/implementation/request.ts | 6 +++--- packages/storage/test/unit/task.test.ts | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/storage/src/implementation/request.ts b/packages/storage/src/implementation/request.ts index 15129af91bd..fae46d7a5ab 100644 --- a/packages/storage/src/implementation/request.ts +++ b/packages/storage/src/implementation/request.ts @@ -120,9 +120,9 @@ class NetworkRequest implements Request { const hitServer = connection.getErrorCode() === ErrorCode.NO_ERROR; const status = connection.getStatus(); if ( - (!hitServer || - isRetryStatusCode(status, this.additionalRetryCodes_)) && - this.retry + !hitServer || + (isRetryStatusCode(status, this.additionalRetryCodes_) && + this.retry) ) { const wasCanceled = connection.getErrorCode() === ErrorCode.ABORT; backoffCallback( diff --git a/packages/storage/test/unit/task.test.ts b/packages/storage/test/unit/task.test.ts index 715a12c0482..3edf4a22ef7 100644 --- a/packages/storage/test/unit/task.test.ts +++ b/packages/storage/test/unit/task.test.ts @@ -726,7 +726,7 @@ describe('Firebase Storage > Upload Task', () => { expect(clock.countTimers()).to.eq(0); clock.restore(); }); - it.only('does not error when pausing inflight request', async () => { + it('does not error when pausing inflight request', async () => { // Kick off upload await runProgressPauseTest(bigBlob); }); From fc80cf194688d98d3d39e15431c85c353a55036e Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Fri, 13 Jan 2023 08:41:27 -0800 Subject: [PATCH 14/15] Added a comment --- packages/storage/test/unit/task.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/storage/test/unit/task.test.ts b/packages/storage/test/unit/task.test.ts index 3edf4a22ef7..afe25645ab5 100644 --- a/packages/storage/test/unit/task.test.ts +++ b/packages/storage/test/unit/task.test.ts @@ -400,6 +400,8 @@ describe('Firebase Storage > Upload Task', () => { // Promise for when we are finally in the pause state const pausedStateCompleted = new Deferred(); let complete = 0; + // Adds a callback for when the state has changed. The callback resolves the pausedStateCompleted promise + // to let our await know when to resume. function addCallbacks(task: UploadTask): void { let lastState: string; task.on( From 3d2b48a2abb09244c4ef5273288310f34f7e41be Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Fri, 13 Jan 2023 08:42:04 -0800 Subject: [PATCH 15/15] Amended comment --- packages/storage/test/unit/task.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/storage/test/unit/task.test.ts b/packages/storage/test/unit/task.test.ts index afe25645ab5..8a4f8bf6d78 100644 --- a/packages/storage/test/unit/task.test.ts +++ b/packages/storage/test/unit/task.test.ts @@ -401,7 +401,7 @@ describe('Firebase Storage > Upload Task', () => { const pausedStateCompleted = new Deferred(); let complete = 0; // Adds a callback for when the state has changed. The callback resolves the pausedStateCompleted promise - // to let our await know when to resume. + // to let our test know when to resume. function addCallbacks(task: UploadTask): void { let lastState: string; task.on(