Skip to content

Fix circular_queue pop_front(count) debug assert and empty() - #141

Open
rootkiller6788 wants to merge 1 commit into
facebook:mainfrom
rootkiller6788:fix-circular-queue-pop-front-count-and-empty
Open

Fix circular_queue pop_front(count) debug assert and empty()#141
rootkiller6788 wants to merge 1 commit into
facebook:mainfrom
rootkiller6788:fix-circular-queue-pop-front-count-and-empty

Conversation

@rootkiller6788

Copy link
Copy Markdown

Summary

Two bugs in circular_queue:

  1. pop_front(count) aborts in debug builds on valid input. The debug assumption FATAL_ASSUME_EQ(offset_ == 0, count == chunk) is wrong: offset_ wraps to zero exactly when the first chunk reaches the end of the backing buffer, which is unrelated to count == chunk. Reproduced: q.push_back(0..2); q.pop_front(1); aborts at circular_queue.h:262 because offset_ == 0 is false while count == chunk is true. Similarly a wrap-around pop with count > chunk aborts because offset_ == 0 is true while count == chunk is false. The only input that survived was count == buffer_size - offset_ exactly. The assertion now encodes the real safety precondition of the second loop: when more elements remain to be destroyed beyond the first chunk, the buffer must have wrapped around (so offset_ was reset to 0).

  2. empty() reports the wrong value. It returned queue_.empty() (emptiness of the backing std::vector) instead of size_ == 0. After all elements are popped, size() is 0 but empty() stays false because the backing vector still holds storage. This breaks the canonical while (!q.empty()) { q.pop_front(); } idiom (it never terminates) and lets front()/back() read already-destroyed elements. It now returns size_ == 0.

Test plan

  • Added regression tests: pop_front_count, pop_front_count_wrap, empty_after_drain.
  • pop_front_count aborts against the old header (wrong assumption); empty_after_drain hangs forever against the old empty().
  • Full circular_queue suite passes with the fix (13/13), plus a randomized model test (5000 mixed push/pop ops, std::deque oracle) and a std::string wrap-around test.

pop_front(count) used a wrong debug assumption: it asserted that
offset_ wraps to zero exactly when count == chunk, but offset_ only
wraps when the first chunk reaches the end of the buffer. Any valid
pop_front(count) that does not wrap (or that pops more than one chunk)
aborted the process in debug builds. The assertion now checks the actual
safety precondition: when more elements remain beyond the first chunk,
the buffer must have wrapped around.

empty() reported the backing vector's emptiness instead of the queue's
logical emptiness, so a drained queue (size() == 0) still reported
empty() == false, breaking the while (!q.empty()) pop_front() idiom
(and enabling front()/back() to read destroyed elements). It now
returns size_ == 0.

Add regression tests for pop_front(count) with and without wrap-around
and for empty() after draining.
@meta-cla meta-cla Bot added the CLA Signed label Aug 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant