Skip to content

Commit b185a0f

Browse files
mikelehenwilhuff
mikelehen
authored and
wilhuff
committed
Fix b/72502745: OnlineState changes cause limbo document crash. (firebase#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.
1 parent 189e327 commit b185a0f

File tree

2 files changed

+269
-11
lines changed

2 files changed

+269
-11
lines changed

‎Firestore/Example/Tests/SpecTests/json/offline_spec_test.json

Lines changed: 254 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -459,5 +459,259 @@
459459
]
460460
}
461461
]
462+
},
463+
"Queries with limbo documents handle going offline.": {
464+
"describeName": "Offline:",
465+
"itName": "Queries with limbo documents handle going offline.",
466+
"tags": [],
467+
"config": {
468+
"useGarbageCollection": true
469+
},
470+
"steps": [
471+
{
472+
"userListen": [
473+
2,
474+
{
475+
"path": "collection",
476+
"filters": [],
477+
"orderBys": []
478+
}
479+
],
480+
"stateExpect": {
481+
"activeTargets": {
482+
"2": {
483+
"query": {
484+
"path": "collection",
485+
"filters": [],
486+
"orderBys": []
487+
},
488+
"resumeToken": ""
489+
}
490+
}
491+
}
492+
},
493+
{
494+
"watchAck": [
495+
2
496+
]
497+
},
498+
{
499+
"watchEntity": {
500+
"docs": [
501+
[
502+
"collection/a",
503+
1000,
504+
{
505+
"key": "a"
506+
}
507+
]
508+
],
509+
"targets": [
510+
2
511+
]
512+
}
513+
},
514+
{
515+
"watchCurrent": [
516+
[
517+
2
518+
],
519+
"resume-token-1000"
520+
],
521+
"watchSnapshot": 1000,
522+
"expect": [
523+
{
524+
"query": {
525+
"path": "collection",
526+
"filters": [],
527+
"orderBys": []
528+
},
529+
"added": [
530+
[
531+
"collection/a",
532+
1000,
533+
{
534+
"key": "a"
535+
}
536+
]
537+
],
538+
"errorCode": 0,
539+
"fromCache": false,
540+
"hasPendingWrites": false
541+
}
542+
]
543+
},
544+
{
545+
"watchReset": [
546+
2
547+
]
548+
},
549+
{
550+
"watchCurrent": [
551+
[
552+
2
553+
],
554+
"resume-token-1001"
555+
],
556+
"watchSnapshot": 1001,
557+
"stateExpect": {
558+
"limboDocs": [
559+
"collection/a"
560+
],
561+
"activeTargets": {
562+
"1": {
563+
"query": {
564+
"path": "collection/a",
565+
"filters": [],
566+
"orderBys": []
567+
},
568+
"resumeToken": ""
569+
},
570+
"2": {
571+
"query": {
572+
"path": "collection",
573+
"filters": [],
574+
"orderBys": []
575+
},
576+
"resumeToken": ""
577+
}
578+
}
579+
},
580+
"expect": [
581+
{
582+
"query": {
583+
"path": "collection",
584+
"filters": [],
585+
"orderBys": []
586+
},
587+
"errorCode": 0,
588+
"fromCache": true,
589+
"hasPendingWrites": false
590+
}
591+
]
592+
},
593+
{
594+
"watchStreamClose": {
595+
"error": {
596+
"code": 14,
597+
"message": "Simulated Backend Error"
598+
}
599+
},
600+
"stateExpect": {
601+
"activeTargets": {
602+
"1": {
603+
"query": {
604+
"path": "collection/a",
605+
"filters": [],
606+
"orderBys": []
607+
},
608+
"resumeToken": ""
609+
},
610+
"2": {
611+
"query": {
612+
"path": "collection",
613+
"filters": [],
614+
"orderBys": []
615+
},
616+
"resumeToken": "resume-token-1001"
617+
}
618+
}
619+
}
620+
},
621+
{
622+
"watchStreamClose": {
623+
"error": {
624+
"code": 14,
625+
"message": "Simulated Backend Error"
626+
}
627+
}
628+
},
629+
{
630+
"watchStreamClose": {
631+
"error": {
632+
"code": 14,
633+
"message": "Simulated Backend Error"
634+
}
635+
}
636+
},
637+
{
638+
"watchAck": [
639+
2
640+
]
641+
},
642+
{
643+
"watchEntity": {
644+
"docs": [],
645+
"targets": [
646+
2
647+
]
648+
}
649+
},
650+
{
651+
"watchCurrent": [
652+
[
653+
2
654+
],
655+
"resume-token-1001"
656+
],
657+
"watchSnapshot": 1001
658+
},
659+
{
660+
"watchAck": [
661+
1
662+
]
663+
},
664+
{
665+
"watchEntity": {
666+
"docs": [],
667+
"targets": [
668+
1
669+
]
670+
}
671+
},
672+
{
673+
"watchCurrent": [
674+
[
675+
1
676+
],
677+
"resume-token-1001"
678+
],
679+
"watchSnapshot": 1001,
680+
"expect": [
681+
{
682+
"query": {
683+
"path": "collection",
684+
"filters": [],
685+
"orderBys": []
686+
},
687+
"removed": [
688+
[
689+
"collection/a",
690+
1000,
691+
{
692+
"key": "a"
693+
}
694+
]
695+
],
696+
"errorCode": 0,
697+
"fromCache": false,
698+
"hasPendingWrites": false
699+
}
700+
],
701+
"stateExpect": {
702+
"limboDocs": [],
703+
"activeTargets": {
704+
"2": {
705+
"query": {
706+
"path": "collection",
707+
"filters": [],
708+
"orderBys": []
709+
},
710+
"resumeToken": "resume-token-1001"
711+
}
712+
}
713+
}
714+
}
715+
]
462716
}
463717
}

