Skip to content

Fix priority queue assertion which enforce the item to have strict order #14496

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.

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 2 commits into from
Mar 18, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions Firestore/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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)

Expand Down
7 changes: 6 additions & 1 deletion Firestore/core/src/local/leveldb_index_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
2 changes: 2 additions & 0 deletions Firestore/core/src/model/field_index.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ namespace firebase {
namespace firestore {
namespace model {

std::atomic<int> 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) {
Expand Down
61 changes: 59 additions & 2 deletions Firestore/core/src/model/field_index.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;
}

/**
Expand Down Expand Up @@ -285,6 +330,14 @@ class FieldIndex {
/** Returns the ArrayContains/ArrayContainsAny segment for this index. */
absl::optional<Segment> 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
Expand All @@ -308,6 +361,10 @@ class FieldIndex {
std::string collection_group_;
std::vector<Segment> segments_;
IndexState state_;
int unique_id_;

// TODO(C++17): Replace with inline static std::atomic<int> ref_count_ = 0;
static std::atomic<int> ref_count_;
};

inline bool operator==(const FieldIndex& lhs, const FieldIndex& rhs) {
Expand Down
Loading