Skip to content

Commit f9c7872

Browse files
schmidt-sebastianschmidt-sebastian
committed
RFC/Not dropping primary status right away
1 parent a7c8497 commit f9c7872

File tree

5 files changed

+120
-64
lines changed

5 files changed

+120
-64
lines changed

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

Lines changed: 25 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -504,16 +504,16 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
504504
// connection is disabled.
505505
await this.remoteStore.fillWritePipeline();
506506
} 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-
}
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+
// }
517517

518518
// NOTE: Both these methods are no-ops for batches that originated from
519519
// other clients.
@@ -842,6 +842,12 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
842842
await this.remoteStore.disableNetwork();
843843
objUtils.forEachNumber(this.queryViewsByTarget, targetId => {
844844
// TODO(multitab): Remove query views for non-local queries.
845+
846+
const queryView = this.queryViewsByTarget[targetId];
847+
const viewChange = queryView.view.clearLimboDocuments();
848+
if (viewChange.snapshot) {
849+
this.viewHandler!([viewChange.snapshot]);
850+
}
845851
this.remoteStore.unlisten(targetId);
846852
});
847853
}
@@ -858,15 +864,15 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
858864
state: QueryTargetState,
859865
error?: FirestoreError
860866
): Promise<void> {
861-
if (this.isPrimary) {
862-
// If we receive a target state notification via Web Storage, we are
863-
// either already secondary or another tab has taken the primary lease.
864-
log.debug(
865-
LOG_TAG,
866-
'Unexpectedly received query state notification when already primary. Releasing primary lease.'
867-
);
868-
await this.applyPrimaryState(false);
869-
}
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+
// }
870876

871877
if (this.queryViewsByTarget[targetId]) {
872878
switch (state) {

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -402,6 +402,31 @@ export class View {
402402
!this.mutatedKeys.isEmpty()
403403
);
404404
}
405+
406+
clearLimboDocuments(): ViewChange {
407+
this.limboDocuments = documentKeySet();
408+
const synced = this.current;
409+
const newSyncState = synced ? SyncState.Synced : SyncState.Local;
410+
const syncStateChanged = newSyncState !== this.syncState;
411+
412+
if (syncStateChanged) {
413+
this.syncState = newSyncState;
414+
415+
const snap: ViewSnapshot = new ViewSnapshot(
416+
this.query,
417+
this.documentSet,
418+
this.documentSet,
419+
[],
420+
this.syncState === SyncState.Local,
421+
!this.mutatedKeys.isEmpty(),
422+
syncStateChanged,
423+
/* excludesMetadataChanges= */ false
424+
);
425+
return { snapshot: snap, limboChanges: [] };
426+
} else {
427+
return { limboChanges: [] };
428+
}
429+
}
405430
}
406431

407432
function compareChangeType(c1: ChangeType, c2: ChangeType): number {

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

Lines changed: 30 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -819,23 +819,37 @@ describeSpec('Listens:', [], () => {
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' });
822+
const docC = doc('collection/c', 3000, { key: 'c' });
822823

823-
return client(0)
824-
.expectPrimaryState(true)
825-
.userListens(query)
826-
.watchAcksFull(query, 1000, docA)
827-
.expectEvents(query, { added: [docA] })
828-
.client(1)
829-
.userListens(query)
830-
.expectEvents(query, { added: [docA] })
831-
.stealPrimaryLease()
832-
.expectListen(query, 'resume-token-1000')
833-
.watchAcksFull(query, 2000, docB)
834-
.expectEvents(query, { added: [docB] })
835-
.client(0)
836-
.expectPrimaryState(false)
837-
.expectEvents(query, { fromCache: true })
838-
.expectEvents(query, { added: [docB] });
824+
// TODO(multitab): Once #994 is merged, the query in this test should flip
825+
// back to `fromCache:false` even without docC
826+
return (
827+
client(0)
828+
.expectPrimaryState(true)
829+
.userListens(query)
830+
.watchAcksFull(query, 1000, docA)
831+
.expectEvents(query, { added: [docA] })
832+
.client(1)
833+
.userListens(query)
834+
.expectEvents(query, { added: [docA] })
835+
.stealPrimaryLease()
836+
.expectListen(query, 'resume-token-1000')
837+
.watchAcksFull(query, 2000, docB)
838+
.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 })
842+
.client(0)
843+
.expectEvents(query, { added: [docB], fromCache: true })
844+
.expectLimboDocs(docB.key)
845+
.client(1)
846+
.watchSends({ affects: [query] }, docC)
847+
.watchSnapshots(3000)
848+
.expectEvents(query, { added: [docC] })
849+
.client(0)
850+
.expectPrimaryState(false)
851+
.expectEvents(query, { added: [docC] })
852+
);
839853
});
840854

