Skip to content

Commit 34d52ae

Browse files
schmidt-sebastianschmidt-sebastian
committed
Some clever review comment before I merge firestore-multi-tab.
1 parent f9c7872 commit 34d52ae

File tree

6 files changed

+51
-51
lines changed

6 files changed

+51
-51
lines changed

‎packages/firestore/src/core/sync_engine.ts

Lines changed: 29 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ import {
5656
AddedLimboDocument,
5757
LimboDocumentChange,
5858
RemovedLimboDocument,
59-
View,
59+
View, ViewChange,
6060
ViewDocumentChanges
6161
} from './view';
6262
import { ViewSnapshot } from './view_snapshot';
@@ -262,14 +262,12 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
262262
* persistence.
263263
*/
264264
// PORTING NOTE: Multi-tab only.
265-
private async synchronizeLocalView(targetId: TargetId): Promise<void> {
266-
return this.localStore
267-
.remoteDocumentKeys(targetId)
268-
.then(async remoteKeys => {
269-
const queryView = this.queryViewsByTarget[targetId];
270-
assert(!!queryView, 'Expected queryView to be defined');
271-
queryView.view.synchronizeWithRemoteKeys(remoteKeys);
272-
});
265+
private async synchronizeLocalView(query:Query, targetId: TargetId): Promise<ViewChange> {
266+
return this.localStore.executeQuery(query).then(docs => {
267+
const queryView = this.queryViewsByTarget[targetId];
268+
assert(!!queryView, 'Expected queryView to be defined');
269+
return queryView.view.synchronizeWithRemoteDocuments(docs);
270+
});
273271
}
274272

275273
/** Stops listening to the query. */
@@ -504,17 +502,6 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
504502
// connection is disabled.
505503
await this.remoteStore.fillWritePipeline();
506504
} else if (batchState === 'acknowledged' || batchState === 'rejected') {
507-
// if (this.isPrimary) {
508-
// // If we receive a notification of an `acknowledged` or `rejected` batch
509-
// // via Web Storage, we are either already secondary or another tab has
510-
// // taken the primary lease.
511-
// log.debug(
512-
// LOG_TAG,
513-
// 'Unexpectedly received mutation batch notification when already primary. Releasing primary lease.'
514-
// );
515-
// await this.applyPrimaryState(false);
516-
// }
517-
518505
// NOTE: Both these methods are no-ops for batches that originated from
519506
// other clients.
520507
this.sharedClientState.removeLocalPendingMutation(batchId);
@@ -827,7 +814,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
827814
// state (the list of syncedDocuments may have gotten out of sync).
828815
await this.localStore.releaseQuery(query, true);
829816
queryData = await this.localStore.allocateQuery(query);
830-
await this.synchronizeLocalView(targetId);
817+
await this.synchronizeLocalView(query, targetId);
831818
}
832819
this.remoteStore.listen(queryData);
833820
});
@@ -840,16 +827,21 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
840827
} else if (isPrimary === false && this.isPrimary !== false) {
841828
this.isPrimary = false;
842829
await this.remoteStore.disableNetwork();
843-
objUtils.forEachNumber(this.queryViewsByTarget, targetId => {
844-
// TODO(multitab): Remove query views for non-local queries.
845830

846-
const queryView = this.queryViewsByTarget[targetId];
847-
const viewChange = queryView.view.clearLimboDocuments();
848-
if (viewChange.snapshot) {
849-
this.viewHandler!([viewChange.snapshot]);
850-
}
831+
let p = Promise.resolve();
832+
objUtils.forEachNumber(this.queryViewsByTarget, targetId => {
833+
p = p.then(async () => {
834+
let queryView = this.queryViewsByTarget[targetId];
835+
// TODO(multitab): Remove query views for non-local queries.
836+
const viewChange = await this.synchronizeLocalView(queryView.query, targetId);
837+
// const viewChange = queryView.view.clearLimboDocuments();
838+
if (viewChange.snapshot) {
839+
this.viewHandler!([viewChange.snapshot]);
840+
}
841+
});
851842
this.remoteStore.unlisten(targetId);
852843
});
844+
await p;
853845
}
854846
}
855847

@@ -864,15 +856,15 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
864856
state: QueryTargetState,
865857
error?: FirestoreError
866858
): Promise<void> {
867-
// if (this.isPrimary) {
868-
// // If we receive a target state notification via Web Storage, we are
869-
// // either already secondary or another tab has taken the primary lease.
870-
// log.debug(
871-
// LOG_TAG,
872-
// 'Unexpectedly received query state notification when already primary. Releasing primary lease.'
873-
// );
874-
// await this.applyPrimaryState(false);
875-
// }
859+
if (this.isPrimary) {
860+
// If we receive a target state notification via Web Storage, we are
861+
// either already secondary or another tab has taken the primary lease.
862+
log.debug(
863+
LOG_TAG,
864+
'Unexpectedly received query state notification when already primary. Ignoring.'
865+
);
866+
return;
867+
}
876868

877869
if (this.queryViewsByTarget[targetId]) {
878870
switch (state) {

‎packages/firestore/src/core/view.ts

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -384,8 +384,19 @@ export class View {
384384
}
385385

386386
// PORTING NOTE: Multi-tab only.
387-
synchronizeWithRemoteKeys(remoteKeys: DocumentKeySet): void {
388-
this._syncedDocuments = remoteKeys;
387+
synchronizeWithRemoteDocuments(remoteDocs: MaybeDocumentMap): ViewChange {
388+
389+
this.limboDocuments = documentKeySet();
390+
const docChanges = this.computeDocChanges(remoteDocs);
391+
const viewChange = this.applyChanges(docChanges, false);
392+
393+
let keys = documentKeySet();
394+
395+
remoteDocs.forEach(key => {
396+
keys = keys.add(key);
397+
});
398+
this._syncedDocuments = keys;
399+
return viewChange;
389400
}
390401

391402
/**

‎packages/firestore/src/util/sorted_map.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
import { assert, fail } from './assert';
1818
import { AnyJs } from './misc';
19+
import {SortedSet} from './sorted_set';
1920

2021
/*
2122
* Implementation of an immutable SortedMap using a Left-leaning

‎packages/firestore/test/unit/specs/limbo_spec.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,8 @@ describeSpec('Limbo Documents:', [], () => {
330330
}
331331
);
332332

333+
//YES WRITE THIS
334+
333335
// TODO(multitab): We need a test case that verifies that a primary client
334336
// that loses its primary lease while documents are in limbo correctly handles
335337
// these documents even when it picks up its lease again.

‎packages/firestore/test/unit/specs/listen_spec.test.ts

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -815,14 +815,12 @@ describeSpec('Listens:', [], () => {
815815
.expectEvents(query, { added: [docB] });
816816
});
817817

818-
specTest('Query recovers after primary takeover', ['multi-client'], () => {
818+
specTest('Query recovers after primary takeover', ['exclusive','multi-client'], () => {
819819
const query = Query.atPath(path('collection'));
820820
const docA = doc('collection/a', 1000, { key: 'a' });
821821
const docB = doc('collection/b', 2000, { key: 'b' });
822822
const docC = doc('collection/c', 3000, { key: 'c' });
823823

824-
// TODO(multitab): Once #994 is merged, the query in this test should flip
825-
// back to `fromCache:false` even without docC
826824
return (
827825
client(0)
828826
.expectPrimaryState(true)
@@ -836,19 +834,17 @@ describeSpec('Listens:', [], () => {
836834
.expectListen(query, 'resume-token-1000')
837835
.watchAcksFull(query, 2000, docB)
838836
.expectEvents(query, { added: [docB] })
839-
// Client 0 sends the query into limbo since it does not know about docB,
840-
// but it still thinks it is primary.
841-
.expectEvents(query, { fromCache: true })
842837
.client(0)
843-
.expectEvents(query, { added: [docB], fromCache: true })
844-
.expectLimboDocs(docB.key)
838+
// Client 0 ignores all events until it transitions to secondary
845839
.client(1)
846840
.watchSends({ affects: [query] }, docC)
847841
.watchSnapshots(3000)
848842
.expectEvents(query, { added: [docC] })
849843
.client(0)
844+
.runTimer(TimerId.ClientMetadataRefresh)
850845
.expectPrimaryState(false)
851-
.expectEvents(query, { added: [docC] })
846+
.expectEvents(query, { fromCache: true})
847+
.expectEvents(query, { added: [docB, docC], fromCache:true })
852848
);
853849
});
854850

‎packages/firestore/test/unit/specs/write_spec.test.ts

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1130,7 +1130,7 @@ describeSpec('Writes:', [], () => {
11301130

11311131
specTest(
11321132
'Mutation recovers after primary takeover',
1133-
['exclusive', 'multi-client'],
1133+
['multi-client'],
11341134
() => {
11351135
const query = Query.atPath(path('collection'));
11361136
const docALocal = doc(
@@ -1156,8 +1156,6 @@ describeSpec('Writes:', [], () => {
11561156
.writeAcks('collection/a', 1000, { expectUserCallback: false })
11571157
.watchAcksFull(query, 1000, docA)
11581158
.expectEvents(query, { metadata: [docA] })
1159-
// Client 0 sends the query into limbo since it doesn't know about docA
1160-
.expectEvents(query, { fromCache: true })
11611159
.client(0)
11621160
.expectUserCallbacks({
11631161
acknowledged: ['collection/a']

0 commit comments

Comments
 (0)