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 all 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
27 changes: 12 additions & 15 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 @@ -327,20 +327,17 @@ export class IndexedDbPersistence implements Persistence {

return this.startRemoteDocumentCache();
})
.then(() => {
return this.simpleDb.runTransaction(
'readonly',
.then(() =>
this.simpleDb.runTransaction(
'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(
highestListenSequenceNumber => {
this.listenSequence = new ListenSequence(
highestListenSequenceNumber,
this.sequenceNumberSyncer
);
}
);
}
txn => getHighestListenSequenceNumber(txn)
)
)
.then(highestListenSequenceNumber => {
this.listenSequence = new ListenSequence(
highestListenSequenceNumber,
this.sequenceNumberSyncer
);
})
.then(() => {
Expand Down Expand Up @@ -654,7 +651,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 @@ -686,7 +683,7 @@ export class IndexedDbPersistence implements Persistence {

getActiveClients(): Promise<ClientId[]> {
return this.simpleDb.runTransaction(
'readonly',
'readonly-idempotent',
[DbClientMetadata.store],
txn => {
return clientMetadataStore(txn)
Expand Down
14 changes: 8 additions & 6 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 @@ -182,11 +182,13 @@ async function assertOrdered(paths: ResourcePath[]): Promise<void> {
});
paths.reverse();

const selected: string[] = [];
await runTransaction(simpleStore => {
return simpleStore.iterate({ keysOnly: true }, key => {
selected.push(key);
});
const selected = await runTransaction(simpleStore => {
const allKeys: string[] = [];
return simpleStore
.iterate({ keysOnly: true }, key => {
allKeys.push(key);
})
.next(() => allKeys);
});

// Finally, verify all the orderings.
Expand Down Expand Up @@ -216,7 +218,7 @@ function runTransaction<T>(
transaction: SimpleDbTransaction
) => PersistencePromise<T>
): Promise<T> {
return db.runTransaction<T>('readwrite', ['test'], txn => {
return db.runTransaction<T>('readwrite-idempotent', ['test'], txn => {
return fn(txn.store<string, boolean>('test'), txn);
});
}
106 changes: 59 additions & 47 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,39 +317,47 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {

return withDb(3, db => {
const sdb = new SimpleDb(db);
return sdb.runTransaction('readwrite', [DbMutationBatch.store], txn => {
const store = txn.store(DbMutationBatch.store);
return PersistencePromise.forEach(
testMutations,
(testMutation: DbMutationBatch) => store.put(testMutation)
);
});
return sdb.runTransaction(
'readwrite-idempotent',
[DbMutationBatch.store],
txn => {
const store = txn.store(DbMutationBatch.store);
return PersistencePromise.forEach(
testMutations,
(testMutation: DbMutationBatch) => store.put(testMutation)
);
}
);
}).then(() =>
withDb(4, db => {
expect(db.version).to.be.equal(4);
expect(getAllObjectStores(db)).to.have.members(V4_STORES);

const sdb = new SimpleDb(db);
return sdb.runTransaction('readwrite', [DbMutationBatch.store], txn => {
const store = txn.store<DbMutationBatchKey, DbMutationBatch>(
DbMutationBatch.store
);
let p = PersistencePromise.forEach(
testMutations,
(testMutation: DbMutationBatch) =>
store.get(testMutation.batchId).next(mutationBatch => {
expect(mutationBatch).to.deep.equal(testMutation);
})
);
p = p.next(() => {
store
.add({} as any) // eslint-disable-line @typescript-eslint/no-explicit-any
.next(batchId => {
expect(batchId).to.equal(43);
});
});
return p;
});
return sdb.runTransaction(
'readwrite-idempotent',
[DbMutationBatch.store],
txn => {
const store = txn.store<DbMutationBatchKey, DbMutationBatch>(
DbMutationBatch.store
);
let p = PersistencePromise.forEach(
testMutations,
(testMutation: DbMutationBatch) =>
store.get(testMutation.batchId).next(mutationBatch => {
expect(mutationBatch).to.deep.equal(testMutation);
})
);
p = p.next(() => {
store
.add({} as any) // eslint-disable-line @typescript-eslint/no-explicit-any
.next(batchId => {
expect(batchId).to.equal(43);
});
});
return p;
}
);
})
);
});
Expand Down Expand Up @@ -422,7 +430,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 +479,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 +531,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 +544,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 +567,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 +618,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 +669,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 +706,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 +748,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 +777,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 +824,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 +858,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,12 +920,16 @@ describe('IndexedDb: canActAsPrimary', () => {
SCHEMA_VERSION,
new SchemaConverter(TEST_SERIALIZER)
);
await simpleDb.runTransaction('readwrite', [DbPrimaryClient.store], txn => {
const primaryStore = txn.store<DbPrimaryClientKey, DbPrimaryClient>(
DbPrimaryClient.store
);
return primaryStore.delete(DbPrimaryClient.key);
});
await simpleDb.runTransaction(
'readwrite-idempotent',
[DbPrimaryClient.store],
txn => {
const primaryStore = txn.store<DbPrimaryClientKey, DbPrimaryClient>(
DbPrimaryClient.store
);
return primaryStore.delete(DbPrimaryClient.key);
}
);
simpleDb.close();
}

Expand Down
6 changes: 3 additions & 3 deletions packages/firestore/test/unit/local/simple_db.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ describe('SimpleDb', () => {
transaction: SimpleDbTransaction
) => PersistencePromise<T>
): Promise<T> {
return db.runTransaction<T>('readwrite', ['users'], txn => {
return db.runTransaction<T>('readwrite-idempotent', ['users'], txn => {
return fn(txn.store<number, User>('users'), txn);
});
}
Expand Down Expand Up @@ -481,7 +481,7 @@ describe('SimpleDb', () => {
['foo', 'd'],
['foob']
];
await db.runTransaction('readwrite', ['users', 'docs'], txn => {
await db.runTransaction('readwrite-idempotent', ['users', 'docs'], txn => {
const docsStore = txn.store<string[], string>('docs');
return PersistencePromise.waitFor(
keys.map(key => {
Expand All @@ -491,7 +491,7 @@ describe('SimpleDb', () => {
);
});

await db.runTransaction('readonly', ['docs'], txn => {
await db.runTransaction('readonly-idempotent', ['docs'], txn => {
const store = txn.store<string[], string>('docs');
const range = IDBKeyRange.bound(['foo'], ['foo', 'c']);
return store.loadAll(range).next(results => {
Expand Down
16 changes: 10 additions & 6 deletions packages/firestore/test/unit/specs/spec_test_runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1650,11 +1650,15 @@ async function clearCurrentPrimaryLease(): Promise<void> {
SCHEMA_VERSION,
new SchemaConverter(TEST_SERIALIZER)
);
await db.runTransaction('readwrite', [DbPrimaryClient.store], txn => {
const primaryClientStore = txn.store<DbPrimaryClientKey, DbPrimaryClient>(
DbPrimaryClient.store
);
return primaryClientStore.delete(DbPrimaryClient.key);
});
await db.runTransaction(
'readwrite-idempotent',
[DbPrimaryClient.store],
txn => {
const primaryClientStore = txn.store<DbPrimaryClientKey, DbPrimaryClient>(
DbPrimaryClient.store
);
return primaryClientStore.delete(DbPrimaryClient.key);
}
);
db.close();
}