Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed issue where pause throws an error #6938

Merged
merged 16 commits into from
Jan 13, 2023
Prev Previous commit
Next Next commit
WIP
  • Loading branch information
maneesht committed Jan 10, 2023
commit 11c164e2524d8442f3e205ee5ba5df2e3988a6d6
8 changes: 4 additions & 4 deletions config/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,10 @@ module.exports = {
'object': 'it',
'property': 'skip'
},
{
'object': 'it',
'property': 'only'
},
// {
// 'object': 'it',
// 'property': 'only'
// },
{
'object': 'describe',
'property': 'skip'
Expand Down
9 changes: 6 additions & 3 deletions packages/storage/src/implementation/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ class NetworkRequest<I extends ConnectionType, O> implements Request<O> {
* Actually starts the retry loop.
*/
private start_(): void {
console.log('start');
const doTheRequest: (
backoffCallback: (success: boolean, ...p2: unknown[]) => void,
canceled: boolean
Expand Down Expand Up @@ -118,11 +119,13 @@ class NetworkRequest<I extends ConnectionType, O> implements Request<O> {
}
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(
Expand Down
4 changes: 4 additions & 0 deletions packages/storage/src/task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
7 changes: 4 additions & 3 deletions packages/storage/test/integration/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion packages/storage/test/unit/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ export class TestingConnection implements Connection<string> {

abort(): void {
this.state = State.START;
this.errorCode = ErrorCode.NO_ERROR;
this.errorCode = ErrorCode.ABORT;
this.resolve();
}

Expand Down
133 changes: 133 additions & 0 deletions packages/storage/test/unit/task.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,135 @@ describe('Firebase Storage > Upload Task', () => {

return promise;
}

async function runProgressPauseTest(blob: FbsBlob): Promise<void> {
const storageService = storageServiceWithHandler(fakeServerHandler(), true);
const task = new UploadTask(
new Reference(storageService, testLocation),
blob
);

// eslint-disable-next-line @typescript-eslint/ban-types
maneesht marked this conversation as resolved.
Show resolved Hide resolved
let resolve: Function, reject: Function;
const promise = new Promise<void>((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<T>(func: T): T {
maneesht marked this conversation as resolved.
Show resolved Hide resolved
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 {
maneesht marked this conversation as resolved.
Show resolved Hide resolved
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',
Expand Down Expand Up @@ -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
Expand Down
21 changes: 18 additions & 3 deletions packages/storage/test/unit/testshared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,8 @@ export type RequestHandler = (
) => Response;

export function storageServiceWithHandler(
handler: RequestHandler
handler: RequestHandler,
delay = false
): FirebaseStorageImpl {
function newSend(
connection: TestingConnection,
Expand All @@ -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));
Expand Down Expand Up @@ -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') };
Expand All @@ -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',
Expand All @@ -349,9 +362,11 @@ export function fakeServerHandler(
}
}

console.log('fallback');
return { status: 400, body: '', headers: {} };
}
return handler;
return handler;

}

/**
Expand Down