b/72533250: Fix issue with limbo resolutions triggering incorrect manufactured deletes. #1556
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ufactured deletes. [Port of firebase/firebase-js-sdk#1014] This fixes an issue occurring when a limbo target receives a documentUpdate, then a global snapshot, and then a CURRENT. Because there was a global snapshot before the CURRENT, WatchChangeAggregator has no pending document updates and calls SyncEngine.remoteKeysForTarget to see if we previously got any document from the backend for the target. See: https://github.com/firebase/firebase-js-sdk/blob/6905339235ad801291edc696dd75a08e80647f5b/packages/firestore/src/remote/watch_change.ts#L422 Prior to this change, remoteKeysForTarget returned empty because it relies on our Views to track the contents of the target, and we don't have Views for limbo targets. Thus WatchChangeAggregator incorrectly manufactures a NoDocument document update which deletes data from our cache. The fix is to have SyncEngine track the fact that we did indeed get a document for the limbo resolution and return it from remoteKeysForTarget.
wilhuff
reviewed
Jul 19, 2018
| /** Tracks a limbo resolution. */ | ||
| class LimboResolution { | ||
| public: | ||
| LimboResolution() : key{DocumentKey()} { |
There was a problem hiding this comment.
DocumentKey as a non-POD type will default initialize itself so you don't need to explicitly default initialize key. bool is a POD type so it has no default initializer, so you do need to initialize it.
Rather than repeat the initialization of documentReceived, add a default assignment below (bool document_received = false;).
With that in place, this can just be
LimboResolution() {
}| * ultimately used by FSTWatchChangeAggregator to decide whether it needs to manufacture a delete | ||
| * event for the target once the target is CURRENT. | ||
| */ | ||
| bool documentReceived; |
| // Since this is a limbo resolution lookup, it's for a single document and it could be | ||
| // added, modified, or removed, but not a combination. | ||
| HARD_ASSERT(change.addedDocuments.size() + change.modifiedDocuments.size() + | ||
| change.removedDocuments.size() <= |
There was a problem hiding this comment.
Did clang-format really do this? Yikes.
| @@ -484,7 +536,7 @@ - (void)trackLimboChange:(FSTLimboDocumentChange *)limboChange { | |||
| targetID:limboTargetID | |||
| listenSequenceNumber:kIrrelevantSequenceNumber | |||
| purpose:FSTQueryPurposeLimboResolution]; | |||
| _limboKeysByTarget[limboTargetID] = key; | |||
| _limboResolutionsByTarget[limboTargetID] = LimboResolution{key}; | |||
There was a problem hiding this comment.
This default constructs the LimboResolution and then assigns it. Better to make this:
_limboResolutionsByTarget.emplace(limboTargetID, LimboResolution{key});
|
In the interest of time I've just fixed the style issues. |
|
Hah, thanks! :) |
wilhuff
pushed a commit
that referenced
this pull request
Jul 19, 2018
…ufactured deletes. (#1556) [Port of firebase/firebase-js-sdk#1014] This fixes an issue occurring when a limbo target receives a documentUpdate, then a global snapshot, and then a CURRENT. Because there was a global snapshot before the CURRENT, WatchChangeAggregator has no pending document updates and calls SyncEngine.remoteKeysForTarget to see if we previously got any document from the backend for the target. See: https://github.com/firebase/firebase-js-sdk/blob/6905339235ad801291edc696dd75a08e80647f5b/packages/firestore/src/remote/watch_change.ts#L422 Prior to this change, remoteKeysForTarget returned empty because it relies on our Views to track the contents of the target, and we don't have Views for limbo targets. Thus WatchChangeAggregator incorrectly manufactures a NoDocument document update which deletes data from our cache. The fix is to have SyncEngine track the fact that we did indeed get a document for the limbo resolution and return it from remoteKeysForTarget.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: I wrote C++ code so please review carefully.
[Port of firebase/firebase-js-sdk#1014]
This fixes an issue occurring when a limbo target receives a documentUpdate,
then a global snapshot, and then a CURRENT. Because there was a global
snapshot before the CURRENT, WatchChangeAggregator has no pending document
updates and calls SyncEngine.remoteKeysForTarget to see if we previously got any
document from the backend for the target. See:
https://github.com/firebase/firebase-js-sdk/blob/6905339235ad801291edc696dd75a08e80647f5b/packages/firestore/src/remote/watch_change.ts#L422
Prior to this change, remoteKeysForTarget returned empty because
it relies on our Views to track the contents of the target, and we don't
have Views for limbo targets. Thus WatchChangeAggregator incorrectly
manufactures a NoDocument document update which deletes data from our
cache.
The fix is to have SyncEngine track the fact that we did indeed get
a document for the limbo resolution and return it from
remoteKeysForTarget.