Skip to content

Commit 6ed49d3

Browse files
schmidt-sebastianschmidt-sebastian
committed
First round of feedback
1 parent 10c2f8e commit 6ed49d3

File tree

6 files changed

+468
-584
lines changed

6 files changed

+468
-584
lines changed

‎packages/firestore/src/local/indexeddb_persistence.ts

+55-63
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ export class IndexedDbTransaction extends PersistenceTransaction {
122122
}
123123

124124
// The different modes supported by `IndexedDbPersistence.runTransaction()`
125-
type TransactionMode =
125+
type IndexedDbTransactionMode =
126126
| 'readonly'
127127
| 'readwrite'
128128
| 'readwrite-primary'
@@ -330,7 +330,6 @@ export class IndexedDbPersistence implements Persistence {
330330
.then(() => {
331331
return this.simpleDb.runTransaction(
332332
'readonly',
333-
/* idempotent= */ false,
334333
[DbTargetGlobal.store],
335334
txn => {
336335
return getHighestListenSequenceNumber(txn).next(
@@ -354,11 +353,8 @@ export class IndexedDbPersistence implements Persistence {
354353
}
355354

356355
private startRemoteDocumentCache(): Promise<void> {
357-
return this.simpleDb.runTransaction(
358-
'readonly',
359-
/* idempotent= */ false,
360-
ALL_STORES,
361-
txn => this.remoteDocumentCache.start(txn)
356+
return this.simpleDb.runTransaction('readonly', ALL_STORES, txn =>
357+
this.remoteDocumentCache.start(txn)
362358
);
363359
}
364360

@@ -404,52 +400,47 @@ export class IndexedDbPersistence implements Persistence {
404400
* primary lease.
405401
*/
406402
private updateClientMetadataAndTryBecomePrimary(): Promise<void> {
407-
return this.simpleDb.runTransaction(
408-
'readwrite',
409-
/* idempotent= */ false,
410-
ALL_STORES,
411-
txn => {
412-
const metadataStore = clientMetadataStore(txn);
413-
return metadataStore
414-
.put(
415-
new DbClientMetadata(
416-
this.clientId,
417-
Date.now(),
418-
this.networkEnabled,
419-
this.inForeground
420-
)
403+
return this.simpleDb.runTransaction('readwrite', ALL_STORES, txn => {
404+
const metadataStore = clientMetadataStore(txn);
405+
return metadataStore
406+
.put(
407+
new DbClientMetadata(
408+
this.clientId,
409+
Date.now(),
410+
this.networkEnabled,
411+
this.inForeground
421412
)
422-
.next(() => {
423-
if (this.isPrimary) {
424-
return this.verifyPrimaryLease(txn).next(success => {
425-
if (!success) {
426-
this.isPrimary = false;
427-
this.queue.enqueueAndForget(() =>
428-
this.primaryStateListener(false)
429-
);
430-
}
431-
});
432-
}
433-
})
434-
.next(() => this.canActAsPrimary(txn))
435-
.next(canActAsPrimary => {
436-
const wasPrimary = this.isPrimary;
437-
this.isPrimary = canActAsPrimary;
438-
439-
if (wasPrimary !== this.isPrimary) {
440-
this.queue.enqueueAndForget(() =>
441-
this.primaryStateListener(this.isPrimary)
442-
);
443-
}
413+
)
414+
.next(() => {
415+
if (this.isPrimary) {
416+
return this.verifyPrimaryLease(txn).next(success => {
417+
if (!success) {
418+
this.isPrimary = false;
419+
this.queue.enqueueAndForget(() =>
420+
this.primaryStateListener(false)
421+
);
422+
}
423+
});
424+
}
425+
})
426+
.next(() => this.canActAsPrimary(txn))
427+
.next(canActAsPrimary => {
428+
const wasPrimary = this.isPrimary;
429+
this.isPrimary = canActAsPrimary;
430+
431+
if (wasPrimary !== this.isPrimary) {
432+
this.queue.enqueueAndForget(() =>
433+
this.primaryStateListener(this.isPrimary)
434+
);
435+
}
444436

445-
if (wasPrimary && !this.isPrimary) {
446-
return this.releasePrimaryLeaseIfHeld(txn);
447-
} else if (this.isPrimary) {
448-
return this.acquireOrExtendPrimaryLease(txn);
449-
}
450-
});
451-
}
452-
);
437+
if (wasPrimary && !this.isPrimary) {
438+
return this.releasePrimaryLeaseIfHeld(txn);
439+
} else if (this.isPrimary) {
440+
return this.acquireOrExtendPrimaryLease(txn);
441+
}
442+
});
443+
});
453444
}
454445

455446
private verifyPrimaryLease(
@@ -664,7 +655,6 @@ export class IndexedDbPersistence implements Persistence {
664655
this.detachWindowUnloadHook();
665656
await this.simpleDb.runTransaction(
666657
'readwrite',
667-
/* idempotent= */ false,
668658
[DbPrimaryClient.store, DbClientMetadata.store],
669659
txn => {
670660
return this.releasePrimaryLeaseIfHeld(txn).next(() =>
@@ -697,7 +687,6 @@ export class IndexedDbPersistence implements Persistence {
697687
getActiveClients(): Promise<ClientId[]> {
698688
return this.simpleDb.runTransaction(
699689
'readonly',
700-
/* idempotent= */ false,
701690
[DbClientMetadata.store],
702691
txn => {
703692
return clientMetadataStore(txn)
@@ -762,26 +751,29 @@ export class IndexedDbPersistence implements Persistence {
762751

763752
runTransaction<T>(
764753
action: string,
765-
mode: TransactionMode,
754+
mode: IndexedDbTransactionMode,
766755
transactionOperation: (
767756
transaction: PersistenceTransaction
768757
) => PersistencePromise<T>
769758
): Promise<T> {
770759
log.debug(LOG_TAG, 'Starting transaction:', action);
771760

772-
const idempotent =
773-
[
774-
'readonly-idempotent',
775-
'readwrite-idempotent',
776-
'readwrite-primary-idempotent'
777-
].indexOf(mode) !== -1;
778-
const readonly = ['readonly', 'readonly-idempotent'].indexOf(mode) !== -1;
761+
// TODO(schmidt-sebastian): Once all transactions are idempotent, simplify
762+
// this.
763+
const idempotent = mode.endsWith('idempotent');
764+
const readonly = mode.startsWith('readonly');
765+
const simpleDbMode = readonly
766+
? idempotent
767+
? 'readonly-idempotent'
768+
: 'readonly'
769+
: idempotent
770+
? 'readwrite-idempotent'
771+
: 'readwrite';
779772

780773
// Do all transactions as readwrite against all object stores, since we
781774
// are the only reader/writer.
782775
return this.simpleDb.runTransaction(
783-
readonly ? 'readonly' : 'readwrite',
784-
idempotent,
776+
simpleDbMode,
785777
ALL_STORES,
786778
simpleDbTxn => {
787779
if (mode === 'readwrite-primary') {

‎packages/firestore/src/local/simple_db.ts

+24-13
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,13 @@ const LOG_TAG = 'SimpleDb';
3131
*/
3232
const TRANSACTION_RETRY_COUNT = 3;
3333

34+
// The different modes supported by `SimpleDb.runTransaction()`
35+
type SimpleDbTransactionMode =
36+
| 'readonly'
37+
| 'readwrite'
38+
| 'readonly-idempotent'
39+
| 'readwrite-idempotent';
40+
3441
export interface SimpleDbSchemaConverter {
3542
createOrUpgrade(
3643
db: IDBDatabase,
@@ -249,17 +256,22 @@ export class SimpleDb {
249256
}
250257

251258
async runTransaction<T>(
252-
mode: 'readonly' | 'readwrite',
253-
idempotent: boolean,
259+
mode: SimpleDbTransactionMode,
254260
objectStores: string[],
255261
transactionFn: (transaction: SimpleDbTransaction) => PersistencePromise<T>
256262
): Promise<T> {
263+
let readonly = mode.startsWith('idempotent');
264+
let idempotent = mode.endsWith('idempotent');
257265
let attemptNumber = 0;
258266

259267
while (true) {
260268
++attemptNumber;
261269

262-
const transaction = SimpleDbTransaction.open(this.db, mode, objectStores);
270+
const transaction = SimpleDbTransaction.open(
271+
this.db,
272+
readonly ? 'readonly' : 'readwrite',
273+
objectStores
274+
);
263275
try {
264276
const transactionFnResult = transactionFn(transaction)
265277
.catch(error => {
@@ -283,8 +295,9 @@ export class SimpleDb {
283295
await transaction.completionPromise;
284296
return transactionFnResult;
285297
} catch (e) {
286-
// TODO: We could probably be smarter about this an not retry exceptions
287-
// that are likely unrecoverable (such as quota exceeded errors).
298+
// TODO(schmidt-sebastian): We could probably be smarter about this and
299+
// not retry exceptions that are likely unrecoverable (such as quota
300+
// exceeded errors).
288301
const retryable =
289302
idempotent &&
290303
isDomException(e) &&
@@ -788,14 +801,12 @@ function checkForAndReportiOSError(error: DOMException): Error {
788801
return error;
789802
}
790803

791-
/**
792-
* Checks whether an error is a DOMException (e.g. as thrown by IndexedDb).
793-
*
794-
* Supports both browsers and Node with persistence.
795-
*/
796-
function isDomException(e: Error): boolean {
804+
/** Checks whether an error is a DOMException (e.g. as thrown by IndexedDb). */
805+
function isDomException(error: Error): boolean {
806+
// DOMException is not a global type in Node with persistence, and hence we
807+
// check the constructor name if the type in unknown.
797808
return (
798-
(typeof DOMException !== 'undefined' && e instanceof DOMException) ||
799-
e.constructor.name === 'DOMException'
809+
(typeof DOMException !== 'undefined' && error instanceof DOMException) ||
810+
error.constructor.name === 'DOMException'
800811
);
801812
}

‎packages/firestore/test/unit/local/encoded_resource_path.test.ts

+3-8
Original file line numberDiff line numberDiff line change
@@ -216,12 +216,7 @@ function runTransaction<T>(
216216
transaction: SimpleDbTransaction
217217
) => PersistencePromise<T>
218218
): Promise<T> {
219-
return db.runTransaction<T>(
220-
'readwrite',
221-
/* idempotent= */ false,
222-
['test'],
223-
txn => {
224-
return fn(txn.store<string, boolean>('test'), txn);
225-
}
226-
);
219+
return db.runTransaction<T>('readwrite', ['test'], txn => {
220+
return fn(txn.store<string, boolean>('test'), txn);
221+
});
227222
}

0 commit comments

Comments
 (0)