Skip to content

Commit

Permalink
Fix b/72502745: OnlineState changes cause limbo document crash. (fire…
Browse files Browse the repository at this point in the history 
…base#470) (firebase#714)

[PORT OF firebase/firebase-js-sdk#470]

Context: I made a previous change to raise isFromCache=true events when the
client goes offline. As part of this change I added an assert for
"OnlineState should not affect limbo documents", which it turns out was not
valid because:
* When we go offline, we set the view to non-current and call
	View.applyChanges().
* View.applyChanges() calls applyTargetChange() even though there's no target
	change and it recalculates limbo documents.
* When the view is not current, we consider no documents to be in limbo.
* Therefore all limbo documents are removed and so applyChanges() ends up
	returning unexpected LimboDocumentChanges.

Fix: I've modified the View logic so that we don't recalculate limbo documents
(and generate LimboDocumentChanges) when the View is not current, so now my
assert holds and there should be less spurious removal / re-adding of limbo
documents.
  • Loading branch information
@mikelehen @wilhuff
mikelehen authored and wilhuff committed Jan 27, 2018
1 parent af6976a commit 4d7c973
Show file tree
Hide file tree
Showing 2 changed files with 269 additions and 11 deletions.
254 changes: 254 additions & 0 deletions Firestore/Example/Tests/SpecTests/json/offline_spec_test.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -459,5 +459,259 @@
]
}
]
},
"Queries with limbo documents handle going offline.": {
"describeName": "Offline:",
"itName": "Queries with limbo documents handle going offline.",
"tags": [],
"config": {
"useGarbageCollection": true
},
"steps": [
{
"userListen": [
2,
{
"path": "collection",
"filters": [],
"orderBys": []
}
],
"stateExpect": {
"activeTargets": {
"2": {
"query": {
"path": "collection",
"filters": [],
"orderBys": []
},
"resumeToken": ""
}
}
}
},
{
"watchAck": [
2
]
},
{
"watchEntity": {
"docs": [
[
"collection/a",
1000,
{
"key": "a"
}
]
],
"targets": [
2
]
}
},
{
"watchCurrent": [
[
2
],
"resume-token-1000"
],
"watchSnapshot": 1000,
"expect": [
{
"query": {
"path": "collection",
"filters": [],
"orderBys": []
},
"added": [
[
"collection/a",
1000,
{
"key": "a"
}
]
],
"errorCode": 0,
"fromCache": false,
"hasPendingWrites": false
}
]
},
{
"watchReset": [
2
]
},
{
"watchCurrent": [
[
2
],
"resume-token-1001"
],
"watchSnapshot": 1001,
"stateExpect": {
"limboDocs": [
"collection/a"
],
"activeTargets": {
"1": {
"query": {
"path": "collection/a",
"filters": [],
"orderBys": []
},
"resumeToken": ""
},
"2": {
"query": {
"path": "collection",
"filters": [],
"orderBys": []
},
"resumeToken": ""
}
}
},
"expect": [
{
"query": {
"path": "collection",
"filters": [],
"orderBys": []
},
"errorCode": 0,
"fromCache": true,
"hasPendingWrites": false
}
]
},
{
"watchStreamClose": {
"error": {
"code": 14,
"message": "Simulated Backend Error"
}
},
"stateExpect": {
"activeTargets": {
"1": {
"query": {
"path": "collection/a",
"filters": [],
"orderBys": []
},
"resumeToken": ""
},
"2": {
"query": {
"path": "collection",
"filters": [],
"orderBys": []
},
"resumeToken": "resume-token-1001"
}
}
}
},
{
"watchStreamClose": {
"error": {
"code": 14,
"message": "Simulated Backend Error"
}
}
},
{
"watchStreamClose": {
"error": {
"code": 14,
"message": "Simulated Backend Error"
}
}
},
{
"watchAck": [
2
]
},
{
"watchEntity": {
"docs": [],
"targets": [
2
]
}
},
{
"watchCurrent": [
[
2
],
"resume-token-1001"
],
"watchSnapshot": 1001
},
{
"watchAck": [
1
]
},
{
"watchEntity": {
"docs": [],
"targets": [
1
]
}
},
{
"watchCurrent": [
[
1
],
"resume-token-1001"
],
"watchSnapshot": 1001,
"expect": [
{
"query": {
"path": "collection",
"filters": [],
"orderBys": []
},
"removed": [
[
"collection/a",
1000,
{
"key": "a"
}
]
],
"errorCode": 0,
"fromCache": false,
"hasPendingWrites": false
}
],
"stateExpect": {
"limboDocs": [],
"activeTargets": {
"2": {
"query": {
"path": "collection",
"filters": [],
"orderBys": []
},
"resumeToken": "resume-token-1001"
}
}
}
}
]
}
}
26 changes: 15 additions & 11 deletions Firestore/Source/Core/FSTView.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,8 +312,8 @@ - (FSTViewChange *)applyChangesToDocuments:(FSTViewDocumentChanges *)docChanges
}
return self.query.comparator(c1.document, c2.document);
}];

NSArray<FSTLimboDocumentChange *> *limboChanges = [self applyTargetChange:targetChange];
[self applyTargetChange:targetChange];
NSArray<FSTLimboDocumentChange *> *limboChanges = [self updateLimboDocuments];
BOOL synced = self.limboDocuments.count == 0 && self.isCurrent;
FSTSyncState newSyncState = synced ? FSTSyncStateSynced : FSTSyncStateLocal;
BOOL syncStateChanged = newSyncState != self.syncState;
Expand Down Expand Up @@ -378,10 +378,9 @@ - (BOOL)shouldBeLimboDocumentKey:(FSTDocumentKey *)key {
}

/**
* Updates syncedDocuments, isAcked, and limbo docs based on the given change.
* @return the list of changes to which docs are in limbo.
* Updates syncedDocuments and current based on the given change.
*/
- (NSArray<FSTLimboDocumentChange *> *)applyTargetChange:(nullable FSTTargetChange *)targetChange {
- (void)applyTargetChange:(nullable FSTTargetChange *)targetChange {
if (targetChange) {
FSTTargetMapping *targetMapping = targetChange.mapping;
if ([targetMapping isKindOfClass:[FSTResetMapping class]]) {
Expand All @@ -408,16 +407,21 @@ - (BOOL)shouldBeLimboDocumentKey:(FSTDocumentKey *)key {
break;
}
}
}

/** Updates limboDocuments and returns any changes as FSTLimboDocumentChanges. */
- (NSArray<FSTLimboDocumentChange *> *)updateLimboDocuments {
// We can only determine limbo documents when we're in-sync with the server.
if (!self.isCurrent) {
return @[];
}

// Recompute the set of limbo docs.
// TODO(klimt): Do this incrementally so that it's not quadratic when updating many documents.
FSTDocumentKeySet *oldLimboDocuments = self.limboDocuments;
self.limboDocuments = [FSTDocumentKeySet keySet];
if (self.isCurrent) {
for (FSTDocument *doc in self.documentSet.documentEnumerator) {
if ([self shouldBeLimboDocumentKey:doc.key]) {
self.limboDocuments = [self.limboDocuments setByAddingObject:doc.key];
}
for (FSTDocument *doc in self.documentSet.documentEnumerator) {
if ([self shouldBeLimboDocumentKey:doc.key]) {
self.limboDocuments = [self.limboDocuments setByAddingObject:doc.key];
}
}

Expand Down

0 comments on commit 4d7c973

Please sign in to comment.