Skip to content

Transition to primary only if IndexedDB ops succeed #3049

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 1 commit into from
May 13, 2020
Merged

Transition to primary only if IndexedDB ops succeed #3049

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 16 additions & 9 deletions packages/firestore/src/core/sync_engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1032,9 +1032,6 @@ export class MultiTabSyncEngine extends SyncEngine

async applyPrimaryState(isPrimary: boolean): Promise<void> {
if (isPrimary === true && this.isPrimary !== true) {
this.isPrimary = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice that sync_engine.ts uses a mixture of reading from this.isPrimary and this.isPrimaryClient. Shouldn't all reads be consolidated to the latter? If so, we should fix this in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.isPrimaryClient is the getter in the API. this.isPrimary is the underlying member variable. A more common pattern is to use this.isPrimary and this._isPrimary. I will update this is in a follow-up.

await this.remoteStore.applyPrimaryState(true);

// Secondary tabs only maintain Views for their local listeners and the
// Views internal state may not be 100% populated (in particular
// secondary tabs don't track syncedDocuments, the set of documents the
Expand All @@ -1043,14 +1040,15 @@ export class MultiTabSyncEngine extends SyncEngine
// match the state on disk.
const activeTargets = this.sharedClientState.getAllActiveQueryTargets();
const activeQueries = await this.synchronizeQueryViewsAndRaiseSnapshots(
activeTargets.toArray()
activeTargets.toArray(),
/*transitionToPrimary=*/ true
);
this.isPrimary = true;
await this.remoteStore.applyPrimaryState(true);
Comment on lines +1046 to +1047
Copy link
Contributor

Choose a reason for hiding this comment

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

I want to verify something. These lines of code were moved down b/c the bit should only be set if a prior IndexDB operation succeeded correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly.

for (const targetData of activeQueries) {
this.remoteStore.listen(targetData);
}
} else if (isPrimary === false && this.isPrimary !== false) {
this.isPrimary = false;

const activeTargets: TargetId[] = [];

let p = Promise.resolve();
Expand All @@ -1070,8 +1068,12 @@ export class MultiTabSyncEngine extends SyncEngine
});
await p;

await this.synchronizeQueryViewsAndRaiseSnapshots(activeTargets);
await this.synchronizeQueryViewsAndRaiseSnapshots(
activeTargets,
/*transitionToPrimary=*/ false
);
this.resetLimboDocuments();
this.isPrimary = false;
await this.remoteStore.applyPrimaryState(false);
}
}
Expand All @@ -1091,9 +1093,14 @@ export class MultiTabSyncEngine extends SyncEngine
* Reconcile the query views of the provided query targets with the state from
* persistence. Raises snapshots for any changes that affect the local
* client and returns the updated state of all target's query data.
*
* @param targets the list of targets with views that need to be recomputed
* @param transitionToPrimary `true` iff the tab transitions from a secondary
* tab to a primary tab
*/
private async synchronizeQueryViewsAndRaiseSnapshots(
targets: TargetId[]
targets: TargetId[],
transitionToPrimary: boolean
): Promise<TargetData[]> {
const activeQueries: TargetData[] = [];
const newViewSnapshots: ViewSnapshot[] = [];
Expand Down Expand Up @@ -1127,7 +1134,7 @@ export class MultiTabSyncEngine extends SyncEngine
}
} else {
debugAssert(
this.isPrimary === true,
transitionToPrimary,
'A secondary tab should never have an active target without an active query.'
);
// For queries that never executed on this client, we need to
Expand Down