Dequeue limbo resolutions when their respective queries are stopped #7455
Conversation
…nges in sync_engine.cc
| @@ -1,6 +1,8 @@ | |||
| # Unreleased | |||
| - [fixed] Fixed a crash that could happen when the App is being deleted and | |||
| there's an active listener (#6909). | |||
| - [fixed] Fixed a bug where local cache inconsistencies were unnecessarily | |||
| being resolved (#7455). | |||
There was a problem hiding this comment.
You should explain what the user visible impact is (as you did in Android).
There was a problem hiding this comment.
On Android, there was a clear link between the fix and the user-facing bug; however, there is no such link on iOS (or web). The user-facing bug appeared to be Android-specific since two independent users reported that the bug only surfaced on Android. So I'm not sure there is anything to call out here for iOS, other than a performance improvement. Also, this change log text was copied nearly verbatim for the corresponding change in the Web SDK (https://github.com/firebase/firebase-js-sdk/pull/4395/files#diff-45e189ed0b5ae6e81e5711aa7152c1224e6aae39afa93c98e80994b7f89a2aea)
| bool cancelled_ = false; | ||
| }; | ||
|
|
||
| void PruneLeadingCancelledQueueEntries(); |
There was a problem hiding this comment.
Can you add a method comment to explain what this does?
| @@ -220,6 +219,53 @@ class SyncEngine : public remote::RemoteStoreCallback, public QueryEventSource { | |||
| bool document_received = false; | |||
| }; | |||
|
|
|||
| class LimboResolutionQueue { | |||
There was a problem hiding this comment.
Should this:
- be an outer class? (@var-const)
- have it's own unit tests?
There was a problem hiding this comment.
Done. I've moved this class to the "util" folder under the class name RandomAccessQueue.
| std::vector<model::DocumentKey> keys() const; | ||
|
|
||
| private: | ||
| class QueueEntry { |
There was a problem hiding this comment.
Since this wrapper only seems to exist to support cancellation, I was wondering if you are up for a challenge. You could advance to the stage of a "true hacker" (tm) if you use unordered_map to indicate whether an entry is still active. You can remove inactive entries from the unordered_map and clean up the vector by treating the map as a source of truth.
There was a problem hiding this comment.
Great idea! But I ran into a bit of a snag with it. Suppose a value is added to the queue, then removed, then added again. In this case, the value could be in the queue twice: once from the original add, which was cancelled, and once from the second add, which has not been cancelled. As a result, when the entry for the original add is popped off of the front then it will be present in the unordered_set and appear to be uncancelled when in fact it was cancelled. This may be acceptable for the ordering of limbo resolutions, which are not terribly sensitive to ordering, but in the general case of a queue it breaks the FIFO ordering.
| * This method has constant-time complexity. | ||
| */ | ||
| const T& front() const { | ||
| return queue_.front().element(); |
There was a problem hiding this comment.
Should we assert here that removed is set to false?
| @@ -0,0 +1,647 @@ | |||
| /* | |||
| * Copyright 2017 Google LLC | |||
There was a problem hiding this comment.
- 2017: Trump but no Covid
- 2021: No Trump but Covid
Not sure which one is better, but I would still lean towards 2021.
There was a problem hiding this comment.
Tough call. But I tend to agree. Changed.
| expected_elements, | ||
| std::string("contains elements: ") + | ||
| ToDebugString(expected_elements)) { | ||
| return expected_elements == arg.elements(); |
There was a problem hiding this comment.
Can you use a built-in here? https://chromium.googlesource.com/external/gmock/+/master/test/gmock-generated-matchers_test.cc#387
There was a problem hiding this comment.
Done. Great suggestion.
| EXPECT_FALSE(queue.empty()); | ||
| } | ||
|
|
||
| TEST(RandomAccessQueueTest, ContainsShouldReturnTrueOnANewlyCreatedQueue) { |
There was a problem hiding this comment.
| TEST(RandomAccessQueueTest, ContainsShouldReturnTrueOnANewlyCreatedQueue) { | |
| TEST(RandomAccessQueueTest, ContainsShouldReturnFalseOnANewlyCreatedQueue) { |
Fix a bug where enqueued limbo resolutions are left in the queue even after all targets that care about their resolutions are stopped. This erroneous behavior was reported in firebase/firebase-android-sdk#2311 against the Android SDK, but the bug is also present in the Web and iOS SDKs. This PR is a port of the equivalent fix in the Web SDK: firebase/firebase-js-sdk#4395. This bug was introduced when limbo resolution throttling was implemented almost a year ago (firebase/firebase-js-sdk#2790).