‎Firestore/Source/Core/FSTView.m

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -312,8 +312,8 @@ - (FSTViewChange *)applyChangesToDocuments:(FSTViewDocumentChanges *)docChanges
312312
}
313313
return self.query.comparator(c1.document, c2.document);
314314
}];
315-
316-
NSArray<FSTLimboDocumentChange *> *limboChanges = [self applyTargetChange:targetChange];
315+
[self applyTargetChange:targetChange];
316+
NSArray<FSTLimboDocumentChange *> *limboChanges = [self updateLimboDocuments];
317317
BOOL synced = self.limboDocuments.count == 0 && self.isCurrent;
318318
FSTSyncState newSyncState = synced ? FSTSyncStateSynced : FSTSyncStateLocal;
319319
BOOL syncStateChanged = newSyncState != self.syncState;
@@ -378,10 +378,9 @@ - (BOOL)shouldBeLimboDocumentKey:(FSTDocumentKey *)key {
378378
}
379379

380380
/**
381-
* Updates syncedDocuments, isAcked, and limbo docs based on the given change.
382-
* @return the list of changes to which docs are in limbo.
381+
* Updates syncedDocuments and current based on the given change.
383382
*/
384-
- (NSArray<FSTLimboDocumentChange *> *)applyTargetChange:(nullable FSTTargetChange *)targetChange {
383+
- (void)applyTargetChange:(nullable FSTTargetChange *)targetChange {
385384
if (targetChange) {
386385
FSTTargetMapping *targetMapping = targetChange.mapping;
387386
if ([targetMapping isKindOfClass:[FSTResetMapping class]]) {
@@ -408,16 +407,21 @@ - (BOOL)shouldBeLimboDocumentKey:(FSTDocumentKey *)key {
408407
break;
409408
}
410409
}
410+
}
411+
412+
/** Updates limboDocuments and returns any changes as FSTLimboDocumentChanges. */
413+
- (NSArray<FSTLimboDocumentChange *> *)updateLimboDocuments {
414+
// We can only determine limbo documents when we're in-sync with the server.
415+
if (!self.isCurrent) {
416+
return @[];
417+
}
411418

412-
// Recompute the set of limbo docs.
413419
// TODO(klimt): Do this incrementally so that it's not quadratic when updating many documents.
414420
FSTDocumentKeySet *oldLimboDocuments = self.limboDocuments;
415421
self.limboDocuments = [FSTDocumentKeySet keySet];
416-
if (self.isCurrent) {
417-
for (FSTDocument *doc in self.documentSet.documentEnumerator) {
418-
if ([self shouldBeLimboDocumentKey:doc.key]) {
419-
self.limboDocuments = [self.limboDocuments setByAddingObject:doc.key];
420-
}
422+
for (FSTDocument *doc in self.documentSet.documentEnumerator) {
423+
if ([self shouldBeLimboDocumentKey:doc.key]) {
424+
self.limboDocuments = [self.limboDocuments setByAddingObject:doc.key];
421425
}
422426
}
423427

0 commit comments

Comments
 (0)