841855
specTest(

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -950,7 +950,10 @@ abstract class TestRunner {
950950
this.expectedActiveTargets = expectation.activeTargets!;
951951
}
952952
if ('isPrimary' in expectation) {
953-
expect(this.syncEngine.isPrimaryClient).to.eq(expectation.isPrimary!);
953+
expect(this.syncEngine.isPrimaryClient).to.eq(
954+
expectation.isPrimary!,
955+
'isPrimary'
956+
);
954957
}
955958
if ('userCallbacks' in expectation) {
956959
expect(this.acknowledgedDocs).to.have.members(

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

Lines changed: 36 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1128,35 +1128,43 @@ describeSpec('Writes:', [], () => {
11281128
});
11291129
});
11301130

1131-
specTest('Mutation recovers after primary takeover', ['multi-client'], () => {
1132-
const query = Query.atPath(path('collection'));
1133-
const docALocal = doc(
1134-
'collection/a',
1135-
0,
1136-
{ k: 'a' },
1137-
{ hasLocalMutations: true }
1138-
);
1139-
const docA = doc('collection/a', 1000, { k: 'a' });
1131+
specTest(
1132+
'Mutation recovers after primary takeover',
1133+
['exclusive', 'multi-client'],
1134+
() => {
1135+
const query = Query.atPath(path('collection'));
1136+
const docALocal = doc(
1137+
'collection/a',
1138+
0,
1139+
{ k: 'a' },
1140+
{ hasLocalMutations: true }
1141+
);
1142+
const docA = doc('collection/a', 1000, { k: 'a' });
11401143

1141-
return client(0)
1142-
.expectPrimaryState(true)
1143-
.userSets('collection/a', { k: 'a' })
1144-
.client(1)
1145-
.userListens(query)
1146-
.expectEvents(query, {
1147-
added: [docALocal],
1148-
hasPendingWrites: true,
1149-
fromCache: true
1150-
})
1151-
.stealPrimaryLease()
1152-
.writeAcks('collection/a', 1000, { expectUserCallback: false })
1153-
.watchAcksFull(query, 1000, docA)
1154-
.expectEvents(query, { metadata: [docA] })
1155-
.client(0)
1156-
.expectUserCallbacks({
1157-
acknowledged: ['collection/a']
1158-
});
1159-
});
1144+
return (
1145+
client(0)
1146+
.expectPrimaryState(true)
1147+
.userSets('collection/a', { k: 'a' })
1148+
.client(1)
1149+
.userListens(query)
1150+
.expectEvents(query, {
1151+
added: [docALocal],
1152+
hasPendingWrites: true,
1153+
fromCache: true
1154+
})
1155+
.stealPrimaryLease()
1156+
.writeAcks('collection/a', 1000, { expectUserCallback: false })
1157+
.watchAcksFull(query, 1000, docA)
1158+
.expectEvents(query, { metadata: [docA] })
1159+
// Client 0 sends the query into limbo since it doesn't know about docA
1160+
.expectEvents(query, { fromCache: true })
1161+
.client(0)
1162+
.expectUserCallbacks({
1163+
acknowledged: ['collection/a']
1164+
})
1165+
);
1166+
}
1167+
);
11601168

11611169
specTest('Write is sent by newly started primary', ['multi-client'], () => {
11621170
return client(0)

0 commit comments

Comments
 (0)