diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md index a006d39ab7b..6a30f747984 100644 --- a/Firestore/CHANGELOG.md +++ b/Firestore/CHANGELOG.md @@ -1,3 +1,6 @@ +# Unreleased +- [fixed] Fixed the customized priority queue compare function used cache index manager. (#14496) + # 11.9.0 - [fixed] Fixed memory leak in `Query.whereField()`. (#13978) diff --git a/Firestore/core/src/local/leveldb_index_manager.cc b/Firestore/core/src/local/leveldb_index_manager.cc index eb41c79ad0b..0455bf88898 100644 --- a/Firestore/core/src/local/leveldb_index_manager.cc +++ b/Firestore/core/src/local/leveldb_index_manager.cc @@ -188,10 +188,15 @@ LevelDbIndexManager::LevelDbIndexManager(const User& user, // The contract for this comparison expected by priority queue is // `std::less`, but std::priority_queue's default order is descending. // We change the order to be ascending by doing left >= right instead. + // Note: priority queue has to have a strict ordering, so here using unique_id + // to order Field Indexes having same `sequence_number` and `collection_group` auto cmp = [](FieldIndex* left, FieldIndex* right) { if (left->index_state().sequence_number() == right->index_state().sequence_number()) { - return left->collection_group() >= right->collection_group(); + if (left->collection_group() == right->collection_group()) { + return left->unique_id() > right->unique_id(); + } + return left->collection_group() > right->collection_group(); } return left->index_state().sequence_number() > right->index_state().sequence_number(); diff --git a/Firestore/core/src/model/field_index.cc b/Firestore/core/src/model/field_index.cc index 9257a4fb66f..4cf58e4d988 100644 --- a/Firestore/core/src/model/field_index.cc +++ b/Firestore/core/src/model/field_index.cc @@ -20,6 +20,8 @@ namespace firebase { namespace firestore { namespace model { +std::atomic FieldIndex::ref_count_{0}; + util::ComparisonResult Segment::CompareTo(const Segment& rhs) const { auto result = field_path().CompareTo(rhs.field_path()); if (result != util::ComparisonResult::Same) { diff --git a/Firestore/core/src/model/field_index.h b/Firestore/core/src/model/field_index.h index 57f159e9c67..992aa941767 100644 --- a/Firestore/core/src/model/field_index.h +++ b/Firestore/core/src/model/field_index.h @@ -243,7 +243,9 @@ class FieldIndex { static util::ComparisonResult SemanticCompare(const FieldIndex& left, const FieldIndex& right); - FieldIndex() : index_id_(UnknownId()) { + FieldIndex() + : index_id_(UnknownId()), + unique_id_(ref_count_.fetch_add(1, std::memory_order_acq_rel)) { } FieldIndex(int32_t index_id, @@ -253,7 +255,50 @@ class FieldIndex { : index_id_(index_id), collection_group_(std::move(collection_group)), segments_(std::move(segments)), - state_(std::move(state)) { + state_(std::move(state)), + unique_id_(ref_count_.fetch_add(1, std::memory_order_acq_rel)) { + } + + // Copy constructor + FieldIndex(const FieldIndex& other) + : index_id_(other.index_id_), + collection_group_(other.collection_group_), + segments_(other.segments_), + state_(other.state_), + unique_id_(ref_count_.fetch_add(1, std::memory_order_acq_rel)) { + } + + // Copy assignment operator + FieldIndex& operator=(const FieldIndex& other) { + if (this != &other) { + index_id_ = other.index_id_; + collection_group_ = other.collection_group_; + segments_ = other.segments_; + state_ = other.state_; + unique_id_ = ref_count_.fetch_add(1, std::memory_order_acq_rel); + } + return *this; + } + + // Move constructor + FieldIndex(FieldIndex&& other) noexcept + : index_id_(other.index_id_), + collection_group_(std::move(other.collection_group_)), + segments_(std::move(other.segments_)), + state_(std::move(other.state_)), + unique_id_(ref_count_.fetch_add(1, std::memory_order_acq_rel)) { + } + + // Move assignment operator + FieldIndex& operator=(FieldIndex&& other) noexcept { + if (this != &other) { + index_id_ = other.index_id_; + collection_group_ = std::move(other.collection_group_); + segments_ = std::move(other.segments_); + state_ = std::move(other.state_); + unique_id_ = ref_count_.fetch_add(1, std::memory_order_acq_rel); + } + return *this; } /** @@ -285,6 +330,14 @@ class FieldIndex { /** Returns the ArrayContains/ArrayContainsAny segment for this index. */ absl::optional GetArraySegment() const; + /** + * Returns the unique identifier for this object, ensuring a strict ordering + * in the priority queue's comparison function. + */ + int unique_id() const { + return unique_id_; + } + /** * A type that can be used as the "Compare" template parameter of ordered * collections to have the elements ordered using @@ -308,6 +361,10 @@ class FieldIndex { std::string collection_group_; std::vector segments_; IndexState state_; + int unique_id_; + + // TODO(C++17): Replace with inline static std::atomic ref_count_ = 0; + static std::atomic ref_count_; }; inline bool operator==(const FieldIndex& lhs, const FieldIndex& rhs) {