Skip to content

Marking SimpleDb calls as idempotent #2251

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.

Sign up for GitHub

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 10 commits into from
Oct 10, 2019
Merged

Marking SimpleDb calls as idempotent #2251

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
48642ad
Add runTransaction parameter
schmidt-sebastian Oct 9, 2019
e4cdfe0
Add transaction retries
schmidt-sebastian Oct 9, 2019
d3669b6
Unit test fix
schmidt-sebastian Oct 9, 2019
10c2f8e
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Oct 9, 2019
a1e5a00
First round of feedback
schmidt-sebastian Oct 9, 2019
aee20aa
Lint
schmidt-sebastian Oct 9, 2019
3048923
Marking all SimpleDb calls as idempotent
schmidt-sebastian Oct 9, 2019
9740f64
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Oct 9, 2019
723de6d
Merge
schmidt-sebastian Oct 10, 2019
8656142
Comments
schmidt-sebastian Oct 10, 2019
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
30 changes: 25 additions & 5 deletions packages/firestore/src/local/indexeddb_persistence.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,15 @@ export class IndexedDbTransaction extends PersistenceTransaction {
}
}

// The different modes supported by `IndexedDbPersistence.runTransaction()`
type IndexedDbTransactionMode =
| 'readonly'
| 'readwrite'
| 'readwrite-primary'
| 'readonly-idempotent'
| 'readwrite-idempotent'
| 'readwrite-primary-idempotent';

/**
* An IndexedDB-backed instance of Persistence. Data is stored persistently
* across sessions.
Expand Down Expand Up @@ -320,7 +329,7 @@ export class IndexedDbPersistence implements Persistence {
})
.then(() => {
return this.simpleDb.runTransaction(
'readonly',
'readonly-idempotent',
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to modify this.listenSequence in the transaction.

Why not move that out of the transaction and into the first then block below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a bunch of calls in the SDK that follow this pattern and re-compute a value inside a transaction, but the re-computation has no side effects. In this case, refactoring the change out of this transaction is pretty simple, so I applied the change. It might not always be so simple in other cases though.

[DbTargetGlobal.store],
txn => {
return getHighestListenSequenceNumber(txn).next(
Expand Down Expand Up @@ -645,7 +654,7 @@ export class IndexedDbPersistence implements Persistence {
this.detachVisibilityHandler();
this.detachWindowUnloadHook();
await this.simpleDb.runTransaction(
'readwrite',
'readwrite-idempotent',
[DbPrimaryClient.store, DbClientMetadata.store],
txn => {
return this.releasePrimaryLeaseIfHeld(txn).next(() =>
Expand Down Expand Up @@ -677,7 +686,7 @@ export class IndexedDbPersistence implements Persistence {

getActiveClients(): Promise<ClientId[]> {
return this.simpleDb.runTransaction(
'readonly',
'readonly-idempotent',
[DbClientMetadata.store],
txn => {
return clientMetadataStore(txn)
Expand Down Expand Up @@ -742,17 +751,28 @@ export class IndexedDbPersistence implements Persistence {

runTransaction<T>(
action: string,
mode: 'readonly' | 'readwrite' | 'readwrite-primary',
mode: IndexedDbTransactionMode,
transactionOperation: (
transaction: PersistenceTransaction
) => PersistencePromise<T>
): Promise<T> {
log.debug(LOG_TAG, 'Starting transaction:', action);

// TODO(schmidt-sebastian): Simplify once all transactions are idempotent.
const idempotent = mode.endsWith('idempotent');
const readonly = mode.startsWith('readonly');
const simpleDbMode = readonly
? idempotent
? 'readonly-idempotent'
: 'readonly'
: idempotent
? 'readwrite-idempotent'
: 'readwrite';

// Do all transactions as readwrite against all object stores, since we
// are the only reader/writer.
return this.simpleDb.runTransaction(
mode === 'readonly' ? 'readonly' : 'readwrite',
simpleDbMode,
ALL_STORES,
simpleDbTxn => {
if (mode === 'readwrite-primary') {
Expand Down
101 changes: 78 additions & 23 deletions packages/firestore/src/local/simple_db.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,19 @@ import { PersistencePromise } from './persistence_promise';

const LOG_TAG = 'SimpleDb';

/**
* The maximum number of retry attempts for an IndexedDb transaction that fails
* with a DOMException.
*/
const TRANSACTION_RETRY_COUNT = 3;

