Skip to content

Fix MutationQueue issue resulting in re-sending acknowledged writes. #909

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 12, 2018
Merged

Fix MutationQueue issue resulting in re-sending acknowledged writes. #909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Firestore/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
neither succeeds nor fails within 10 seconds, the SDK will consider itself
"offline", causing getDocument() calls to resolve with cached results, rather
than continuing to wait.
- [fixed] Fixed a potential race condition after calling `enableNetwork()` that
could result in a "Mutation batchIDs must be acknowledged in order" assertion
crash.

# v0.10.3
- [fixed] Fixed a regression in the 4.10.0 Firebase iOS SDK release that
Expand Down
16 changes: 16 additions & 0 deletions Firestore/Example/Tests/Local/FSTMutationQueueTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,22 @@ - (void)testNextMutationBatchAfterBatchID {
XCTAssertNil(notFound);
}

- (void)testNextMutationBatchAfterBatchIDSkipsAcknowledgedBatches {
if ([self isTestBaseClass]) return;

NSMutableArray<FSTMutationBatch *> *batches = [self createBatches:3];
XCTAssertEqualObjects([self.mutationQueue nextMutationBatchAfterBatchID:kFSTBatchIDUnknown],
batches[0]);

[self acknowledgeBatch:batches[0]];
XCTAssertEqualObjects([self.mutationQueue nextMutationBatchAfterBatchID:kFSTBatchIDUnknown],
batches[1]);
XCTAssertEqualObjects([self.mutationQueue nextMutationBatchAfterBatchID:batches[0].batchID],
batches[1]);
XCTAssertEqualObjects([self.mutationQueue nextMutationBatchAfterBatchID:batches[1].batchID],
batches[2]);
}

- (void)testAllMutationBatchesThroughBatchID {
if ([self isTestBaseClass]) return;

Expand Down
192 changes: 192 additions & 0 deletions Firestore/Example/Tests/SpecTests/json/write_spec_test.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3407,6 +3407,198 @@
}
]
},
"Held writes are not re-sent after disable/enable network.": {
"describeName": "Writes:",
"itName": "Held writes are not re-sent after disable/enable network.",
"tags": [],
"config": {
"useGarbageCollection": true
},
"steps": [
{
"userListen": [
2,
{
"path": "collection",
"filters": [],
"orderBys": []
}
],
"stateExpect": {
"activeTargets": {
"2": {
"query": {
"path": "collection",
"filters": [],
"orderBys": []
},
"resumeToken": ""
}
}
}
},
{
"watchAck": [
2
]
},
{
"watchEntity": {
"docs": [],
"targets": [
2
]
}
},
{
"watchCurrent": [
[
2
],
"resume-token-500"
],
"watchSnapshot": 500,
"expect": [
{
"query": {
"path": "collection",
"filters": [],
"orderBys": []
},
"errorCode": 0,
"fromCache": false,
"hasPendingWrites": false
}
]
},
{
"userSet": [
"collection/a",
{
"v": 1
}
],
"expect": [
{
"query": {
"path": "collection",
"filters": [],
"orderBys": []
},
"added": [
[
"collection/a",
0,
{
"v": 1
},
"local"
]
],
"errorCode": 0,
"fromCache": false,
"hasPendingWrites": true
}
]
},
{
"writeAck": {
"version": 1000,
"expectUserCallback": true
},
"stateExpect": {
"writeStreamRequestCount": 2
}
},
{
"enableNetwork": false,
"stateExpect": {
"activeTargets": {},
"limboDocs": [],
"writeStreamRequestCount": 3
},
"expect": [
{
"query": {
"path": "collection",
"filters": [],
"orderBys": []
},
"errorCode": 0,
"fromCache": true,
"hasPendingWrites": true
}
]
},
{
"enableNetwork": true,
"stateExpect": {
"activeTargets": {
"2": {
"query": {
"path": "collection",
"filters": [],
"orderBys": []
},
"resumeToken": "resume-token-500"
}
},
"writeStreamRequestCount": 3
}
},
{
"watchAck": [
2
]
},
{
"watchEntity": {
"docs": [
[
"collection/a",
1000,
{
"v": 1
}
]
],
"targets": [
2
]
}
},
{
"watchCurrent": [
[
2
],
"resume-token-2000"
],
"watchSnapshot": 2000,
"expect": [
{
"query": {
"path": "collection",
"filters": [],
"orderBys": []
},
"metadata": [
[
"collection/a",
1000,
{
"v": 1
}
]
],
"errorCode": 0,
"fromCache": false,
"hasPendingWrites": false
}
]
}
]
},
"Held writes are released when there are no queries left.": {
"describeName": "Writes:",
"itName": "Held writes are released when there are no queries left.",
Expand Down
9 changes: 7 additions & 2 deletions Firestore/Source/Local/FSTLevelDBMutationQueue.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,12 @@ - (nullable FSTMutationBatch *)lookupMutationBatch:(FSTBatchID)batchID {
}

- (nullable FSTMutationBatch *)nextMutationBatchAfterBatchID:(FSTBatchID)batchID {
std::string key = [self mutationKeyForBatchID:batchID + 1];
// All batches with batchID <= self.metadata.lastAcknowledgedBatchId have been acknowledged so
// the first unacknowledged batch after batchID will have a batchID larger than both of these
// values.
FSTBatchID nextBatchID = MAX(batchID, self.metadata.lastAcknowledgedBatchId) + 1;

std::string key = [self mutationKeyForBatchID:nextBatchID];
std::unique_ptr<Iterator> it(_db->NewIterator(StandardReadOptions()));
it->Seek(key);

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

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

Expand Down
4 changes: 2 additions & 2 deletions Firestore/Source/Local/FSTMemoryMutationQueue.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,10 +192,10 @@ - (nullable FSTMutationBatch *)nextMutationBatchAfterBatchID:(FSTBatchID)batchID

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

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

// Finally return the first non-tombstone batch.
Expand Down