Skip to content

Allow remote updates from watch to heal a cache with synthesized deletes in it #1015

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 14 commits into from
Jul 18, 2018
Merged

Allow remote updates from watch to heal a cache with synthesized deletes in it #1015

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
448c522
Write a spec test for the busted cache
wilhuff Jul 17, 2018
6ce8ed4
Relax individual document revert constraints
wilhuff Jul 17, 2018
44850e5
Use a more realistic scenario for watch sending a version back in time
wilhuff Jul 17, 2018
9bc9ba4
Revert "Use a more realistic scenario for watch sending a version bac…
wilhuff Jul 17, 2018
fd9de63
Revert "Relax individual document revert constraints"
wilhuff Jul 17, 2018
e694ead
Disregard NoDocuments when checking against existing document versions.
wilhuff Jul 17, 2018
8cbb935
Modify spec test to demonstrate deletedDoc issue. (#1017)
mikelehen Jul 18, 2018
168d86d
Allow updates for targets where the document is modified
wilhuff Jul 18, 2018
ac4aa80
Use version/resume token to match deleted and non-deleted cases
wilhuff Jul 18, 2018
0dc3eed
Adjust comments and only accept document updates when athoritative
wilhuff Jul 18, 2018
317467e
Remove the need for current consider a change authoritative
wilhuff Jul 18, 2018
7bfb951
Implement review feedback
wilhuff Jul 18, 2018
5b38dba
Merge branch 'master' into wilhuff/allow-healing
wilhuff Jul 18, 2018
b825a94
Merge branch 'master' into wilhuff/allow-healing
wilhuff Jul 18, 2018
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
41 changes: 7 additions & 34 deletions packages/firestore/src/local/local_store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,40 +458,6 @@ export class LocalStore {
}
);

let changedDocKeys = documentKeySet();
remoteEvent.documentUpdates.forEach((key, doc) => {
changedDocKeys = changedDocKeys.add(key);
promises.push(
documentBuffer.getEntry(txn, key).next(existingDoc => {
// Make sure we don't apply an old document version to the remote
// cache, though we make an exception for SnapshotVersion.MIN which
// can happen for manufactured events (e.g. in the case of a limbo
// document resolution failing).
if (
existingDoc == null ||
doc.version.isEqual(SnapshotVersion.MIN) ||
doc.version.compareTo(existingDoc.version) >= 0
) {
documentBuffer.addEntry(doc);
} else {
log.debug(
LOG_TAG,
'Ignoring outdated watch update for ',
key,
'. Current version:',
existingDoc.version,
' Watch version:',
doc.version
);
}

// The document might be garbage because it was unreferenced by
// everything. Make sure to mark it as garbage if it is...
this.garbageCollector.addPotentialGarbageKey(key);
})
);
});

// HACK: The only reason we allow a null snapshot version is so that we
// can synthesize remote events when we get permission denied errors while
// trying to resolve the state of a locally cached document that is in
Expand All @@ -511,6 +477,13 @@ export class LocalStore {
);
}

let changedDocKeys = documentKeySet();
remoteEvent.documentUpdates.forEach((key, doc) => {
changedDocKeys = changedDocKeys.add(key);
documentBuffer.addEntry(doc);
this.garbageCollector.addPotentialGarbageKey(key);
});

let releasedWriteKeys: DocumentKeySet;
return PersistencePromise.waitFor(promises)
.next(() => this.releaseHeldBatchResults(txn, documentBuffer))
Expand Down
40 changes: 38 additions & 2 deletions packages/firestore/test/unit/specs/listen_spec.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,18 +271,54 @@ describeSpec('Listens:', [], () => {
// (the document falls out) and send us a snapshot that's ahead of
// docAv3 (which is already in our cache).
.userListens(visibleQuery, 'resume-token-1000')
.watchAcksFull(visibleQuery, 5000, docAv2)
.watchAcks(visibleQuery)
.watchSends({ affects: [visibleQuery] }, docAv2)
.watchSnapshots(5000)
.watchSends({ affects: [visibleQuery] }, docAv3)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not convinced watch will send us this. It would have to keep track of any document that it previously told us was in the query and somehow continue to send us changes for it even though it's no longer in the query, wouldn't it? Or maybe once it sends us a CURRENT it's allowed to stop? I don't really know what guarantee we're expecting and how watch would implement it.

In any case, I think this test as modified no longer reflects the comment a few lines above. We should probably get @jdimond's blessing and then update the comment as well if we're changing this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this scenario is possible as written then I can't make this change. I could adapt the change to only allow a bypass of the version check in the case of a delete.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. 😦

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Take 4 ready for review. This passes all the tests we've thrown at it but does so by introducing a notion of authoritative updates--updates sent by watch in a configuration that we can blindly trust.

.watchCurrents(visibleQuery, 'resume-token-5000')
.watchSnapshots(6000)
.expectEvents(visibleQuery, { fromCache: false })
.userUnlistens(visibleQuery)
.watchRemoves(visibleQuery)
// Listen to allQuery again and make sure we still get docAv3.
.userListens(allQuery, 'resume-token-4000')
.expectEvents(allQuery, { added: [docAv3], fromCache: true })
.watchAcksFull(allQuery, 6000)
.watchAcksFull(allQuery, 7000)
.expectEvents(allQuery, { fromCache: false })
);
});

specTest('Deleted documents in cache are fixed', [], () => {
const allQuery = Query.atPath(path('collection'));
const docAv1 = doc('collection/a', 1000, { key: 'a' });
const docDeleted = deletedDoc('collection/a', 2000);

return (
spec()
// Presuppose an initial state where the remote document cache has a
// broken synthesized delete at a timestamp later than the true version
// of the document. This requires both adding and later removing the
// document in order to force the watch change aggregator to propagate
// the deletion.
.withGCEnabled(false)
.userListens(allQuery)
.watchAcksFull(allQuery, 1000, docAv1)
.expectEvents(allQuery, { added: [docAv1], fromCache: false })
.watchSends({ removed: [allQuery] }, docDeleted)
.watchSnapshots(2000, [allQuery], 'resume-token-2000')
.watchSnapshots(2000)
.expectEvents(allQuery, { removed: [docAv1], fromCache: false })
.userUnlistens(allQuery)
.watchRemoves(allQuery)

// Now when the client listens expect the cached NoDocument to be
// discarded because the global snapshot version exceeds what came before.
.userListens(allQuery, 'resume-token-2000')
.watchAcksFull(allQuery, 3000, docAv1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - wouldn't this send an existence filter with count of 1 and not send this document (since the resume token is newer than the document)?

It looks like a query with a single NoDocument associated has an document count of 1, and we will not reset the query (only verified by glancing through the code).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's probably true... It would either send us an existence filter, or it could send us a RESET and then the contents.

You could fix it by using a different query than allQuery above to generate the initial state we want (and then we wouldn't include an existence filter here). BUT in general this test is an invalid set of watch events since we're intentionally getting the cache into a bad state. So I'm not sure we care.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed this to use a separate setup query to sidestep the existence filter issue.

.expectEvents(allQuery, { added: [docAv1], fromCache: false })
);
});

specTest('Listens are reestablished after network disconnect', [], () => {
const expectRequestCount = requestCounts =>
requestCounts.addTarget + requestCounts.removeTarget;
Expand Down