Skip to content

Commit a7c8497

Browse files
schmidt-sebastianschmidt-sebastian
committed
Review comments
1 parent e51265b commit a7c8497

File tree

5 files changed

+74
-70
lines changed

5 files changed

+74
-70
lines changed

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

Lines changed: 40 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -289,11 +289,11 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
289289

290290
if (!targetRemainsActive) {
291291
this.remoteStore.unlisten(queryView.targetId);
292-
await this.removeAndCleanupQuery(queryView);
293-
return this.localStore
292+
await this.localStore
294293
.releaseQuery(query, /*keepPersistedQueryData=*/ false)
294+
.then(() => this.removeAndCleanupQuery(queryView))
295295
.then(() => this.localStore.collectGarbage())
296-
.catch(err => this.tryRecoverClient(err));
296+
.catch(err => this.tryRecoverFromPrimaryLeaseLoss(err));
297297
}
298298
} else {
299299
await this.removeAndCleanupQuery(queryView);
@@ -399,7 +399,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
399399
.then(changes => {
400400
return this.emitNewSnapsAndNotifyLocalStore(changes, remoteEvent);
401401
})
402-
.catch(err => this.tryRecoverClient(err));
402+
.catch(err => this.tryRecoverFromPrimaryLeaseLoss(err));
403403
}
404404

405405
/**
@@ -469,11 +469,10 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
469469
} else {
470470
const queryView = this.queryViewsByTarget[targetId];
471471
assert(!!queryView, 'Unknown targetId: ' + targetId);
472-
await this.removeAndCleanupQuery(queryView);
473-
await this.localStore.releaseQuery(
474-
queryView.query,
475-
/* keepPersistedQueryData */ false
476-
);
472+
await this.localStore
473+
.releaseQuery(queryView.query, /* keepPersistedQueryData */ false)
474+
.then(() => this.removeAndCleanupQuery(queryView))
475+
.catch(err => this.tryRecoverFromPrimaryLeaseLoss(err));
477476
this.errorHandler!(queryView.query, err);
478477
}
479478
}
@@ -505,10 +504,16 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
505504
// connection is disabled.
506505
await this.remoteStore.fillWritePipeline();
507506
} else if (batchState === 'acknowledged' || batchState === 'rejected') {
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-
await this.applyPrimaryState(false);
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+
}
512517

513518
// NOTE: Both these methods are no-ops for batches that originated from
514519
// other clients.
@@ -542,7 +547,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
542547
this.sharedClientState.removeLocalPendingMutation(batchId);
543548
return this.emitNewSnapsAndNotifyLocalStore(changes);
544549
})
545-
.catch(err => this.tryRecoverClient(err));
550+
.catch(err => this.tryRecoverFromPrimaryLeaseLoss(err));
546551
}
547552

548553
rejectFailedWrite(batchId: BatchId, error: FirestoreError): Promise<void> {
@@ -561,7 +566,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
561566
this.sharedClientState.removeLocalPendingMutation(batchId);
562567
return this.emitNewSnapsAndNotifyLocalStore(changes);
563568
})
564-
.catch(err => this.tryRecoverClient(err));
569+
.catch(err => this.tryRecoverFromPrimaryLeaseLoss(err));
565570
}
566571

567572
private addMutationCallback(
@@ -738,22 +743,28 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
738743
if (this.isPrimary) {
739744
await this.localStore
740745
.collectGarbage()
741-
.catch(err => this.tryRecoverClient(err));
746+
.catch(err => this.tryRecoverFromPrimaryLeaseLoss(err));
742747
}
743748
}
744749