// The different modes supported by `SimpleDb.runTransaction()`
type SimpleDbTransactionMode =
| 'readonly'
| 'readwrite'
| 'readonly-idempotent'
| 'readwrite-idempotent';

export interface SimpleDbSchemaConverter {
createOrUpgrade(
db: IDBDatabase,
Expand Down Expand Up @@ -242,32 +255,64 @@ export class SimpleDb {
};
}

runTransaction<T>(
mode: 'readonly' | 'readwrite',
async runTransaction<T>(
mode: SimpleDbTransactionMode,
objectStores: string[],
transactionFn: (transaction: SimpleDbTransaction) => PersistencePromise<T>
): Promise<T> {
const transaction = SimpleDbTransaction.open(this.db, mode, objectStores);
const transactionFnResult = transactionFn(transaction)
.catch(error => {
// Abort the transaction if there was an error.
transaction.abort(error);
// We cannot actually recover, and calling `abort()` will cause the transaction's
// completion promise to be rejected. This in turn means that we won't use
// `transactionFnResult` below. We return a rejection here so that we don't add the
// possibility of returning `void` to the type of `transactionFnResult`.
return PersistencePromise.reject<T>(error);
})
.toPromise();

// As noted above, errors are propagated by aborting the transaction. So
// we swallow any error here to avoid the browser logging it as unhandled.
transactionFnResult.catch(() => {});

// Wait for the transaction to complete (i.e. IndexedDb's onsuccess event to
// fire), but still return the original transactionFnResult back to the
// caller.
return transaction.completionPromise.then(() => transactionFnResult);
const readonly = mode.startsWith('readonly');
const idempotent = mode.endsWith('idempotent');
let attemptNumber = 0;

while (true) {
++attemptNumber;

const transaction = SimpleDbTransaction.open(
this.db,
readonly ? 'readonly' : 'readwrite',
objectStores
);
try {
const transactionFnResult = transactionFn(transaction)
.catch(error => {
// Abort the transaction if there was an error.
transaction.abort(error);
// We cannot actually recover, and calling `abort()` will cause the transaction's
// completion promise to be rejected. This in turn means that we won't use
// `transactionFnResult` below. We return a rejection here so that we don't add the
// possibility of returning `void` to the type of `transactionFnResult`.
return PersistencePromise.reject<T>(error);
})
.toPromise();

// As noted above, errors are propagated by aborting the transaction. So
// we swallow any error here to avoid the browser logging it as unhandled.
transactionFnResult.catch(() => {});

// Wait for the transaction to complete (i.e. IndexedDb's onsuccess event to
// fire), but still return the original transactionFnResult back to the
// caller.
await transaction.completionPromise;
return transactionFnResult;
} catch (e) {
// TODO(schmidt-sebastian): We could probably be smarter about this and
// not retry exceptions that are likely unrecoverable (such as quota
// exceeded errors).
const retryable =
idempotent &&
isDomException(e) &&
attemptNumber < TRANSACTION_RETRY_COUNT;
debug(
'Transaction failed with error: %s. Retrying: %s.',
e.message,
retryable
);

if (!retryable) {
return Promise.reject(e);
}
}
}
}

close(): void {
Expand Down Expand Up @@ -755,3 +800,13 @@ function checkForAndReportiOSError(error: DOMException): Error {
}
return error;
}

/** Checks whether an error is a DOMException (e.g. as thrown by IndexedDb). */
function isDomException(error: Error): boolean {
// DOMException is not a global type in Node with persistence, and hence we
// check the constructor name if the type in unknown.
return (
(typeof DOMException !== 'undefined' && error instanceof DOMException) ||
error.constructor.name === 'DOMException'
);
}
12 changes: 9 additions & 3 deletions packages/firestore/test/unit/local/encoded_resource_path.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ async function assertOrdered(paths: ResourcePath[]): Promise<void> {

const selected: string[] = [];
await runTransaction(simpleStore => {
selected.splice(0); // The function might get re-run.
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be cleaner if the selected array were just returned from the transaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's much cleaner. Updated.

return simpleStore.iterate({ keysOnly: true }, key => {
selected.push(key);
});
Expand Down Expand Up @@ -216,7 +217,12 @@ function runTransaction<T>(
transaction: SimpleDbTransaction
) => PersistencePromise<T>
): Promise<T> {
return db.runTransaction<T>('readwrite', ['test'], txn => {
return fn(txn.store<string, boolean>('test'), txn);
});
return db.runTransaction<T>(
'readwrite-idempotent',
['test'],
txn => {
return fn(txn.store<string, boolean>('test'), txn);
}
);

}
34 changes: 17 additions & 17 deletions packages/firestore/test/unit/local/indexeddb_persistence.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
return withDb(2, db => {
const sdb = new SimpleDb(db);
return sdb.runTransaction(
'readwrite',
'readwrite-idempotent',
[DbTarget.store, DbTargetGlobal.store, DbMutationBatch.store],
txn => {
const targets = txn.store<DbTargetKey, DbTarget>(DbTarget.store);
Expand Down Expand Up @@ -252,7 +252,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {

const sdb = new SimpleDb(db);
return sdb.runTransaction(
'readwrite',
'readwrite-idempotent',
[DbTarget.store, DbTargetGlobal.store, DbMutationBatch.store],
txn => {
const targets = txn.store<DbTargetKey, DbTarget>(DbTarget.store);
Expand Down Expand Up @@ -317,7 +317,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {

return withDb(3, db => {
const sdb = new SimpleDb(db);
return sdb.runTransaction('readwrite', [DbMutationBatch.store], txn => {
return sdb.runTransaction('readwrite-idempotent', [DbMutationBatch.store], txn => {
const store = txn.store(DbMutationBatch.store);
return PersistencePromise.forEach(
testMutations,
Expand All @@ -330,7 +330,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
expect(getAllObjectStores(db)).to.have.members(V4_STORES);

const sdb = new SimpleDb(db);
return sdb.runTransaction('readwrite', [DbMutationBatch.store], txn => {
return sdb.runTransaction('readwrite-idempotent', [DbMutationBatch.store], txn => {
const store = txn.store<DbMutationBatchKey, DbMutationBatch>(
DbMutationBatch.store
);
Expand Down Expand Up @@ -422,7 +422,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
return withDb(4, db => {
const sdb = new SimpleDb(db);
// We can only use the V4 stores here, since that's as far as we've upgraded.
return sdb.runTransaction('readwrite', V4_STORES, txn => {
return sdb.runTransaction('readwrite-idempotent', V4_STORES, txn => {
const mutationBatchStore = txn.store<
DbMutationBatchKey,
DbMutationBatch
Expand Down Expand Up @@ -471,7 +471,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {

const sdb = new SimpleDb(db);
// There is no V5_STORES, continue using V4.
return sdb.runTransaction('readwrite', V4_STORES, txn => {
return sdb.runTransaction('readwrite-idempotent', V4_STORES, txn => {
const mutationBatchStore = txn.store<
DbMutationBatchKey,
DbMutationBatch
Expand Down Expand Up @@ -523,7 +523,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
}));
// V5 stores doesn't exist
const sdb = new SimpleDb(db);
return sdb.runTransaction('readwrite', V4_STORES, txn => {
return sdb.runTransaction('readwrite-idempotent', V4_STORES, txn => {
const store = txn.store<DbRemoteDocumentKey, DbRemoteDocument>(
DbRemoteDocument.store
);
Expand All @@ -536,7 +536,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
});
await withDb(6, db => {
const sdb = new SimpleDb(db);
return sdb.runTransaction('readonly', V6_STORES, txn => {
return sdb.runTransaction('readwrite-idempotent', V6_STORES, txn => {
const store = txn.store<
DbRemoteDocumentGlobalKey,
DbRemoteDocumentGlobal
Expand All @@ -559,7 +559,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
const serializer = TEST_SERIALIZER;

const sdb = new SimpleDb(db);
return sdb.runTransaction('readwrite', V6_STORES, txn => {
return sdb.runTransaction('readwrite-idempotent', V6_STORES, txn => {
const targetGlobalStore = txn.store<DbTargetGlobalKey, DbTargetGlobal>(
DbTargetGlobal.store
);
Expand Down Expand Up @@ -610,7 +610,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
// Now run the migration and verify
await withDb(7, db => {
const sdb = new SimpleDb(db);
return sdb.runTransaction('readonly', V6_STORES, txn => {
return sdb.runTransaction('readwrite-idempotent', V6_STORES, txn => {
const targetDocumentStore = txn.store<
DbTargetDocumentKey,
DbTargetDocument
Expand Down Expand Up @@ -661,7 +661,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {

await withDb(7, db => {
const sdb = new SimpleDb(db);
return sdb.runTransaction('readwrite', V6_STORES, txn => {
return sdb.runTransaction('readwrite-idempotent', V6_STORES, txn => {
const remoteDocumentStore = txn.store<
DbRemoteDocumentKey,
DbRemoteDocument
Expand Down Expand Up @@ -698,7 +698,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
// Migrate to v8 and verify index entries.
await withDb(8, db => {
const sdb = new SimpleDb(db);
return sdb.runTransaction('readwrite', V8_STORES, txn => {
return sdb.runTransaction('readwrite-idempotent', V8_STORES, txn => {
const collectionParentsStore = txn.store<
DbCollectionParentKey,
DbCollectionParent
Expand Down Expand Up @@ -740,7 +740,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {

await withDb(8, db => {
const sdb = new SimpleDb(db);
return sdb.runTransaction('readwrite', V8_STORES, txn => {
return sdb.runTransaction('readwrite-idempotent', V8_STORES, txn => {
const remoteDocumentStore = txn.store<
DbRemoteDocumentKey,
DbRemoteDocument
Expand Down Expand Up @@ -769,7 +769,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
// Migrate to v9 and verify that new documents are indexed.
await withDb(9, db => {
const sdb = new SimpleDb(db);
return sdb.runTransaction('readwrite', V8_STORES, txn => {
return sdb.runTransaction('readwrite-idempotent', V8_STORES, txn => {
const remoteDocumentStore = txn.store<
DbRemoteDocumentKey,
DbRemoteDocument
Expand Down Expand Up @@ -816,7 +816,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {

await withDb(9, db => {
const sdb = new SimpleDb(db);
return sdb.runTransaction('readwrite', V8_STORES, txn => {
return sdb.runTransaction('readwrite-idempotent', V8_STORES, txn => {
return addDocs(txn, oldDocPaths, /* version= */ 1).next(() =>
addDocs(txn, newDocPaths, /* version= */ 2).next(() => {
const remoteDocumentStore = txn.store<
Expand Down Expand Up @@ -850,7 +850,7 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {

await withDb(9, db => {
const sdb = new SimpleDb(db);
return sdb.runTransaction('readwrite', V8_STORES, txn => {
return sdb.runTransaction('readwrite-idempotent', V8_STORES, txn => {
return addDocs(txn, oldDocPaths, /* version= */ 1).next(() =>
addDocs(txn, newDocPaths, /* version= */ 2).next(() => {
const remoteDocumentStore = txn.store<
Expand Down Expand Up @@ -912,7 +912,7 @@ describe('IndexedDb: canActAsPrimary', () => {
SCHEMA_VERSION,
new SchemaConverter(TEST_SERIALIZER)
);
await simpleDb.runTransaction('readwrite', [DbPrimaryClient.store], txn => {
await simpleDb.runTransaction('readwrite-idempotent', [DbPrimaryClient.store], txn => {
const primaryStore = txn.store<DbPrimaryClientKey, DbPrimaryClient>(
DbPrimaryClient.store
);
Expand Down
Loading