Dequeue limbo resolutions when their respective queries are stopped #2404
Conversation
…ng one if already enqueued.
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (1bb4d403) is created by Prow via merging commits: be59902 b428adc. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (1bb4d403) is created by Prow via merging commits: be59902 b428adc. |
firebase-firestore/CHANGELOG.md
Outdated
| @@ -4,6 +4,9 @@ | |||
| Bundles contain pre-packaged data produced with the NodeJS Server SDK and | |||
| can be used to populate Firestore's cache without reading documents from | |||
| the backend. | |||
| - [fixed] Fix a Firestore bug where local cache inconsistencies were | |||
| unnecessarily being resolved, resulting in the incompletion of `Task` objects | |||
There was a problem hiding this comment.
s/incompletion/something that is easier to parse
firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java
Outdated
Show resolved
Hide resolved
| import com.google.common.collect.ImmutableMap; | ||
| import com.google.common.collect.ImmutableSet; |
There was a problem hiding this comment.
One of our goals is to eventually kick out Guava from our dependencies. Can you find another solution?
…ions()`. Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>
| public Queue<DocumentKey> getEnqueuedLimboDocumentResolutions() { | ||
| // Make a defensive copy as the Queue continues to be modified. | ||
| return new ArrayDeque<>(enqueuedLimboResolutions); | ||
| public LinkedHashSet<DocumentKey> getEnqueuedLimboDocumentResolutions() { |
There was a problem hiding this comment.
Super nit (here and above): We usually try to use more generic types in our return types, as this allows us to change the implementation without changing the callsites. If the callsites don't require the HashMap or LinkedHashSet functionality, I would just use Map<> and Set<>.
Please note that this might not apply if getEnqueuedLimboDocumentResolutions() only uses the Set API but its consumers rely on the iteration order of a LinkedHashSet. Then I would still return LinkedHashSet as we will not be able to replace it later. You should still generify the signature in line 713 though.
| @@ -694,7 +694,9 @@ private void trackLimboChange(LimboDocumentChange change) { | |||
| private void pumpEnqueuedLimboResolutions() { | |||
| while (!enqueuedLimboResolutions.isEmpty() | |||
| && activeLimboTargetsByKey.size() < maxConcurrentLimboResolutions) { | |||
| DocumentKey key = enqueuedLimboResolutions.remove(); | |||
| Iterator<DocumentKey> it = enqueuedLimboResolutions.iterator(); | |||
There was a problem hiding this comment.
Looks like you need to import Iterator.
…some return types to more general types (e.g. LinkedHashSet -> List).
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 #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).