Skip to content

Add retry logic around logic IndexedDB connections. #4047

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

Merged
merged 2 commits into from
Nov 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -324,4 +324,29 @@ describe('platform_browser/persistence/indexed_db', () => {
});
});
});

describe('closed IndexedDB connection', () => {
it('should retry by reopening the connection', async () => {
const closeDb = async (): Promise<void> => {
const db = await ((persistence as unknown) as {
_openDb(): Promise<IDBDatabase>;
})._openDb();
db.close();
};
const key = 'my-super-special-persistence-type';
const value = PersistenceType.LOCAL;

expect(await persistence._get(key)).to.be.null;

await closeDb();
await persistence._set(key, value);

await closeDb();
expect(await persistence._get(key)).to.be.eq(value);

await closeDb();
await persistence._remove(key);
expect(await persistence._get(key)).to.be.null;
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ function deleteObject(db: IDBDatabase, key: string): Promise<void> {

/** @internal */
export const _POLLING_INTERVAL_MS = 800;
/** @internal */
export const _TRANSACTION_RETRY_COUNT = 3;

class IndexedDBLocalPersistence implements Persistence {
static type: 'LOCAL' = 'LOCAL';
Expand Down Expand Up @@ -193,14 +195,34 @@ class IndexedDBLocalPersistence implements Persistence {
);
}

private async initialize(): Promise<IDBDatabase> {
async _openDb(): Promise<IDBDatabase> {
if (this.db) {
return this.db;
}
this.db = await _openDatabase();
return this.db;
}

async _withRetries<T>(op: (db: IDBDatabase) => Promise<T>): Promise<T> {
let numAttempts = 0;

while (true) {
try {
const db = await this._openDb();
return await op(db);
} catch (e) {
if (numAttempts++ > _TRANSACTION_RETRY_COUNT) {
throw e;
}
if (this.db) {
this.db.close();
this.db = undefined;
}
// TODO: consider adding exponential backoff
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth adding any sort of delay here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question for @schmidt-sebastian I think he mentioned they had this in Firestore but I couldn't find proof

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We retry immediately at this scope, but at the higher level we retry with backoff: https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/util/async_queue.ts#L361

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, will consider adding that in a followup if this doesn't fix it

}
}
}

/**
* IndexedDB events do not propagate from the main window to the worker context. We rely on a
* postMessage interface to send these events to the worker ourselves.
Expand Down Expand Up @@ -318,38 +340,35 @@ class IndexedDBLocalPersistence implements Persistence {
}

async _set(key: string, value: PersistenceValue): Promise<void> {
const db = await this.initialize();
return this._withPendingWrite(async () => {
await _putObject(db, key, value);
await this._withRetries((db: IDBDatabase) => _putObject(db, key, value));
this.localCache[key] = value;
return this.notifyServiceWorker(key);
});
}

async _get<T extends PersistenceValue>(key: string): Promise<T | null> {
const db = await this.initialize();
const obj = (await getObject(db, key)) as T;
const obj = (await this._withRetries((db: IDBDatabase) =>
getObject(db, key)
)) as T;
this.localCache[key] = obj;
return obj;
}

async _remove(key: string): Promise<void> {
const db = await this.initialize();
return this._withPendingWrite(async () => {
await deleteObject(db, key);
await this._withRetries((db: IDBDatabase) => deleteObject(db, key));
delete this.localCache[key];
return this.notifyServiceWorker(key);
});
}

private async _poll(): Promise<string[]> {
const db = await _openDatabase();

// TODO: check if we need to fallback if getAll is not supported
const getAllRequest = getObjectStore(db, false).getAll();
const result = await new DBPromise<DBObject[] | null>(
getAllRequest
).toPromise();
const result = await this._withRetries((db: IDBDatabase) => {
const getAllRequest = getObjectStore(db, false).getAll();
return new DBPromise<DBObject[] | null>(getAllRequest).toPromise();
});

if (!result) {
return [];
Expand Down