Skip to content

Mark mostly readonly calls as idempotent #2252

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

Mark mostly readonly calls as idempotent #2252

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
06f9e83
Mark (most) readonly calls as idempotent
schmidt-sebastian Oct 9, 2019
2dc4170
Fix unit tests
schmidt-sebastian Oct 10, 2019
b948238
Merge
schmidt-sebastian Oct 10, 2019
525d2ad
Remove IndexedDbTransactionMode
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
12 changes: 2 additions & 10 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 @@ -62,6 +62,7 @@ import { MutationQueue } from './mutation_queue';
import {
Persistence,
PersistenceTransaction,
PersistenceTransactionMode,
PrimaryStateListener,
ReferenceDelegate
} from './persistence';
Expand Down Expand Up @@ -121,15 +122,6 @@ 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 @@ -751,7 +743,7 @@ export class IndexedDbPersistence implements Persistence {

runTransaction<T>(
action: string,
mode: IndexedDbTransactionMode,
mode: PersistenceTransactionMode,
transactionOperation: (
transaction: PersistenceTransaction
) => PersistencePromise<T>
Expand Down
82 changes: 45 additions & 37 deletions packages/firestore/src/local/local_store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ export class LocalStore {
lookupMutationDocuments(batchId: BatchId): Promise<MaybeDocumentMap | null> {
return this.persistence.runTransaction(
'Lookup mutation documents',
'readonly',
'readonly-idempotent',
txn => {
return this.mutationQueue
.lookupMutationKeys(txn, batchId)
Expand Down Expand Up @@ -412,7 +412,7 @@ export class LocalStore {
getHighestUnacknowledgedBatchId(): Promise<BatchId> {
return this.persistence.runTransaction(
'Get highest unacknowledged batch id',
'readonly',
'readonly-idempotent',
txn => {
return this.mutationQueue.getHighestUnacknowledgedBatchId(txn);
}
Expand All @@ -423,7 +423,7 @@ export class LocalStore {
getLastStreamToken(): Promise<ProtoByteString> {
return this.persistence.runTransaction(
'Get last stream token',
'readonly',
'readonly-idempotent',
txn => {
return this.mutationQueue.getLastStreamToken(txn);
}
Expand All @@ -438,7 +438,7 @@ export class LocalStore {
setLastStreamToken(streamToken: ProtoByteString): Promise<void> {
return this.persistence.runTransaction(
'Set last stream token',
'readwrite-primary',
'readwrite-primary-idempotent',
txn => {
return this.mutationQueue.setLastStreamToken(txn, streamToken);
}
Expand All @@ -452,7 +452,7 @@ export class LocalStore {
getLastRemoteSnapshotVersion(): Promise<SnapshotVersion> {
return this.persistence.runTransaction(
'Get last remote snapshot version',
'readonly',
'readonly-idempotent',
txn => this.queryCache.getLastRemoteSnapshotVersion(txn)
);
}
Expand Down Expand Up @@ -731,7 +731,7 @@ export class LocalStore {
nextMutationBatch(afterBatchId?: BatchId): Promise<MutationBatch | null> {
return this.persistence.runTransaction(
'Get next mutation batch',
'readonly',
'readonly-idempotent',
txn => {
if (afterBatchId === undefined) {
afterBatchId = BATCHID_UNKNOWN;
Expand All @@ -749,9 +749,13 @@ export class LocalStore {
* found - used for testing.
*/
readDocument(key: DocumentKey): Promise<MaybeDocument | null> {
return this.persistence.runTransaction('read document', 'readonly', txn => {
return this.localDocuments.getDocument(txn, key);
});
return this.persistence.runTransaction(
'read document',
'readonly-idempotent',
txn => {
return this.localDocuments.getDocument(txn, key);
}
);
}

/**
Expand Down Expand Up @@ -870,33 +874,37 @@ export class LocalStore {
let lastLimboFreeSnapshotVersion = SnapshotVersion.MIN;
let remoteKeys = documentKeySet();

return this.persistence.runTransaction('Execute query', 'readonly', txn => {
return this.getQueryData(txn, query)
.next(queryData => {
if (queryData) {
lastLimboFreeSnapshotVersion =
queryData.lastLimboFreeSnapshotVersion;
return this.queryCache
.getMatchingKeysForTargetId(txn, queryData.targetId)
.next(result => {
remoteKeys = result;
});
}
})
.next(() =>
this.queryEngine.getDocumentsMatchingQuery(
txn,
query,
usePreviousResults
? lastLimboFreeSnapshotVersion
: SnapshotVersion.MIN,
usePreviousResults ? remoteKeys : documentKeySet()
return this.persistence.runTransaction(
'Execute query',
'readonly-idempotent',
txn => {
return this.getQueryData(txn, query)
.next(queryData => {
if (queryData) {
lastLimboFreeSnapshotVersion =
queryData.lastLimboFreeSnapshotVersion;
return this.queryCache
.getMatchingKeysForTargetId(txn, queryData.targetId)
.next(result => {
remoteKeys = result;
});
}
})
.next(() =>
this.queryEngine.getDocumentsMatchingQuery(
txn,
query,
usePreviousResults
? lastLimboFreeSnapshotVersion
: SnapshotVersion.MIN,
usePreviousResults ? remoteKeys : documentKeySet()
)
)
)
.next(documents => {
return { documents, remoteKeys };
});
});
.next(documents => {
return { documents, remoteKeys };
});
}
);
}

/**
Expand All @@ -906,7 +914,7 @@ export class LocalStore {
remoteDocumentKeys(targetId: TargetId): Promise<DocumentKeySet> {
return this.persistence.runTransaction(
'Remote document keys',
'readonly',
'readonly-idempotent',
txn => {
return this.queryCache.getMatchingKeysForTargetId(txn, targetId);
}
Expand Down Expand Up @@ -988,7 +996,7 @@ export class LocalStore {
} else {
return this.persistence.runTransaction(
'Get query data',
'readonly',
'readonly-idempotent',
txn => {
return this.queryCache
.getQueryDataForTarget(txn, targetId)
Expand Down
3 changes: 2 additions & 1 deletion packages/firestore/src/local/memory_persistence.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import { MutationQueue } from './mutation_queue';
import {
Persistence,
PersistenceTransaction,
PersistenceTransactionMode,
PrimaryStateListener,
ReferenceDelegate
} from './persistence';
Expand Down Expand Up @@ -169,7 +170,7 @@ export class MemoryPersistence implements Persistence {

runTransaction<T>(
action: string,
mode: 'readonly' | 'readwrite' | 'readwrite-primary',
mode: PersistenceTransactionMode,
transactionOperation: (
transaction: PersistenceTransaction
) => PersistencePromise<T>
Expand Down
11 changes: 10 additions & 1 deletion packages/firestore/src/local/persistence.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,15 @@ export abstract class PersistenceTransaction {
abstract readonly currentSequenceNumber: ListenSequenceNumber;
}

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

/**
* Callback type for primary state notifications. This callback can be
* registered with the persistence layer to get notified when we transition from
Expand Down Expand Up @@ -255,7 +264,7 @@ export interface Persistence {
*/
runTransaction<T>(
action: string,
mode: 'readonly' | 'readwrite' | 'readwrite-primary',
mode: PersistenceTransactionMode,
transactionOperation: (
transaction: PersistenceTransaction
) => PersistencePromise<T>
Expand Down
8 changes: 8 additions & 0 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 @@ -273,6 +273,14 @@ export class SimpleDb {
objectStores
);
try {
// TODO(schmidt-sebastian): Remove this code/comment or find a way to
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this here for now to make my development process easier. This actually caught an issue during development, when I marked getNewDocumentChanges as idempotent. It is not yet since it modifies internal state that is used during execution.

// make this a test-only setting.
// // Horrible hack to verify that idempotent functions can be run more
// // than once.
// const transactionFnResult = (idempotent && attemptNumber === 1
// ? transactionFn(transaction)
// : PersistencePromise.resolve({} as T)
// ).next(() => transactionFn(transaction))
const transactionFnResult = transactionFn(transaction)
.catch(error => {
// Abort the transaction if there was an error.
Expand Down
46 changes: 25 additions & 21 deletions packages/firestore/test/unit/local/index_free_query_engine.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,27 +144,31 @@ describe('IndexFreeQueryEngine', () => {
'Encountered runQuery() call not wrapped in expectIndexFreeQuery()/expectFullCollectionQuery()'
);

return persistence.runTransaction('runQuery', 'readonly', txn => {
return queryCache
.getMatchingKeysForTargetId(txn, TEST_TARGET_ID)
.next(remoteKeys => {
return queryEngine
.getDocumentsMatchingQuery(
txn,
query,
lastLimboFreeSnapshot,
remoteKeys
)
.next(docs => {
const view = new View(query, remoteKeys);
const viewDocChanges = view.computeDocChanges(docs);
return view.applyChanges(
viewDocChanges,
/*updateLimboDocuments=*/ true
).snapshot!.docs;
});
});
});
return persistence.runTransaction(
'runQuery',
'readonly-idempotent',
txn => {
return queryCache
.getMatchingKeysForTargetId(txn, TEST_TARGET_ID)
.next(remoteKeys => {
return queryEngine
.getDocumentsMatchingQuery(
txn,
query,
lastLimboFreeSnapshot,
remoteKeys
)
.next(docs => {
const view = new View(query, remoteKeys);
const viewDocChanges = view.computeDocChanges(docs);
return view.applyChanges(
viewDocChanges,
/*updateLimboDocuments=*/ true
).snapshot!.docs;
});
});
}
);
}

beforeEach(async () => {
Expand Down
6 changes: 3 additions & 3 deletions packages/firestore/test/unit/local/local_store.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1546,7 +1546,7 @@ function genericLocalStoreTests(
// At this point, we have not yet confirmed that the query is limbo free.
let cachedQueryData = await persistence.runTransaction(
'getQueryData',
'readonly',
'readonly-idempotent',
txn => localStore.getQueryData(txn, query)
);
expect(
Expand All @@ -1559,7 +1559,7 @@ function genericLocalStoreTests(
]);
cachedQueryData = await persistence.runTransaction(
'getQueryData',
'readonly',
'readonly-idempotent',
txn => localStore.getQueryData(txn, query)
);
expect(cachedQueryData!.lastLimboFreeSnapshotVersion.isEqual(version(10)))
Expand All @@ -1572,7 +1572,7 @@ function genericLocalStoreTests(
if (!gcIsEager) {
cachedQueryData = await persistence.runTransaction(
'getQueryData',
'readonly',
'readonly-idempotent',
txn => localStore.getQueryData(txn, query)
);
expect(cachedQueryData!.lastLimboFreeSnapshotVersion.isEqual(version(10)))
Expand Down
4 changes: 2 additions & 2 deletions packages/firestore/test/unit/local/test_index_manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export class TestIndexManager {
addToCollectionParentIndex(collectionPath: ResourcePath): Promise<void> {
return this.persistence.runTransaction(
'addToCollectionParentIndex',
'readwrite',
'readwrite-idempotent',
txn => {
return this.indexManager.addToCollectionParentIndex(
txn,
Expand All @@ -45,7 +45,7 @@ export class TestIndexManager {
getCollectionParents(collectionId: string): Promise<ResourcePath[]> {
return this.persistence.runTransaction(
'getCollectionParents',
'readwrite',
'readonly-idempotent',
txn => {
return this.indexManager.getCollectionParents(txn, collectionId);
}
Expand Down
Loading