lib: make the queue contract true, and enforce what prose cannot - #15
Merged
Merged
Conversation
Four documented promises did not hold, and nothing in the suite could have caught any of them: every T in the tests has a noexcept move assignment, so the exception paths were unreachable, and no test drove a non-blocking retry loop to termination. **The exception contract was false for two queues.** MutexQueue and SpscQueue both claimed "the failing pop()/try_pop() leaves the element queued". Both dequeue with `out = std::move(slot)`, so a throw part-way through leaves out modified and the queued element hollowed. What actually holds is the queue-side invariant: the indices do not move, so nothing is lost or duplicated and the count still reconciles. The headers now say that and nothing more. The push side is genuinely clean on both — the slot is written before the index advances — and that half is kept. **MpmcQueue's version was accurate and unenforced.** It described the damage correctly (a throw strands a claimed ticket whose sequence is never re-published, so later operations on that slot spin forever) and then asked in prose for a T whose move assignment cannot throw. That is compile-time-checkable, so it is now a static_assert. No existing instantiation is affected. **try_push could not distinguish "full" from "closed"** on any of the three, and closed() — the only discriminator — was documented as advisory with callers told not to drive control flow from it. So the natural idiom `while (!q.try_push(v)) yield();` spins forever after close(). closed() now has one documented sanctioned use, and each @return names it. The same doc block covers the by-value hazard: try_push is a sink, so a failed attempt has already consumed an rvalue argument, and a retry loop must re-materialise its argument rather than reuse the object. A move-only value that cannot be re-created has no correct retry loop at all. **"consumed even when the push fails" appeared seven times** across the three headers and is false for lvalue arguments, which are copied and left intact. Only rvalues are consumed. Tests for the paths that let these survive. Two new typed contract tests run over all three queues: one drives both retry loops to termination through closed(), and would pass vacuously if it did not first assert that a full-but-open queue reports closed() == false; the other pins the rvalue/lvalue asymmetry and the move-only consequence. A separate ThrowingMoveContract suite covers MutexQueue and SpscQueue with a counter-armed throwing type, pinning both what the queues guarantee and what they explicitly do not. MpmcQueue is absent from it by construction — its static_assert is its test. Two guards that were not guarding. STYLE.md promised -Wdocumentation enforcement "or the build fails", but nothing promoted it past a warning: a renamed @PARAM warned and exited 0. It is now -Werror=documentation, and the tree is clean under it. And CI's Release job configured -DCQ_BUILD_TESTS=OFF, so the suite only ever ran Debug+TSan — no NDEBUG path, no optimiser, and a much narrower interleaving space. Two of the three queues are lock-free, which is exactly where -O0 and -O3 diverge. README updated: the contract table gains the retry-loop rule and the throwing-move divergence, and the element-type row notes MpmcQueue's extra requirement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round-1 review follow-ups. The headline one repeats the pattern this PR was written to end: broadening "the failing push()/try_push()" into "the queue's invariants hold, nothing is lost or duplicated" swept in the two functions that genuinely can lose data. SpscQueue::try_push_n and try_pop_n publish one index for the whole batch, so a throw part-way through strands the already-moved elements outside both the caller's span and the queue (push) or inside both (pop). Reproduced at capacity 8 with a 4-element batch throwing on the third move: push loses two elements outright; pop yields six observed elements from four pushed, two of them hollowed. They are the only functions in the library that can do this, and they were the only ones the new suite did not cover. Fixed where it belongs rather than in prose: both bulk functions now static_assert a noexcept move assignment, the same remedy MpmcQueue uses and for the same reason. Because they are non-template members, SpscQueue<T> for a throwing T still compiles as long as the bulk ops are not called — which is what keeps ThrowingMoveContract able to instantiate it. Verified both directions: single-element use of such a T compiles, bulk use does not. Six smaller corrections from the same round: - The sanctioned consumer idiom was lossy. Breaking on closed() alone strands an element a producer pushed between the failed try_pop and the check; measured, the naive form exits with size() == 1. The README now shows the re-attempt that blocking pop() performs internally, and the contract test uses it rather than enshrining the naive form. - "untouched on failure" and "a failing pop" used "failure" for two different events in the same header — a false return and a throw. Split the vocabulary. - "retrying the pop yields a hollowed element" asserted a specific outcome one sentence after correctly calling the states unspecified. Now "may yield". - Blocking push(), recommended as the escape for an unrecoverable move-only value, drops that value on a closed queue. It narrows the window; it does not close it, and now says so. - MpmcQueue's @tparam still listed only DefaultConstructible and MoveAssignable while the class prose and README both stated the noexcept requirement. The @tparam is where a reader looks. - MpmcQueue's damage description said the queue "degrades (later operations on the slot spin)". It is worse: the consumer can never advance past the stranded ticket, and the producer stops once the ring wraps onto it. That understatement was the justification for the static_assert. Also cut the twelve-line retry-loop worked example that was triplicated across the three headers down to three lines and a pointer. It is prose rather than anything -Wdocumentation can check, any correction to it had to be made three times, and the README is where the idiom and its two traps now live. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit replaced a lossy consumer idiom with a differently
lossy one, and justified it with a claim about pop() that is not true.
while (!q.try_pop(out)) {
if (q.closed() && !q.try_pop(out)) break;
}
When the inner try_pop succeeds, && short-circuits, so the loop does not
break; the loop condition then calls try_pop again and overwrites out,
discarding the element the inner pop just retrieved. Measured against a
producer that pushes twice and closes: 91 of 400 trials lost an element.
The replacement loses none.
pop() does not do what the comment claimed. It is
`if (closed()) return try_pop(out);` — it returns whatever the re-attempt
gives, unconditionally. The README idiom now mirrors that, written as the
drain loop a consumer actually wants, and the contract test uses it rather
than pinning the broken form.
That test had also stopped testing what it is named for. Dropping the
pre-drain meant its first try_pop succeeded, so the closed()-based break
never ran and the consumer half would have passed with closed() stuck at
false. The drain-loop shape restores it: reaching the break requires
consulting closed(), and a closed() stuck false trips the spin guard.
"Or stop the producers before calling close()" had the advice inverted. It
offered that as an alternative to the re-attempt, which holds only for
MutexQueue, where both reads happen under one lock. For SpscQueue and
MpmcQueue — the two the sentence named — the re-attempt is what orders the
consumer after the producer's last push, so both are needed, not either.
Three claims left half-corrected in the README while the headers were
fixed: the element-type row omitted SpscQueue's bulk requirement, so a
reader planning try_push_n with a throwing T was told it would work when it
does not compile; the throwing-move paragraph still said "yields" where the
headers now say "may yield"; and SpscQueue's own @tparam said nothing about
the requirement, in a commit that argued the @tparam is where readers look.
Also move <type_traits> to the .ipp that uses it, and state the bulk
static_assert's rationale once instead of twice — the message itself cannot
be factored out, since C++20 requires a string literal there.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ters on
The previous commit shipped a correct drain loop with an explanation that
licensed dropping the part that makes it correct — the same failure mode as
the commit before it.
Under MutexQueue both reads happen under one lock, so "closed and
drained" has no window and the re-attempt is merely harmless.
Inverted. In the loop the reader is shown, try_pop, closed() and the second
try_pop are three separate acquisitions; nothing spans them. MutexQueue has
the widest window of the three, not the narrowest, because the lock hand-off
after a failed try_pop is long enough for the producer to finish both pushes
and the close before the consumer reacquires. Per 400 trials, breaking on
closed() alone:
MutexQueue 139-178 lost SpscQueue 4-10 MpmcQueue 2-6
against zero for the loop as written, on all three. "Merely harmless" was an
invitation to drop the re-attempt on the one queue that needs it about
thirty times more than the others.
The true fact it was derived from is about MutexQueue::pop, not about the
reader's loop: pop() resolves closed-and-drained under one lock, which is
also why MutexQueue::close carries no stop-producers-first precondition
while SpscQueue's and MpmcQueue's do. Both are now stated where they belong,
and the parenthetical "which is what pop() does" is scoped to the two queues
whose pop() is that line verbatim.
Also: drop <type_traits> from spsc_queue.hpp, which uses no trait — the
previous commit said it moved the include and only added one. Correct the
.ipp comment claiming a stranded MpmcQueue slot makes "every later operation
spin forever": try_push/try_pop report it full or empty forever and only the
blocking forms spin. Re-wrap three doc lines left running past the paragraph
width, move the bulk carve-out to the end of SpscQueue's Exceptions
paragraph so "element values are not protected" no longer reads as if it
were about the bulk ops, and note that the moved-from std::string assertion
is a libstdc++/libc++ observation rather than a standard guarantee.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round-4 review. The previous commit named a false claim, fixed one of the two copies, and shipped the other — and said it moved a sentence when it duplicated it. README still carried "every later operation on that slot spins forever" for a stranded MpmcQueue ticket, the exact wording 8dd626f identified as wrong and corrected in mpmc_queue.hpp. The PR shipped two contradictory descriptions of one failure mode, with the wrong one in the more-read artifact. What actually happens: try_push/try_pop report that slot full or empty forever, and only the blocking forms spin on it. SpscQueue's Exceptions paragraph came out worse than it went in. The "element values are not protected" sentence was copied rather than moved, so it appeared in both paragraphs; the single-element paragraph lost the "a failing push()/try_push() enqueues nothing" clause into the bulk one, where it does not belong; and the bulk paragraph ended on "none of this is reachable for a T whose move assignment is noexcept", which is vacuous there because those two ops static_assert exactly that. Rewritten as two paragraphs that each say one thing: the single-element guarantees and their limits, then why the bulk ops cannot offer even the first of them. "no lock or ordering spans them" overstated the point into a contradiction with the paragraph nineteen lines later. On SpscQueue and MpmcQueue an ordering does span closed() and the re-attempt — closed()'s acquire load pairs with close()'s release store, which is precisely what that later paragraph relies on. The intended claim is that the three steps are not one atomic unit, and it now says that. The loss table now names the machine it was measured on and says the ratio between rows is the point rather than the counts, which move: a second run on different hardware got 152-208 / 4-22 / 0-15 against the 139-178 / 4-10 / 2-6 recorded here, same ordering, same conclusion. Also re-wrap the two mpmc_queue.hpp lines a previous commit left at 90 and 93 characters while claiming to have re-wrapped exactly this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r exception paragraphs, drop the unreproducible table Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e, drop the implementation-defined string half Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four documented promises did not hold, and no test could reach them — every
Tin the suite has anoexceptmove assignment, and nothing drove a non-blocking retry loop to termination.MutexQueue/SpscQueue. Both claimed a failing pop "leaves the element queued".out = std::move(slot)throwing leavesoutmodified and the queued element hollowed; only the indices hold. The headers now say that and no more.MpmcQueueasked in prose for anoexceptmove assignment. A throw strands a claimed ticket and the queue stalls on it. Now astatic_assert.try_pushcould not tell "full" from "closed", andclosed()was documented as not for control flow.closed()now has one sanctioned use; the README carries both retry idioms and their traps.try_pushretry loop must re-materialise its argument.Found while fixing:
SpscQueue's bulk ops publish one index per batch, so a mid-batch throw loses or duplicates elements — the only place in the library that can. Theystatic_assertanoexceptmove assignment; single-element use of a throwingTstill compiles.Also:
-Wdocumentationis now-Werror=documentation(STYLE.md already promised it), and the Release CI job builds and runs the tests instead of skipping them.Files
include/cq/{mutex,spsc,mpmc}_queue.hppclosed()as the retry discriminator; one class-level note on by-value arguments;static_assertonMpmcQueueinclude/cq/spsc_queue.ippstatic_assertontry_push_n/try_pop_ntests/queue_contract_test.cppNonBlockingRetryLoopsTerminateViaClosed,FailedPushConsumesRvaluesAndLeavesLvaluesIntact(all three);ThrowingMoveContract.*(MutexQueue,SpscQueue)README.mdCMakeLists.txt,STYLE.md,.github/workflows/ci.yml-Werror=documentation; Release job runs testsVerification
Release
ctest78/78, TSan 76/76,--gtest_shuffle --gtest_repeat=20clean,clang-format/clang-tidyclean. Bulk guard checked both ways: single-element use of a throwingTcompiles, bulk use does not.🤖 Generated with Claude Code