Skip to content

Ignore IndexedDB failure during lease refresh #3020

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 3 commits into from
May 14, 2020
Merged

Ignore IndexedDB failure during lease refresh #3020

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3073f60
Ignore IndexedDB failure during lease refresh
schmidt-sebastian May 5, 2020
d87dc1e
Rewrite everything
schmidt-sebastian May 6, 2020
8d08444
Review
schmidt-sebastian May 7, 2020
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
149 changes: 74 additions & 75 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 @@ -101,7 +101,7 @@ const MAX_PRIMARY_ELIGIBLE_AGE_MS = 5000;
const CLIENT_METADATA_REFRESH_INTERVAL_MS = 4000;
/** User-facing error when the primary lease is required but not available. */
const PRIMARY_LEASE_EXCLUSIVE_ERROR_MSG =
'Another tab has exclusive access to the persistence layer. ' +
'Failed to obtain exclusive access to the persistence layer. ' +
'To allow shared access, make sure to invoke ' +
'`enablePersistence()` with `synchronizeTabs:true` in all tabs.';
const UNSUPPORTED_PLATFORM_ERROR_MSG =
Expand Down Expand Up @@ -191,11 +191,12 @@ export class IndexedDbPersistence implements Persistence {
private readonly document: Document | null;
private readonly window: Window;

// Technically these types should be `| undefined` because they are
// Technically `simpleDb` should be `| undefined` because it is
// initialized asynchronously by start(), but that would be more misleading
// than useful.
private simpleDb!: SimpleDb;
private listenSequence!: ListenSequence;

private listenSequence: ListenSequence | null = null;

private _started = false;
private isPrimary = false;
Expand Down Expand Up @@ -287,7 +288,15 @@ export class IndexedDbPersistence implements Persistence {
// having the persistence lock), so it's the first thing we should do.
return this.updateClientMetadataAndTryBecomePrimary();
})
.then(() => {
.then(e => {
if (!this.isPrimary && !this.allowTabSynchronization) {
// Fail `start()` if `synchronizeTabs` is disabled and we cannot
// obtain the primary lease.
throw new FirestoreError(
Code.FAILED_PRECONDITION,
PRIMARY_LEASE_EXCLUSIVE_ERROR_MSG
);
}
this.attachVisibilityHandler();
this.attachWindowUnloadHook();

Expand Down Expand Up @@ -375,8 +384,10 @@ export class IndexedDbPersistence implements Persistence {
* primary lease.
*/
private updateClientMetadataAndTryBecomePrimary(): Promise<void> {
return this.simpleDb
.runTransaction('readwrite', ALL_STORES, txn => {
return this.runTransaction(
'updateClientMetadataAndTryBecomePrimary',
'readwrite',
txn => {
const metadataStore = clientMetadataStore(txn);
return metadataStore
.put(
Expand Down Expand Up @@ -409,10 +420,18 @@ export class IndexedDbPersistence implements Persistence {
return /* canActAsPrimary= */ false;
}
});
})
}
)
.catch(e => {
if (!this.allowTabSynchronization) {
throw e;
if (e.name === 'IndexedDbTransactionError') {
logDebug(LOG_TAG, 'Failed to extend owner lease: ', e);
// Proceed with the existing state. Any subsequent access to
// IndexedDB will verify the lease.
return this.isPrimary;
} else {
throw e;
}
}

logDebug(
Expand All @@ -433,7 +452,7 @@ export class IndexedDbPersistence implements Persistence {
}

private verifyPrimaryLease(
txn: SimpleDbTransaction
txn: PersistenceTransaction
): PersistencePromise<boolean> {
const store = primaryClientStore(txn);
return store.get(DbPrimaryClient.key).next(primaryClient => {
Expand All @@ -442,7 +461,7 @@ export class IndexedDbPersistence implements Persistence {
}

private removeClientMetadata(
txn: SimpleDbTransaction
txn: PersistenceTransaction
): PersistencePromise<void> {
const metadataStore = clientMetadataStore(txn);
return metadataStore.delete(this.clientId);
Expand Down Expand Up @@ -536,7 +555,7 @@ export class IndexedDbPersistence implements Persistence {
* (foreground) client should become leaseholder instead.
*/
private canActAsPrimary(
txn: SimpleDbTransaction
txn: PersistenceTransaction
): PersistencePromise<boolean> {
const store = primaryClientStore(txn);
return store
Expand All @@ -561,33 +580,10 @@ export class IndexedDbPersistence implements Persistence {
if (currentLeaseIsValid) {
if (this.isLocalClient(currentPrimary) && this.networkEnabled) {
return true;
}

if (!this.isLocalClient(currentPrimary)) {
if (!currentPrimary!.allowTabSynchronization) {
// Fail the `canActAsPrimary` check if the current leaseholder has
// not opted into multi-tab synchronization. If this happens at
// client startup, we reject the Promise returned by
// `enablePersistence()` and the user can continue to use Firestore
// with in-memory persistence.
// If this fails during a lease refresh, we will instead block the
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like removing this error here is a regression. Consider:

  • If the client successfully starts
  • goes into the background and loses access to IndexedDB
  • stays in the background for longer than the lease duration

If device comes back online the user opens another tab before our retry has succeeded in refreshing the lease, there's a very real chance we'll lose this race. Users don't even have to be mixing and matching modes for this to fail now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Summary of my last 5 minutes:

  • Gil is wrong.
  • Gil is right.
  • Gil is wrong.
  • Gil is right.
  • Gil it right and wrong.

The idea behind the lease refresh was it would be a "best effort" only operation. If if fails or stalls, any operation to IndexedDB should verify the expected state. That is:

  • "readwrite-primary" operations should verify that the tab has the lease or can become the leaseholder. This already happens.
  • "readwrite", "readonly" operations should verify that they are allowed to access IndexedDB (i.e. there is no primary tab without synchronizeTabs turned on). This is not happening right now. Instead, this is periodically verified via the code that I removed.

So, in conclusion: We should move this verification and run it as part of every transaction. I will leave the code in master and work on this in a separate PR.

// AsyncQueue from executing further operations. Note that this is
// acceptable since mixing & matching different `synchronizeTabs`
// settings is not supported.
//
// TODO(b/114226234): Remove this check when `synchronizeTabs` can
// no longer be turned off.
throw new FirestoreError(
Code.FAILED_PRECONDITION,
PRIMARY_LEASE_EXCLUSIVE_ERROR_MSG
);
}

} else if (!this.isLocalClient(currentPrimary)) {
return false;
}
}

if (this.networkEnabled && this.inForeground) {
} else if (this.networkEnabled && this.inForeground) {
return true;
}

Expand Down Expand Up @@ -645,15 +641,13 @@ export class IndexedDbPersistence implements Persistence {
}
this.detachVisibilityHandler();
this.detachWindowUnloadHook();
await this.simpleDb.runTransaction(
'readwrite',
[DbPrimaryClient.store, DbClientMetadata.store],
txn => {
return this.releasePrimaryLeaseIfHeld(txn).next(() =>
this.removeClientMetadata(txn)
);
}
);
await this.runTransaction('shutdown', 'readwrite', txn => {
return this.releasePrimaryLeaseIfHeld(txn).next(() =>
this.removeClientMetadata(txn)
);
}).catch(e => {
logDebug(LOG_TAG, 'Proceeding with shutdown despite failure: ', e);
});
this.simpleDb.close();

// Remove the entry marking the client as zombied from LocalStorage since
Expand Down Expand Up @@ -684,19 +678,15 @@ export class IndexedDbPersistence implements Persistence {
* PORTING NOTE: This is only used for Web multi-tab.
*/
getActiveClients(): Promise<ClientId[]> {
return this.simpleDb.runTransaction(
'readonly',
[DbClientMetadata.store],
txn => {
return clientMetadataStore(txn)
.loadAll()
.next(clients =>
this.filterActiveClients(clients, MAX_CLIENT_AGE_MS).map(
clientMetadata => clientMetadata.clientId
)
);
}
);
return this.runTransaction('getActiveClients', 'readonly', txn => {
return clientMetadataStore(txn)
.loadAll()
.next(clients =>
this.filterActiveClients(clients, MAX_CLIENT_AGE_MS).map(
clientMetadata => clientMetadata.clientId
)
);
});
}

static async clearPersistence(persistenceKey: string): Promise<void> {
Expand Down Expand Up @@ -767,7 +757,9 @@ export class IndexedDbPersistence implements Persistence {
.runTransaction(simpleDbMode, ALL_STORES, simpleDbTxn => {
persistenceTransaction = new IndexedDbTransaction(
simpleDbTxn,
this.listenSequence.next()
this.listenSequence
? this.listenSequence.next()
: ListenSequence.INVALID
);

if (mode === 'readwrite-primary') {
Expand All @@ -776,12 +768,12 @@ export class IndexedDbPersistence implements Persistence {
// executing transactionOperation(). This ensures that even if the
// transactionOperation takes a long time, we'll use a recent
// leaseTimestampMs in the extended (or newly acquired) lease.
return this.verifyPrimaryLease(simpleDbTxn)
return this.verifyPrimaryLease(persistenceTransaction)
.next(holdsPrimaryLease => {
if (holdsPrimaryLease) {
return /* holdsPrimaryLease= */ true;
}
return this.canActAsPrimary(simpleDbTxn);
return this.canActAsPrimary(persistenceTransaction);
})
.next(holdsPrimaryLease => {
if (!holdsPrimaryLease) {
Expand All @@ -800,14 +792,14 @@ export class IndexedDbPersistence implements Persistence {
return transactionOperation(persistenceTransaction);
})
.next(result => {
return this.acquireOrExtendPrimaryLease(simpleDbTxn).next(
() => result
);
return this.acquireOrExtendPrimaryLease(
persistenceTransaction
).next(() => result);
});
} else {
return this.verifyAllowTabSynchronization(simpleDbTxn).next(() =>
transactionOperation(persistenceTransaction)
);
return this.verifyAllowTabSynchronization(
persistenceTransaction
).next(() => transactionOperation(persistenceTransaction));
}
})
.then(result => {
Expand All @@ -823,7 +815,7 @@ export class IndexedDbPersistence implements Persistence {
// TODO(b/114226234): Remove this check when `synchronizeTabs` can no longer
// be turned off.
private verifyAllowTabSynchronization(
txn: SimpleDbTransaction
txn: PersistenceTransaction
): PersistencePromise<void> {
const store = primaryClientStore(txn);
return store.get(DbPrimaryClient.key).next(currentPrimary => {
Expand All @@ -836,7 +828,10 @@ export class IndexedDbPersistence implements Persistence {
!this.isClientZombied(currentPrimary.ownerId);

if (currentLeaseIsValid && !this.isLocalClient(currentPrimary)) {
if (!currentPrimary!.allowTabSynchronization) {
if (
!this.allowTabSynchronization ||
!currentPrimary!.allowTabSynchronization
) {
throw new FirestoreError(
Code.FAILED_PRECONDITION,
PRIMARY_LEASE_EXCLUSIVE_ERROR_MSG
Expand All @@ -851,7 +846,7 @@ export class IndexedDbPersistence implements Persistence {
* method does not verify that the client is eligible for this lease.
*/
private acquireOrExtendPrimaryLease(
txn: SimpleDbTransaction
txn: PersistenceTransaction
): PersistencePromise<void> {
const newPrimary = new DbPrimaryClient(
this.clientId,
Expand Down Expand Up @@ -887,7 +882,7 @@ export class IndexedDbPersistence implements Persistence {

/** Checks the primary lease and removes it if we are the current primary. */
private releasePrimaryLeaseIfHeld(
txn: SimpleDbTransaction
txn: PersistenceTransaction
): PersistencePromise<void> {
const store = primaryClientStore(txn);
return store.get(DbPrimaryClient.key).next(primaryClient => {
Expand Down Expand Up @@ -1052,18 +1047,22 @@ export class IndexedDbPersistence implements Persistence {
* Helper to get a typed SimpleDbStore for the primary client object store.
*/
function primaryClientStore(
txn: SimpleDbTransaction
txn: PersistenceTransaction
): SimpleDbStore<DbPrimaryClientKey, DbPrimaryClient> {
return txn.store<DbPrimaryClientKey, DbPrimaryClient>(DbPrimaryClient.store);
return IndexedDbPersistence.getStore<DbPrimaryClientKey, DbPrimaryClient>(
txn,
DbPrimaryClient.store
);
}

/**
* Helper to get a typed SimpleDbStore for the client metadata object store.
*/
function clientMetadataStore(
txn: SimpleDbTransaction
txn: PersistenceTransaction
): SimpleDbStore<DbClientMetadataKey, DbClientMetadata> {
return txn.store<DbClientMetadataKey, DbClientMetadata>(
return IndexedDbPersistence.getStore<DbClientMetadataKey, DbClientMetadata>(
txn,
DbClientMetadata.store
);
}
Expand Down
Loading