745750
/**
746751
* Marks the client as secondary if an IndexedDb operation fails because the
747752
* primary lease has been taken by another client. This can happen when the
748753
* client is temporarily CPU throttled and fails to renew its lease in time,
749-
* in which we treat the current client as secondary. We can always revert
750-
* back to primary status via the lease refresh in our persistence layer.
754+
* in which case we treat the current client as secondary. We can always
755+
* regain our primary lease via the lease refresh in our persistence layer.
751756
*
752-
* @param err An error returned by an IndexedDb operation.
757+
* @param err An error returned by a LocalStore operation.
753758
* @return A Promise that resolves after we recovered, or the original error.
754759
*/
755-
private async tryRecoverClient(err: FirestoreError): Promise<void> {
760+
private async tryRecoverFromPrimaryLeaseLoss(
761+
err: FirestoreError
762+
): Promise<void> {
756763
if (err.code === Code.FAILED_PRECONDITION) {
764+
log.debug(
765+
LOG_TAG,
766+
'Unexpectedly lost primary lease, reverting to secondary'
767+
);
757768
return this.applyPrimaryState(false);
758769
} else {
759770
throw err;
@@ -830,6 +841,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
830841
this.isPrimary = false;
831842
await this.remoteStore.disableNetwork();
832843
objUtils.forEachNumber(this.queryViewsByTarget, targetId => {
844+
// TODO(multitab): Remove query views for non-local queries.
833845
this.remoteStore.unlisten(targetId);
834846
});
835847
}
@@ -849,6 +861,10 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
849861
if (this.isPrimary) {
850862
// If we receive a target state notification via Web Storage, we are
851863
// 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+
);
852868
await this.applyPrimaryState(false);
853869
}
854870

@@ -912,11 +928,10 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
912928
// removed if it has been rejected by the backend.
913929
if (queryView) {
914930
this.remoteStore.unlisten(targetId);
915-
await this.removeAndCleanupQuery(queryView);
916-
await this.localStore.releaseQuery(
917-
queryView.query,
918-
/*keepPersistedQueryData=*/ false
919-
);
931+
await this.localStore
932+
.releaseQuery(queryView.query, /*keepPersistedQueryData=*/ false)
933+
.then(() => this.removeAndCleanupQuery(queryView))
934+
.catch(err => this.tryRecoverFromPrimaryLeaseLoss(err));
920935
}
921936
}
922937
}

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,9 +100,9 @@ export interface Persistence {
100100
shutdown(deleteData?: boolean): Promise<void>;
101101

102102
/**
103-
* Registers a listener that gets called when the primary state of the
104-
* instance changes. Upon registering, this listener is invoked immediately
105-
* with the current primary state.
103+
* Registers a listener that gets called immediately wih the current primary
104+
* state and then periodically as the lease is verified (likely with the same
105+
* state).
106106
*
107107
* PORTING NOTE: This is only used for Web multi-tab.
108108
*/

‎packages/firestore/src/remote/remote_store.ts

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -565,18 +565,21 @@ export class RemoteStore implements TargetMetadataProvider {
565565
this.writeStream.writeMutations(batch.mutations);
566566
}
567567
},
568-
err => {
569-
if (err.code === Code.FAILED_PRECONDITION) {
570-
// We have temporarily lost our primary lease. If we recover,
571-
// Sync Engine will re-enable the network.
572-
return this.disableNetwork();
573-
} else {
574-
return Promise.reject(err);
575-
}
576-
}
568+
err => this.tryRecoverFromPrimaryLeaseLoss(err)
577569
);
578570
}
579571

572+
private tryRecoverFromPrimaryLeaseLoss(err: FirestoreError): void {
573+
if (err.code === Code.FAILED_PRECONDITION) {
574+
// We have temporarily lost our primary lease. If we recover,
575+
// Sync Engine will re-enable the network.
576+
this.disableNetworkInternal();
577+
this.onlineStateTracker.set(OnlineState.Unknown);
578+
} else {
579+
throw err;
580+
}
581+
}
582+
580583
private onMutationResult(
581584
commitVersion: SnapshotVersion,
582585
results: MutationResult[]
@@ -646,15 +649,7 @@ export class RemoteStore implements TargetMetadataProvider {
646649

647650
return this.localStore
648651
.setLastStreamToken(emptyByteString())
649-
.catch(err => {
650-
if (err.code === Code.FAILED_PRECONDITION) {
651-
// We have temporarily lost our primary lease. If we recover,
652-
// Sync Engine will re-enable the network.
653-
return this.disableNetwork();
654-
} else {
655-
return Promise.reject(err);
656-
}
657-
});
652+
.catch(err => this.tryRecoverFromPrimaryLeaseLoss(err));
658653
} else {
659654
// Some other error, don't reset stream token. Our stream logic will
660655
// just retry with exponential backoff.

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

Lines changed: 17 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -820,27 +820,22 @@ describeSpec('Listens:', [], () => {
820820
const docA = doc('collection/a', 1000, { key: 'a' });
821821
const docB = doc('collection/b', 2000, { key: 'b' });
822822

823-
return (
824-
client(0)
825-
.expectPrimaryState(true)
826-
.userListens(query)
827-
.watchAcksFull(query, 1000, docA)
828-
.expectEvents(query, { added: [docA] })
829-
.client(1)
830-
.userListens(query)
831-
.expectEvents(query, { added: [docA] })
832-
.stealPrimaryLease()
833-
.expectListen(query, 'resume-token-1000')
834-
.watchAcksFull(query, 2000, docB)
835-
.expectEvents(query, { added: [docB] })
836-
.client(0)
837-
.expectPrimaryState(false)
838-
// TODO(multitab): Suppres the additional `fromCache`, which is raised
839-
// locally because we disable the network when we lose the primary
840-
// lease.
841-
.expectEvents(query, { fromCache: true })
842-
.expectEvents(query, { added: [docB] })
843-
);
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] });
844839
});
845840

846841
specTest(
@@ -862,7 +857,7 @@ describeSpec('Listens:', [], () => {
862857
.stealPrimaryLease()
863858
.client(0)
864859
// Send a watch update to client 0, who is longer primary (but doesn't
865-
// it yet). The watch update gets ignored.
860+
// know it yet). The watch update gets ignored.
866861
.watchAcksFull(query, 1000, docA)
867862
.expectPrimaryState(false)
868863
.client(1)

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
///<reference path="../../util/test_platform.ts"/>
21
/**
32
* Copyright 2017 Google Inc.
43
*
@@ -1012,8 +1011,8 @@ abstract class TestRunner {
10121011
if (this.expectedActiveTargets) {
10131012
if (!obj.isEmpty(this.expectedActiveTargets)) {
10141013
await this.connection.waitForWatchOpen();
1014+
await this.queue.drain();
10151015
}
1016-
await this.queue.drain();
10171016
}
10181017

10191018
const actualTargets = obj.shallowCopy(this.connection.activeTargets);

0 commit comments

Comments
 (0)