Skip to content

Commit 05a8130

Browse files
mikelehenmikelehen
authored
Fix MutationQueue issue resulting in re-sending acknowledged writes. (firebase#909)
Port of: firebase/firebase-js-sdk#559 Should address firebase#772 once released. getNextMutationBatchAfterBatchId() was not respecting highestAcknowledgedBatchId and therefore we were resending writes after the network was disabled / re-enabled.
1 parent 42ce551 commit 05a8130

File tree

5 files changed

+220
-4
lines changed

5 files changed

+220
-4
lines changed

‎Firestore/CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,9 @@
33
neither succeeds nor fails within 10 seconds, the SDK will consider itself
44
"offline", causing getDocument() calls to resolve with cached results, rather
55
than continuing to wait.
6+
- [fixed] Fixed a potential race condition after calling `enableNetwork()` that
7+
could result in a "Mutation batchIDs must be acknowledged in order" assertion
8+
crash.
69

710
# v0.10.3
811
- [fixed] Fixed a regression in the 4.10.0 Firebase iOS SDK release that

‎Firestore/Example/Tests/Local/FSTMutationQueueTests.mm

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,22 @@ - (void)testNextMutationBatchAfterBatchID {
217217
XCTAssertNil(notFound);
218218
}
219219

220+
- (void)testNextMutationBatchAfterBatchIDSkipsAcknowledgedBatches {
221+
if ([self isTestBaseClass]) return;
222+
223+
NSMutableArray<FSTMutationBatch *> *batches = [self createBatches:3];
224+
XCTAssertEqualObjects([self.mutationQueue nextMutationBatchAfterBatchID:kFSTBatchIDUnknown],
225+
batches[0]);
226+
227+
[self acknowledgeBatch:batches[0]];
228+
XCTAssertEqualObjects([self.mutationQueue nextMutationBatchAfterBatchID:kFSTBatchIDUnknown],
229+
batches[1]);
230+
XCTAssertEqualObjects([self.mutationQueue nextMutationBatchAfterBatchID:batches[0].batchID],
231+
batches[1]);
232+
XCTAssertEqualObjects([self.mutationQueue nextMutationBatchAfterBatchID:batches[1].batchID],
233+
batches[2]);
234+
}
235+
220236
- (void)testAllMutationBatchesThroughBatchID {
221237
if ([self isTestBaseClass]) return;
222238

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

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3407,6 +3407,198 @@
34073407
}
34083408
]
34093409
},
3410+
"Held writes are not re-sent after disable/enable network.": {
3411+
"describeName": "Writes:",
3412+
"itName": "Held writes are not re-sent after disable/enable network.",
3413+
"tags": [],
3414+
"config": {
3415+
"useGarbageCollection": true
3416+
},
3417+
"steps": [
3418+
{
3419+
"userListen": [
3420+
2,
3421+
{
3422+
"path": "collection",
3423+
"filters": [],
3424+
"orderBys": []
3425+
}
3426+
],
3427+
"stateExpect": {
3428+
"activeTargets": {
3429+
"2": {
3430+
"query": {
3431+
"path": "collection",
3432+
"filters": [],
3433+
"orderBys": []
3434+
},
3435+
"resumeToken": ""
3436+
}
3437+
}
3438+
}
3439+
},
3440+
{
3441+
"watchAck": [
3442+
2
3443+
]
3444+
},
3445+
{
3446+
"watchEntity": {
3447+
"docs": [],
3448+
"targets": [
3449+
2
3450+
]
3451+
}
3452+
},
3453+
{
3454+
"watchCurrent": [
3455+
[
3456+
2
3457+
],
3458+
"resume-token-500"
3459+
],
3460+
"watchSnapshot": 500,
3461+
"expect": [
3462+
{
3463+
"query": {
3464+
"path": "collection",
3465+
"filters": [],
3466+
"orderBys": []
3467+
},
3468+
"errorCode": 0,
3469+
"fromCache": false,
3470+
"hasPendingWrites": false
3471+
}
3472+
]
3473+
},
3474+
{
3475+
"userSet": [
3476+
"collection/a",
3477+
{
3478+
"v": 1
3479+
}
3480+
],
3481+
"expect": [
3482+
{
3483+
"query": {
3484+
"path": "collection",
3485+
"filters": [],
3486+
"orderBys": []
3487+
},
3488+
"added": [
3489+
[
3490+
"collection/a",
3491+
0,
3492+
{
3493+
"v": 1
3494+
},
3495+
"local"
3496+
]
3497+
],
3498+
"errorCode": 0,
3499+
"fromCache": false,
3500+
"hasPendingWrites": true
3501+
}
3502+
]
3503+
},
3504+
{
3505+
"writeAck": {
3506+
"version": 1000,
3507+
"expectUserCallback": true
3508+
},
3509+
"stateExpect": {
3510+
"writeStreamRequestCount": 2
3511+
}
3512+
},
3513+
{
3514+
"enableNetwork": false,
3515+
"stateExpect": {
3516+
"activeTargets": {},
3517+
"limboDocs": [],
3518+
"writeStreamRequestCount": 3
3519+
},
3520+
"expect": [
3521+
{
3522+
"query": {
3523+
"path": "collection",
3524+
"filters": [],
3525+
"orderBys": []
3526+
},
3527+
"errorCode": 0,
3528+
"fromCache": true,
3529+
"hasPendingWrites": true
3530+
}
3531+
]
3532+
},
3533+
{
3534+
"enableNetwork": true,
3535+
"stateExpect": {
3536+
"activeTargets": {
3537+
"2": {
3538+
"query": {
3539+
"path": "collection",
3540+
"filters": [],
3541+
"orderBys": []
3542+
},
3543+
"resumeToken": "resume-token-500"
3544+
}
3545+
},
3546+
"writeStreamRequestCount": 3
3547+
}
3548+
},
3549+
{
3550+
"watchAck": [
3551+
2
3552+
]
3553+
},
3554+
{
3555+
"watchEntity": {
3556+
"docs": [
3557+
[
3558+
"collection/a",
3559+
1000,
3560+
{
3561+
"v": 1
3562+
}
3563+
]
3564+
],
3565+
"targets": [
3566+
2
3567+
]
3568+
}
3569+
},
3570+
{
3571+
"watchCurrent": [
3572+
[
3573+
2
3574+
],
3575+
"resume-token-2000"
3576+
],
3577+
"watchSnapshot": 2000,
3578+
"expect": [
3579+
{
3580+
"query": {
3581+
"path": "collection",
3582+
"filters": [],
3583+
"orderBys": []
3584+
},
3585+
"metadata": [
3586+
[
3587+
"collection/a",
3588+
1000,
3589+
{
3590+
"v": 1
3591+
}
3592+
]
3593+
],
3594+
"errorCode": 0,
3595+
"fromCache": false,
3596+
"hasPendingWrites": false
3597+
}
3598+
]
3599+
}
3600+
]
3601+
},
34103602
"Held writes are released when there are no queries left.": {
34113603
"describeName": "Writes:",
34123604
"itName": "Held writes are released when there are no queries left.",

‎Firestore/Source/Local/FSTLevelDBMutationQueue.mm

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -315,7 +315,12 @@ - (nullable FSTMutationBatch *)lookupMutationBatch:(FSTBatchID)batchID {
315315
}
316316

317317
- (nullable FSTMutationBatch *)nextMutationBatchAfterBatchID:(FSTBatchID)batchID {
318-
std::string key = [self mutationKeyForBatchID:batchID + 1];
318+
// All batches with batchID <= self.metadata.lastAcknowledgedBatchId have been acknowledged so
319+
// the first unacknowledged batch after batchID will have a batchID larger than both of these
320+
// values.
321+
FSTBatchID nextBatchID = MAX(batchID, self.metadata.lastAcknowledgedBatchId) + 1;
322+
323+
std::string key = [self mutationKeyForBatchID:nextBatchID];
319324
std::unique_ptr<Iterator> it(_db->NewIterator(StandardReadOptions()));
320325
it->Seek(key);
321326

@@ -336,7 +341,7 @@ - (nullable FSTMutationBatch *)nextMutationBatchAfterBatchID:(FSTBatchID)batchID
336341
return nil;
337342
}
338343

339-
FSTAssert(rowKey.batchID > batchID, @"Should have found mutation after %d", batchID);
344+
FSTAssert(rowKey.batchID >= nextBatchID, @"Should have found mutation after %d", nextBatchID);
340345
return [self decodedMutationBatch:it->value()];
341346
}
342347

‎Firestore/Source/Local/FSTMemoryMutationQueue.mm

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -192,10 +192,10 @@ - (nullable FSTMutationBatch *)nextMutationBatchAfterBatchID:(FSTBatchID)batchID
192192

193193
// All batches with batchID <= self.highestAcknowledgedBatchID have been acknowledged so the
194194
// first unacknowledged batch after batchID will have a batchID larger than both of these values.
195-
batchID = MAX(batchID + 1, self.highestAcknowledgedBatchID);
195+
FSTBatchID nextBatchID = MAX(batchID, self.highestAcknowledgedBatchID) + 1;
196196

197197
// The requested batchID may still be out of range so normalize it to the start of the queue.
198-
NSInteger rawIndex = [self indexOfBatchID:batchID];
198+
NSInteger rawIndex = [self indexOfBatchID:nextBatchID];
199199
NSUInteger index = rawIndex < 0 ? 0 : (NSUInteger)rawIndex;
200200

201201
// Finally return the first non-tombstone batch.

0 commit comments

Comments
 (0)