From 80a0f5a505438d70e68ff64e26401fc1fee772e4 Mon Sep 17 00:00:00 2001 From: Debra Date: Fri, 28 Aug 2026 13:49:22 +0800 Subject: [PATCH 1/7] lib: make the queue contract true, and enforce what prose cannot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .github/workflows/ci.yml | 14 +++- CMakeLists.txt | 6 +- README.md | 19 ++++- STYLE.md | 4 +- include/cq/mpmc_queue.hpp | 38 +++++++-- include/cq/mutex_queue.hpp | 41 +++++++-- include/cq/spsc_queue.hpp | 37 ++++++-- tests/queue_contract_test.cpp | 153 ++++++++++++++++++++++++++++++++++ 8 files changed, 281 insertions(+), 31 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 55995cc..c49ba14 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,16 +32,22 @@ jobs: - name: Test run: ctest --test-dir build-tsan --output-on-failure --no-tests=error - bench-build: - name: Benchmarks (Release, smoke-run) + release: + name: Tests + benchmark smoke-run (Release) runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - name: Configure - run: cmake -B build-rel -DCMAKE_BUILD_TYPE=Release -DCQ_BUILD_TESTS=OFF + # Tests stay ON. Without this job they only ever run Debug+TSan, which + # compiles no NDEBUG path, applies no optimiser, and runs the checksum + # stress test roughly an order of magnitude slower — a very different + # and much narrower slice of the interleaving space. Two of the three + # queues here are lock-free, which is exactly the code where -O0 and + # -O3 diverge. + run: cmake -B build-rel -DCMAKE_BUILD_TYPE=Release - name: Build run: cmake --build build-rel - - name: Smoke-run benchmarks + - name: Test and smoke-run benchmarks run: ctest --test-dir build-rel --output-on-failure --no-tests=error lint: diff --git a/CMakeLists.txt b/CMakeLists.txt index fe1c200..0ec9ff0 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -36,8 +36,10 @@ target_link_libraries(cq INTERFACE Threads::Threads) add_library(cq_warnings INTERFACE) target_compile_options(cq_warnings INTERFACE $<$:-Wall -Wextra -Wpedantic -Wconversion> - # Validates Doxygen comments against signatures (clang-only). - $<$:-Wdocumentation> + # Validates Doxygen comments against signatures (clang-only). An error, not a + # warning, because STYLE.md promises the check is enforced — as a warning it + # exits 0 and a stale @param ships green. + $<$:-Wdocumentation -Werror=documentation> $<$:/W4>) # Applied only to our own targets: instrumenting the FetchContent deps just diff --git a/README.md b/README.md index 4dc40b8..ae14c7b 100644 --- a/README.md +++ b/README.md @@ -49,8 +49,8 @@ entirely, which is only possible by giving something else up. ## The contract -All three promise the same things. Choose between them on threading, not on -behaviour. +All three promise the same things, with one exception noted below. Choose +between them on threading, not on behaviour. | | | |---|---| @@ -61,11 +61,23 @@ behaviour. | **Shutdown** | `close()` refuses new items and wakes every waiter | | **Draining** | Items already queued still come out after `close()` | | **Loss** | Nothing accepted is dropped, duplicated, or reordered | -| **Element type** | Any `T` that is `DefaultConstructible` and `MoveAssignable` | +| **Element type** | Any `T` that is `DefaultConstructible` and `MoveAssignable` — plus `noexcept` move assignment for `MpmcQueue`, which `static_assert`s it | | **Lifetime** | The queue must outlive every thread using it | +| **Non-blocking loops** | `try_push`/`try_pop` return `false` for "not now" and "never again" alike; `closed()` is what lets a retry loop terminate | +| **A throwing move** | The one place the three differ — see below | Every operation reports whether it succeeded, and every one is `[[nodiscard]]`. +**If `T`'s move assignment can throw**, that is where the three part company. +`MutexQueue` and `SpscQueue` keep their indices intact — nothing is lost or +duplicated, and the count still reconciles — but the element values are not +protected: a failed pop leaves both the destination and the still-queued +element in valid-but-unspecified states, so retrying yields a hollowed element. +`MpmcQueue` cannot survive it at all: a throw strands a claimed ticket whose +sequence is never re-published, and every later operation on that slot spins +forever. It therefore refuses such a `T` at compile time rather than degrading +silently. + ## The three queues They differ on one axis — **how many threads may touch each side** — and read @@ -78,6 +90,7 @@ as a sequence of trades. | Exceeding that | — | **undefined behaviour, no diagnostic** | — | | Bounded wait (`try_push_for`) | yes | no | no | | Bulk ops (`try_push_n` / `try_pop_n`) | no | yes | no | +| Throwing move assignment | survivable | survivable | rejected at compile time | | A blocked thread | sleeps | spins briefly, then sleeps | spins briefly, then sleeps | | Built from | mutex + condition variables | atomics only | atomics only | diff --git a/STYLE.md b/STYLE.md index 6df0b48..c12c326 100644 --- a/STYLE.md +++ b/STYLE.md @@ -13,8 +13,8 @@ checked in CI). - **Internal code** (tests, benchmarks, private members, function bodies): plain `//` prose. Explain *why*, not *what*. - Tag hygiene is compiler-enforced: clang builds compile with - `-Wdocumentation`, which rejects `@param` names that do not match the - signature. Keep comments in sync with code or the build fails. + `-Wdocumentation -Werror=documentation`, which rejects `@param` names that + do not match the signature. Keep comments in sync with code or the build fails. - `TODO(username): description` for known follow-ups. ## Layout diff --git a/include/cq/mpmc_queue.hpp b/include/cq/mpmc_queue.hpp index a90ce89..b1772a3 100644 --- a/include/cq/mpmc_queue.hpp +++ b/include/cq/mpmc_queue.hpp @@ -3,6 +3,7 @@ #include #include +#include #include #include "cq/backoff.hpp" @@ -20,7 +21,10 @@ namespace cq { /// Thread-safety: after construction, all member functions may be called /// concurrently from any number of threads. closed() and size() return /// advisory snapshots — drive control flow off the push/pop return values -/// instead. +/// instead, with one exception: try_push()/try_pop() return false for "not +/// now" and for "never again" alike, so a non-blocking retry loop needs +/// closed() to terminate. See try_push() for how such a loop must handle its +/// argument. /// /// Lifetime: the queue must outlive every thread using it — call close() and /// join all producers/consumers before destruction. Destroying the queue @@ -30,7 +34,8 @@ namespace cq { /// slot's sequence is never re-published and the queue degrades (later /// operations on the slot spin); unlike the locked queue there is no way to /// return a claimed ticket. Use element types whose move assignment cannot -/// throw. +/// throw — the static_assert below enforces that, because the damage is +/// silent and unrecoverable. /// /// @tparam T Element type. Must be DefaultConstructible (ring slots are /// constructed up front) and MoveAssignable. @@ -39,6 +44,13 @@ template // counter gets a private cache line (see cq/cache_line.hpp). // NOLINTNEXTLINE(clang-analyzer-optin.performance.Padding) class MpmcQueue { + // Prose cannot enforce this and the failure is silent: a throwing move + // assignment leaves a claimed ticket whose sequence is never re-published, + // so every later operation on that slot spins forever. + static_assert(std::is_nothrow_move_assignable_v, + "MpmcQueue requires a T whose move assignment is noexcept: a throw would " + "strand a claimed slot and permanently degrade the queue"); + public: /// @param capacity Fixed number of slots; never resized. /// @throws std::invalid_argument if capacity is 0. @@ -52,13 +64,29 @@ class MpmcQueue { MpmcQueue& operator=(MpmcQueue&&) = delete; /// Enqueues a value, spinning while the queue is full. - /// @param value Element to enqueue; consumed even when the push fails. + /// @param value Element to enqueue, taken by value. An rvalue argument is + /// moved from at the call — including when the push fails, in which case + /// the value is discarded. An lvalue argument is copied and left intact. /// @return false if the queue is closed (the value is dropped). [[nodiscard]] bool push(T value); /// Enqueues a value without blocking. - /// @param value Element to enqueue; consumed even when the push fails. - /// @return false if the queue is full or closed. + /// + /// A retry loop must re-materialise its argument every pass: this is a + /// by-value sink, so a failed attempt has already consumed an rvalue and + /// retrying with the same object pushes a moved-from husk, silently. + /// + /// while (!q.try_push(make_value())) { // NOT try_push(std::move(v)) + /// if (q.closed()) break; + /// } + /// + /// A move-only value that cannot be re-created has no correct retry loop — + /// it is unrecoverable after a failed pass. Use blocking push(). + /// @param value Element to enqueue, taken by value. An rvalue argument is + /// moved from at the call — including when the push fails, in which case + /// the value is discarded. An lvalue argument is copied and left intact. + /// @return false if the queue is full or closed; closed() tells them apart, + /// and a retry loop needs it to terminate. [[nodiscard]] bool try_push(T value); /// Dequeues into out, spinning while the queue is empty and open. diff --git a/include/cq/mutex_queue.hpp b/include/cq/mutex_queue.hpp index 920283b..d3bef24 100644 --- a/include/cq/mutex_queue.hpp +++ b/include/cq/mutex_queue.hpp @@ -19,15 +19,22 @@ namespace cq { /// Thread-safety: after construction, all member functions may be called /// concurrently from any number of producer and consumer threads. closed() /// and size() return advisory snapshots — drive control flow off the -/// push/pop return values instead. +/// push/pop return values instead, with one exception: try_push()/try_pop() +/// return false for "not now" and for "never again" alike, so a non-blocking +/// retry loop needs closed() to terminate. See try_push() for how such a loop +/// must handle its argument. /// /// Lifetime: the queue must outlive every thread using it — call close() /// and join all producers/consumers before destruction. Destroying the /// queue while a thread is blocked in push()/pop() is undefined behavior. /// -/// Exceptions: if T's move assignment throws, the failing push()/try_push() -/// enqueues nothing and the failing pop()/try_pop() leaves the element -/// queued — the queue itself stays consistent. +/// Exceptions: if T's move assignment throws, the queue's own invariants hold +/// — its indices do not move, so nothing is lost or duplicated and the element +/// count still reconciles. Element *values* are not protected: a failing +/// push()/try_push() enqueues nothing, but a failing pop()/try_pop() leaves +/// both out and the still-queued element in valid-but-unspecified states, so +/// retrying the pop yields a hollowed element rather than the original. None +/// of this is reachable for a T whose move assignment is noexcept. /// /// @tparam T Element type. Must be DefaultConstructible (ring slots are /// constructed up front) and MoveAssignable. @@ -46,13 +53,29 @@ class MutexQueue { MutexQueue& operator=(MutexQueue&&) = delete; /// Enqueues a value, blocking while the queue is full. - /// @param value Element to enqueue; consumed even when the push fails. + /// @param value Element to enqueue, taken by value. An rvalue argument is + /// moved from at the call — including when the push fails, in which case + /// the value is discarded. An lvalue argument is copied and left intact. /// @return false if the queue is closed (the value is dropped). [[nodiscard]] bool push(T value); /// Enqueues a value without blocking. - /// @param value Element to enqueue; consumed even when the push fails. - /// @return false if the queue is full or closed. + /// + /// A retry loop must re-materialise its argument every pass: this is a + /// by-value sink, so a failed attempt has already consumed an rvalue and + /// retrying with the same object pushes a moved-from husk, silently. + /// + /// while (!q.try_push(make_value())) { // NOT try_push(std::move(v)) + /// if (q.closed()) break; + /// } + /// + /// A move-only value that cannot be re-created has no correct retry loop — + /// it is unrecoverable after a failed pass. Use blocking push(). + /// @param value Element to enqueue, taken by value. An rvalue argument is + /// moved from at the call — including when the push fails, in which case + /// the value is discarded. An lvalue argument is copied and left intact. + /// @return false if the queue is full or closed; closed() tells them apart, + /// and a retry loop needs it to terminate. [[nodiscard]] bool try_push(T value); /// Dequeues into out, blocking while the queue is empty and open. @@ -71,7 +94,9 @@ class MutexQueue { /// indefinitely, and try_push(), which does not wait at all. /// @tparam Rep Arithmetic type of the timeout's tick count. /// @tparam Period std::ratio giving the timeout's tick period. - /// @param value Element to enqueue; consumed even when the push fails. + /// @param value Element to enqueue, taken by value. An rvalue argument is + /// moved from at the call — including when the push fails, in which case + /// the value is discarded. An lvalue argument is copied and left intact. /// @param timeout Longest time to wait. A non-positive timeout makes this /// equivalent to try_push(). /// @return false if the timeout elapsed with the queue still full, or if diff --git a/include/cq/spsc_queue.hpp b/include/cq/spsc_queue.hpp index 52d7ae5..dcd6f01 100644 --- a/include/cq/spsc_queue.hpp +++ b/include/cq/spsc_queue.hpp @@ -23,15 +23,22 @@ namespace cq { /// try_push) and at most one thread the consumer side (pop, try_pop), /// concurrently with each other. close(), closed(), size(), and capacity() /// may be called from any thread. closed() and size() return advisory -/// snapshots — drive control flow off the push/pop return values instead. +/// snapshots — drive control flow off the push/pop return values instead, +/// with one exception: try_push()/try_pop() return false for "not now" and +/// for "never again" alike, so a non-blocking retry loop needs closed() to +/// terminate. See try_push() for how such a loop must handle its argument. /// /// Lifetime: the queue must outlive both threads using it — call close() and /// join the producer/consumer before destruction. Destroying the queue while /// a thread is spinning in push()/pop() is undefined behavior. /// -/// Exceptions: if T's move assignment throws, the failing push()/try_push() -/// enqueues nothing and the failing pop()/try_pop() leaves the element -/// queued — the queue itself stays consistent. +/// Exceptions: if T's move assignment throws, the queue's own invariants hold +/// — its indices do not move, so nothing is lost or duplicated and the element +/// count still reconciles. Element *values* are not protected: a failing +/// push()/try_push() enqueues nothing, but a failing pop()/try_pop() leaves +/// both out and the still-queued element in valid-but-unspecified states, so +/// retrying the pop yields a hollowed element rather than the original. None +/// of this is reachable for a T whose move assignment is noexcept. /// /// @tparam T Element type. Must be DefaultConstructible (ring slots are /// constructed up front) and MoveAssignable. @@ -56,13 +63,29 @@ class SpscQueue { /// Enqueues a value, waiting while the queue is full (brief spin, then a /// timed sleep — near-zero CPU while blocked). Producer side. - /// @param value Element to enqueue; consumed even when the push fails. + /// @param value Element to enqueue, taken by value. An rvalue argument is + /// moved from at the call — including when the push fails, in which case + /// the value is discarded. An lvalue argument is copied and left intact. /// @return false if the queue is closed (the value is dropped). [[nodiscard]] bool push(T value); /// Enqueues a value without blocking. Producer side. - /// @param value Element to enqueue; consumed even when the push fails. - /// @return false if the queue is full or closed. + /// + /// A retry loop must re-materialise its argument every pass: this is a + /// by-value sink, so a failed attempt has already consumed an rvalue and + /// retrying with the same object pushes a moved-from husk, silently. + /// + /// while (!q.try_push(make_value())) { // NOT try_push(std::move(v)) + /// if (q.closed()) break; + /// } + /// + /// A move-only value that cannot be re-created has no correct retry loop — + /// it is unrecoverable after a failed pass. Use blocking push(). + /// @param value Element to enqueue, taken by value. An rvalue argument is + /// moved from at the call — including when the push fails, in which case + /// the value is discarded. An lvalue argument is copied and left intact. + /// @return false if the queue is full or closed; closed() tells them apart, + /// and a retry loop needs it to terminate. [[nodiscard]] bool try_push(T value); /// Dequeues into out, waiting while the queue is empty and open (brief diff --git a/tests/queue_contract_test.cpp b/tests/queue_contract_test.cpp index 6792d04..c9d646c 100644 --- a/tests/queue_contract_test.cpp +++ b/tests/queue_contract_test.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include @@ -39,6 +40,7 @@ class QueueContract : public ::testing::Test { protected: using IntQueue = typename Family::template Queue; using MoveOnlyQueue = typename Family::template Queue>; + using StringQueue = typename Family::template Queue; }; struct FamilyNames { @@ -183,5 +185,156 @@ TYPED_TEST(QueueContract, CloseWakesBlockedPush) { [&] { q.close(); })); } +// try_push()/try_pop() return false for "full/empty, retry" and for "closed, +// never" alike. closed() is the only discriminator, so a non-blocking loop +// that omits it never terminates. Pins the idiom the headers document. +TYPED_TEST(QueueContract, NonBlockingRetryLoopsTerminateViaClosed) { + typename TestFixture::IntQueue q(1); + ASSERT_TRUE(q.try_push(1)); + // Full but still open: try_push fails while closed() says "keep retrying". + // Without this the loops below break on their first pass and the test would + // pass even if closed() were stuck at true. + ASSERT_FALSE(q.try_push(2)); + ASSERT_FALSE(q.closed()) << "closed() must distinguish full-and-open from closed"; + q.close(); + + int spins = 0; + while (!q.try_push(2)) { + if (q.closed()) { + break; + } + ASSERT_LT(++spins, 1000) << "try_push loop never terminated"; + } + + int out = 0; + ASSERT_TRUE(q.try_pop(out)); // the pre-close element still drains + spins = 0; + while (!q.try_pop(out)) { + if (q.closed()) { + break; + } + ASSERT_LT(++spins, 1000) << "try_pop loop never terminated"; + } + EXPECT_EQ(q.size(), 0U); +} + +// push() takes T by value, so a failed push consumes an rvalue argument while +// leaving an lvalue one intact. That asymmetry is why a try_push retry loop +// must re-materialise its argument, and why move-only values that cannot be +// re-created have no correct retry loop at all. +TYPED_TEST(QueueContract, FailedPushConsumesRvaluesAndLeavesLvaluesIntact) { + typename TestFixture::StringQueue q(1); + ASSERT_TRUE(q.try_push("filler")); // now full: every push below fails + + const std::string lvalue = "still here"; + EXPECT_FALSE(q.try_push(lvalue)); + EXPECT_EQ(lvalue, "still here") << "an lvalue argument is copied, not consumed"; + + std::string rvalue = "consumed"; + EXPECT_FALSE(q.try_push(std::move(rvalue))); + // Reading a moved-from object is the assertion, not an accident. + // NOLINTNEXTLINE(bugprone-use-after-move) + EXPECT_TRUE(rvalue.empty()) << "an rvalue argument is moved from even on failure"; + + typename TestFixture::MoveOnlyQueue mq(1); + ASSERT_TRUE(mq.try_push(std::make_unique(1))); + auto owned = std::make_unique(2); + EXPECT_FALSE(mq.try_push(std::move(owned))); + // NOLINTNEXTLINE(bugprone-use-after-move) + EXPECT_EQ(owned, nullptr) << "retrying with the same object would push a husk"; +} + +// A throwing move assignment is the one place the three queues genuinely +// differ, so it gets its own suite. MutexQueue and SpscQueue survive it with +// their indices intact; MpmcQueue cannot — a throw strands a claimed ticket +// whose sequence is never re-published — which is why it static_asserts the +// requirement instead of appearing here. That static_assert is its test. +// +// Nothing else in the suite can reach this path: every other T here has a +// noexcept move assignment. The throw is armed by a counter rather than a flag +// on the element, so the element enqueues normally and turns hostile only for +// the dequeue. +int g_moves_until_throw = -1; // negative: never throw +constexpr int kStolenMarker = -999; // what a stolen-from value is left holding +constexpr int kSentinel = 99; // pre-loaded into out; the failed pop overwrites it + +// NOLINTBEGIN(misc-non-private-member-variables-in-classes) -- a two-field test +// payload; accessors would only obscure what the assertions check. +struct ThrowingMove { + int value = 0; + std::string label; + + ThrowingMove() = default; + ThrowingMove(int v, std::string l) : value(v), label(std::move(l)) {} + ThrowingMove(ThrowingMove&&) = default; + ThrowingMove(const ThrowingMove&) = delete; + ThrowingMove& operator=(const ThrowingMove&) = delete; + + // Throwing from a move assignment is the entire point of this type, so the + // two checks that forbid it are suppressed rather than satisfied. + // NOLINTNEXTLINE(performance-noexcept-move-constructor,bugprone-exception-escape) + ThrowingMove& operator=(ThrowingMove&& other) { + value = std::exchange(other.value, kStolenMarker); // first member stolen... + const bool armed = g_moves_until_throw == 0; + if (g_moves_until_throw >= 0) { + --g_moves_until_throw; + } + if (armed) { + throw std::runtime_error("move assignment failed"); // ...then this throws + } + label = std::move(other.label); + return *this; + } +}; +// NOLINTEND(misc-non-private-member-variables-in-classes) + +template +class ThrowingMoveContract : public ::testing::Test { + protected: + using Queue = typename Family::template Queue; +}; + +using SurvivingFamilies = ::testing::Types; +TYPED_TEST_SUITE(ThrowingMoveContract, SurvivingFamilies, FamilyNames); + +TYPED_TEST(ThrowingMoveContract, ThrowingPopKeepsInvariantsButNotElementValues) { + typename TestFixture::Queue q(4); + ASSERT_TRUE(q.try_push({1, "one"})); + ASSERT_TRUE(q.try_push({2, "two"})); + + ThrowingMove out{kSentinel, "sentinel"}; + g_moves_until_throw = 0; // the next move assignment is the pop's + EXPECT_THROW(static_cast(q.try_pop(out)), std::runtime_error); + g_moves_until_throw = -1; + + // Queue side: the indices never moved, so nothing was lost or duplicated. + EXPECT_EQ(q.size(), 2U); + + // Element side: the headers promise nothing, and indeed both the destination + // and the still-queued element were disturbed. + EXPECT_EQ(out.value, 1) << "out was modified despite the failure"; + EXPECT_EQ(out.label, "sentinel") << "the throw landed between the two members"; + + ThrowingMove retry; + ASSERT_TRUE(q.try_pop(retry)); + EXPECT_EQ(retry.value, kStolenMarker) << "retrying yields a hollowed element"; + EXPECT_EQ(retry.label, "one"); + + ThrowingMove neighbour; + ASSERT_TRUE(q.try_pop(neighbour)); + EXPECT_EQ(neighbour.value, 2) << "the next element is untouched"; + EXPECT_EQ(q.size(), 0U) << "the count still reconciles, which is why a checksum misses this"; +} + +TYPED_TEST(ThrowingMoveContract, ThrowingPushEnqueuesNothing) { + // The push side genuinely upholds its guarantee: the slot is written before + // the index advances, so a throw leaves the queue empty. + typename TestFixture::Queue q(2); + g_moves_until_throw = 0; + EXPECT_THROW(static_cast(q.try_push({7, "seven"})), std::runtime_error); + g_moves_until_throw = -1; + EXPECT_EQ(q.size(), 0U); +} + } // namespace } // namespace cq From 69168af4dca4d208ded6b554d514100ed9fd6aeb Mon Sep 17 00:00:00 2001 From: Debra Date: Fri, 28 Aug 2026 14:08:15 +0800 Subject: [PATCH 2/7] lib: scope the exception claim to the ops that can honour it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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) --- README.md | 36 +++++++++++++++++++++++++++++++++-- include/cq/mpmc_queue.hpp | 34 ++++++++++++++++----------------- include/cq/mutex_queue.hpp | 26 +++++++++++-------------- include/cq/spsc_queue.hpp | 33 ++++++++++++++++---------------- include/cq/spsc_queue.ipp | 16 ++++++++++++++++ tests/queue_contract_test.cpp | 8 ++++++-- 6 files changed, 100 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index ae14c7b..c709333 100644 --- a/README.md +++ b/README.md @@ -63,7 +63,7 @@ between them on threading, not on behaviour. | **Loss** | Nothing accepted is dropped, duplicated, or reordered | | **Element type** | Any `T` that is `DefaultConstructible` and `MoveAssignable` — plus `noexcept` move assignment for `MpmcQueue`, which `static_assert`s it | | **Lifetime** | The queue must outlive every thread using it | -| **Non-blocking loops** | `try_push`/`try_pop` return `false` for "not now" and "never again" alike; `closed()` is what lets a retry loop terminate | +| **Non-blocking loops** | `try_push`/`try_pop` return `false` for "not now" and "never again" alike; `closed()` is what lets a retry loop terminate — [see below](#non-blocking-loops) | | **A throwing move** | The one place the three differ — see below | Every operation reports whether it succeeded, and every one is `[[nodiscard]]`. @@ -78,6 +78,38 @@ sequence is never re-published, and every later operation on that slot spins forever. It therefore refuses such a `T` at compile time rather than degrading silently. +### Non-blocking loops + +`try_push`/`try_pop` return `false` for "not now" and for "never again" alike, +so a loop that only tests the return value never terminates after `close()`. +`closed()` is the discriminator, and both sides have a trap. + +**Producer.** `try_push` is a by-value sink, so a failed attempt has already +consumed an rvalue argument. Re-materialise it every pass: + +```cpp +while (!q.try_push(make_value())) { // NOT try_push(std::move(v)) + if (q.closed()) break; +} +``` + +A move-only value that cannot be re-created has no correct `try_push` loop at +all — after one failed attempt it is gone. Blocking `push()` narrows the window +to "closed" but does not close it: it too drops the value it was given. + +**Consumer.** Observing `closed()` is not the same as the queue being empty; a +producer may have pushed between the failed `try_pop` and the check. Re-attempt +once after observing it, which is what blocking `pop()` does internally: + +```cpp +while (!q.try_pop(out)) { + if (q.closed() && !q.try_pop(out)) break; +} +``` + +Or stop the producers before calling `close()` — the precondition `SpscQueue` +and `MpmcQueue` already state, and which `MutexQueue` allows you to skip. + ## The three queues They differ on one axis — **how many threads may touch each side** — and read @@ -90,7 +122,7 @@ as a sequence of trades. | Exceeding that | — | **undefined behaviour, no diagnostic** | — | | Bounded wait (`try_push_for`) | yes | no | no | | Bulk ops (`try_push_n` / `try_pop_n`) | no | yes | no | -| Throwing move assignment | survivable | survivable | rejected at compile time | +| Throwing move assignment | survivable | survivable (single-element ops; bulk ops reject it) | rejected at compile time | | A blocked thread | sleeps | spins briefly, then sleeps | spins briefly, then sleeps | | Built from | mutex + condition variables | atomics only | atomics only | diff --git a/include/cq/mpmc_queue.hpp b/include/cq/mpmc_queue.hpp index b1772a3..3f5146e 100644 --- a/include/cq/mpmc_queue.hpp +++ b/include/cq/mpmc_queue.hpp @@ -31,14 +31,16 @@ namespace cq { /// while a thread is spinning in push()/pop() is undefined behavior. /// /// Exceptions: if T's move assignment throws while a slot is claimed, that -/// slot's sequence is never re-published and the queue degrades (later -/// operations on the slot spin); unlike the locked queue there is no way to -/// return a claimed ticket. Use element types whose move assignment cannot -/// throw — the static_assert below enforces that, because the damage is -/// silent and unrecoverable. +/// slot's sequence is never re-published, and the queue does not merely +/// degrade: the consumer side can never advance past the stranded ticket, and +/// the producer side stops as soon as the ring wraps back onto it. Unlike the +/// locked queue there is no way to return a claimed ticket. Use element types whose move assignment +/// cannot throw — the static_assert below enforces that, because the damage is silent and +/// unrecoverable. /// /// @tparam T Element type. Must be DefaultConstructible (ring slots are -/// constructed up front) and MoveAssignable. +/// constructed up front), MoveAssignable, and — unlike the other two queues +/// — nothrow-MoveAssignable; see Exceptions above. template // The "excessive padding" the analyzer flags is deliberate: each position // counter gets a private cache line (see cq/cache_line.hpp). @@ -67,21 +69,17 @@ class MpmcQueue { /// @param value Element to enqueue, taken by value. An rvalue argument is /// moved from at the call — including when the push fails, in which case /// the value is discarded. An lvalue argument is copied and left intact. - /// @return false if the queue is closed (the value is dropped). + /// @return false if the queue is closed. The value is dropped: blocking + /// push() narrows the window in which a move-only argument can be lost, + /// but does not close it. [[nodiscard]] bool push(T value); /// Enqueues a value without blocking. /// - /// A retry loop must re-materialise its argument every pass: this is a + /// A retry loop must re-materialise its argument every pass — this is a /// by-value sink, so a failed attempt has already consumed an rvalue and - /// retrying with the same object pushes a moved-from husk, silently. - /// - /// while (!q.try_push(make_value())) { // NOT try_push(std::move(v)) - /// if (q.closed()) break; - /// } - /// - /// A move-only value that cannot be re-created has no correct retry loop — - /// it is unrecoverable after a failed pass. Use blocking push(). + /// retrying with the same object pushes a moved-from husk. See the README's + /// "Non-blocking loops" section for the worked idiom and its limits. /// @param value Element to enqueue, taken by value. An rvalue argument is /// moved from at the call — including when the push fails, in which case /// the value is discarded. An lvalue argument is copied and left intact. @@ -95,8 +93,8 @@ class MpmcQueue { [[nodiscard]] bool pop(T& out); /// Dequeues into out without blocking. - /// @param[out] out Receives the dequeued element on success; untouched on - /// failure. + /// @param[out] out Receives the dequeued element on success; untouched on a + /// false return; disturbed if T's move assignment throws (see Exceptions). /// @return false if the queue is empty (including transiently, while a /// producer has claimed the next slot but not yet published it). [[nodiscard]] bool try_pop(T& out); diff --git a/include/cq/mutex_queue.hpp b/include/cq/mutex_queue.hpp index d3bef24..2d874f1 100644 --- a/include/cq/mutex_queue.hpp +++ b/include/cq/mutex_queue.hpp @@ -33,7 +33,7 @@ namespace cq { /// count still reconciles. Element *values* are not protected: a failing /// push()/try_push() enqueues nothing, but a failing pop()/try_pop() leaves /// both out and the still-queued element in valid-but-unspecified states, so -/// retrying the pop yields a hollowed element rather than the original. None +/// retrying the pop may yield a hollowed element rather than the original. None /// of this is reachable for a T whose move assignment is noexcept. /// /// @tparam T Element type. Must be DefaultConstructible (ring slots are @@ -56,21 +56,17 @@ class MutexQueue { /// @param value Element to enqueue, taken by value. An rvalue argument is /// moved from at the call — including when the push fails, in which case /// the value is discarded. An lvalue argument is copied and left intact. - /// @return false if the queue is closed (the value is dropped). + /// @return false if the queue is closed. The value is dropped: blocking + /// push() narrows the window in which a move-only argument can be lost, + /// but does not close it. [[nodiscard]] bool push(T value); /// Enqueues a value without blocking. /// - /// A retry loop must re-materialise its argument every pass: this is a + /// A retry loop must re-materialise its argument every pass — this is a /// by-value sink, so a failed attempt has already consumed an rvalue and - /// retrying with the same object pushes a moved-from husk, silently. - /// - /// while (!q.try_push(make_value())) { // NOT try_push(std::move(v)) - /// if (q.closed()) break; - /// } - /// - /// A move-only value that cannot be re-created has no correct retry loop — - /// it is unrecoverable after a failed pass. Use blocking push(). + /// retrying with the same object pushes a moved-from husk. See the README's + /// "Non-blocking loops" section for the worked idiom and its limits. /// @param value Element to enqueue, taken by value. An rvalue argument is /// moved from at the call — including when the push fails, in which case /// the value is discarded. An lvalue argument is copied and left intact. @@ -84,8 +80,8 @@ class MutexQueue { [[nodiscard]] bool pop(T& out); /// Dequeues into out without blocking. - /// @param[out] out Receives the dequeued element on success; untouched on - /// failure. + /// @param[out] out Receives the dequeued element on success; untouched on a + /// false return; disturbed if T's move assignment throws (see Exceptions). /// @return false if the queue is empty. [[nodiscard]] bool try_pop(T& out); @@ -110,8 +106,8 @@ class MutexQueue { /// and drains, or timeout elapses. /// @tparam Rep Arithmetic type of the timeout's tick count. /// @tparam Period std::ratio giving the timeout's tick period. - /// @param[out] out Receives the dequeued element on success; untouched on - /// failure. + /// @param[out] out Receives the dequeued element on success; untouched on a + /// false return; disturbed if T's move assignment throws (see Exceptions). /// @param timeout Longest time to wait. A non-positive timeout makes this /// equivalent to try_pop(). /// @return false if the timeout elapsed with the queue still empty, or diff --git a/include/cq/spsc_queue.hpp b/include/cq/spsc_queue.hpp index dcd6f01..884254d 100644 --- a/include/cq/spsc_queue.hpp +++ b/include/cq/spsc_queue.hpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include "cq/backoff.hpp" @@ -32,12 +33,16 @@ namespace cq { /// join the producer/consumer before destruction. Destroying the queue while /// a thread is spinning in push()/pop() is undefined behavior. /// -/// Exceptions: if T's move assignment throws, the queue's own invariants hold -/// — its indices do not move, so nothing is lost or duplicated and the element -/// count still reconciles. Element *values* are not protected: a failing +/// Exceptions: for the single-element operations, if T's move assignment +/// throws the queue's own invariants hold — its indices do not move, so +/// nothing is lost or duplicated and the element count still reconciles. +/// try_push_n()/try_pop_n() cannot offer that: they publish one index for the +/// whole batch, so a throw part-way through would strand the moved elements +/// outside both the span and the queue, or inside both. They therefore +/// static_assert a noexcept move assignment. Element *values* are not protected: a failing /// push()/try_push() enqueues nothing, but a failing pop()/try_pop() leaves /// both out and the still-queued element in valid-but-unspecified states, so -/// retrying the pop yields a hollowed element rather than the original. None +/// retrying the pop may yield a hollowed element rather than the original. None /// of this is reachable for a T whose move assignment is noexcept. /// /// @tparam T Element type. Must be DefaultConstructible (ring slots are @@ -66,21 +71,17 @@ class SpscQueue { /// @param value Element to enqueue, taken by value. An rvalue argument is /// moved from at the call — including when the push fails, in which case /// the value is discarded. An lvalue argument is copied and left intact. - /// @return false if the queue is closed (the value is dropped). + /// @return false if the queue is closed. The value is dropped: blocking + /// push() narrows the window in which a move-only argument can be lost, + /// but does not close it. [[nodiscard]] bool push(T value); /// Enqueues a value without blocking. Producer side. /// - /// A retry loop must re-materialise its argument every pass: this is a + /// A retry loop must re-materialise its argument every pass — this is a /// by-value sink, so a failed attempt has already consumed an rvalue and - /// retrying with the same object pushes a moved-from husk, silently. - /// - /// while (!q.try_push(make_value())) { // NOT try_push(std::move(v)) - /// if (q.closed()) break; - /// } - /// - /// A move-only value that cannot be re-created has no correct retry loop — - /// it is unrecoverable after a failed pass. Use blocking push(). + /// retrying with the same object pushes a moved-from husk. See the README's + /// "Non-blocking loops" section for the worked idiom and its limits. /// @param value Element to enqueue, taken by value. An rvalue argument is /// moved from at the call — including when the push fails, in which case /// the value is discarded. An lvalue argument is copied and left intact. @@ -95,8 +96,8 @@ class SpscQueue { [[nodiscard]] bool pop(T& out); /// Dequeues into out without blocking. Consumer side. - /// @param[out] out Receives the dequeued element on success; untouched on - /// failure. + /// @param[out] out Receives the dequeued element on success; untouched on a + /// false return; disturbed if T's move assignment throws (see Exceptions). /// @return false if the queue is empty. [[nodiscard]] bool try_pop(T& out); diff --git a/include/cq/spsc_queue.ipp b/include/cq/spsc_queue.ipp index fa1d1f0..73f20c7 100644 --- a/include/cq/spsc_queue.ipp +++ b/include/cq/spsc_queue.ipp @@ -138,6 +138,14 @@ void SpscQueue::dequeue(std::size_t head, T& out) { template std::size_t SpscQueue::try_push_n(std::span items) { + // Bulk publishes one index for the whole batch, so a throw part-way through + // would leave the moved elements outside both the caller's span and the + // queue (push) or inside both (pop) — the only data loss or duplication + // anywhere in this library. The single-element ops survive a throw and do + // not carry this requirement. + static_assert(std::is_nothrow_move_assignable_v, + "SpscQueue bulk transfer requires a T whose move assignment is noexcept: " + "a throw mid-batch loses or duplicates elements"); if (items.empty() || closed_.load(std::memory_order_relaxed)) { return 0; } @@ -162,6 +170,14 @@ std::size_t SpscQueue::try_push_n(std::span items) { template std::size_t SpscQueue::try_pop_n(std::span out) { + // Bulk publishes one index for the whole batch, so a throw part-way through + // would leave the moved elements outside both the caller's span and the + // queue (push) or inside both (pop) — the only data loss or duplication + // anywhere in this library. The single-element ops survive a throw and do + // not carry this requirement. + static_assert(std::is_nothrow_move_assignable_v, + "SpscQueue bulk transfer requires a T whose move assignment is noexcept: " + "a throw mid-batch loses or duplicates elements"); if (out.empty()) { return 0; } diff --git a/tests/queue_contract_test.cpp b/tests/queue_contract_test.cpp index c9d646c..ced77c3 100644 --- a/tests/queue_contract_test.cpp +++ b/tests/queue_contract_test.cpp @@ -206,15 +206,19 @@ TYPED_TEST(QueueContract, NonBlockingRetryLoopsTerminateViaClosed) { ASSERT_LT(++spins, 1000) << "try_push loop never terminated"; } + // The consumer idiom re-attempts after observing closed(): a producer may + // have pushed between the failed try_pop and the check, and breaking on + // closed() alone strands that element. Verified separately: the naive form + // exits with size() == 1. int out = 0; - ASSERT_TRUE(q.try_pop(out)); // the pre-close element still drains spins = 0; while (!q.try_pop(out)) { - if (q.closed()) { + if (q.closed() && !q.try_pop(out)) { break; } ASSERT_LT(++spins, 1000) << "try_pop loop never terminated"; } + EXPECT_EQ(out, 1) << "the pre-close element must still drain"; EXPECT_EQ(q.size(), 0U); } From 572dd3192549046e912adac9be3ab6c004399969 Mon Sep 17 00:00:00 2001 From: Debra Date: Fri, 28 Aug 2026 14:28:48 +0800 Subject: [PATCH 3/7] docs: fix the consumer idiom this branch introduced, which lost elements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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) --- README.md | 34 ++++++++++++++++++++++++++-------- include/cq/spsc_queue.hpp | 3 ++- include/cq/spsc_queue.ipp | 25 +++++++++++-------------- tests/queue_contract_test.cpp | 28 ++++++++++++++++++---------- 4 files changed, 57 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index c709333..dac4302 100644 --- a/README.md +++ b/README.md @@ -61,7 +61,7 @@ between them on threading, not on behaviour. | **Shutdown** | `close()` refuses new items and wakes every waiter | | **Draining** | Items already queued still come out after `close()` | | **Loss** | Nothing accepted is dropped, duplicated, or reordered | -| **Element type** | Any `T` that is `DefaultConstructible` and `MoveAssignable` — plus `noexcept` move assignment for `MpmcQueue`, which `static_assert`s it | +| **Element type** | Any `T` that is `DefaultConstructible` and `MoveAssignable` — plus `noexcept` move assignment for `MpmcQueue` and for `SpscQueue`'s bulk ops, both of which `static_assert` it | | **Lifetime** | The queue must outlive every thread using it | | **Non-blocking loops** | `try_push`/`try_pop` return `false` for "not now" and "never again" alike; `closed()` is what lets a retry loop terminate — [see below](#non-blocking-loops) | | **A throwing move** | The one place the three differ — see below | @@ -72,7 +72,10 @@ Every operation reports whether it succeeded, and every one is `[[nodiscard]]`. `MutexQueue` and `SpscQueue` keep their indices intact — nothing is lost or duplicated, and the count still reconciles — but the element values are not protected: a failed pop leaves both the destination and the still-queued -element in valid-but-unspecified states, so retrying yields a hollowed element. +element in valid-but-unspecified states, so retrying may yield a hollowed +element. `SpscQueue`'s bulk `try_push_n`/`try_pop_n` are the exception: they +publish one index per batch, so they reject a throwing `T` at compile time +rather than lose or duplicate elements mid-batch. `MpmcQueue` cannot survive it at all: a throw strands a claimed ticket whose sequence is never re-published, and every later operation on that slot spins forever. It therefore refuses such a `T` at compile time rather than degrading @@ -97,18 +100,33 @@ A move-only value that cannot be re-created has no correct `try_push` loop at all — after one failed attempt it is gone. Blocking `push()` narrows the window to "closed" but does not close it: it too drops the value it was given. -**Consumer.** Observing `closed()` is not the same as the queue being empty; a +**Consumer.** Observing `closed()` is not the same as the queue being empty: a producer may have pushed between the failed `try_pop` and the check. Re-attempt -once after observing it, which is what blocking `pop()` does internally: +once after observing it, and take whatever that attempt gives — which is what +blocking `pop()` does (`if (closed()) return try_pop(out);`): ```cpp -while (!q.try_pop(out)) { - if (q.closed() && !q.try_pop(out)) break; +for (T item;;) { + if (!q.try_pop(item)) { + if (!q.closed()) continue; // not now — keep trying + if (!q.try_pop(item)) break; // closed and drained + } + use(item); } ``` -Or stop the producers before calling `close()` — the precondition `SpscQueue` -and `MpmcQueue` already state, and which `MutexQueue` allows you to skip. +The re-attempt is load-bearing, not defensive. Breaking on `closed()` alone +drops whatever arrived in the window; so does a form that only breaks when the +re-attempt *fails*, because the successful re-attempt's element is then +overwritten by the next loop condition. Measured against a producer that +pushes twice and closes: those two forms lost an element in 91 of 400 trials, +the loop above in none. + +For `SpscQueue` and `MpmcQueue` the re-attempt is also what orders you after +the producer's last push — their `close()` documents that producers must stop +first, and the re-attempt is how a consumer observes that they have. Under +`MutexQueue` both reads happen under one lock, so "closed and drained" has no +window and the re-attempt is merely harmless. ## The three queues diff --git a/include/cq/spsc_queue.hpp b/include/cq/spsc_queue.hpp index 884254d..6c62fda 100644 --- a/include/cq/spsc_queue.hpp +++ b/include/cq/spsc_queue.hpp @@ -46,7 +46,8 @@ namespace cq { /// of this is reachable for a T whose move assignment is noexcept. /// /// @tparam T Element type. Must be DefaultConstructible (ring slots are -/// constructed up front) and MoveAssignable. +/// constructed up front) and MoveAssignable; try_push_n()/try_pop_n() +/// additionally require a noexcept move assignment and static_assert it. template // The "excessive padding" the analyzer flags is deliberate: head_ and tail_ // each get a private cache line (see cq/cache_line.hpp). diff --git a/include/cq/spsc_queue.ipp b/include/cq/spsc_queue.ipp index 73f20c7..0c77100 100644 --- a/include/cq/spsc_queue.ipp +++ b/include/cq/spsc_queue.ipp @@ -10,10 +10,19 @@ #include #include #include +#include #include namespace cq { +// Both try_push_n and try_pop_n open with the same static_assert. Bulk +// publishes one index for the whole batch, so a throw part-way through would +// leave the moved elements outside both the caller's span and the queue (push) +// or inside both (pop) — the only data loss or duplication anywhere in this +// library. The single-element ops survive a throw and carry no such +// requirement, which is why the check sits on those two bodies and not on the +// class. The message cannot be factored out: C++20 requires a string literal. + template SpscQueue::SpscQueue(std::size_t capacity) : buffer_(ring_slots(capacity)) {} @@ -138,14 +147,8 @@ void SpscQueue::dequeue(std::size_t head, T& out) { template std::size_t SpscQueue::try_push_n(std::span items) { - // Bulk publishes one index for the whole batch, so a throw part-way through - // would leave the moved elements outside both the caller's span and the - // queue (push) or inside both (pop) — the only data loss or duplication - // anywhere in this library. The single-element ops survive a throw and do - // not carry this requirement. static_assert(std::is_nothrow_move_assignable_v, - "SpscQueue bulk transfer requires a T whose move assignment is noexcept: " - "a throw mid-batch loses or duplicates elements"); + "SpscQueue bulk transfer requires a noexcept move assignment"); if (items.empty() || closed_.load(std::memory_order_relaxed)) { return 0; } @@ -170,14 +173,8 @@ std::size_t SpscQueue::try_push_n(std::span items) { template std::size_t SpscQueue::try_pop_n(std::span out) { - // Bulk publishes one index for the whole batch, so a throw part-way through - // would leave the moved elements outside both the caller's span and the - // queue (push) or inside both (pop) — the only data loss or duplication - // anywhere in this library. The single-element ops survive a throw and do - // not carry this requirement. static_assert(std::is_nothrow_move_assignable_v, - "SpscQueue bulk transfer requires a T whose move assignment is noexcept: " - "a throw mid-batch loses or duplicates elements"); + "SpscQueue bulk transfer requires a noexcept move assignment"); if (out.empty()) { return 0; } diff --git a/tests/queue_contract_test.cpp b/tests/queue_contract_test.cpp index ced77c3..e01e3b6 100644 --- a/tests/queue_contract_test.cpp +++ b/tests/queue_contract_test.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include @@ -206,19 +207,26 @@ TYPED_TEST(QueueContract, NonBlockingRetryLoopsTerminateViaClosed) { ASSERT_LT(++spins, 1000) << "try_push loop never terminated"; } - // The consumer idiom re-attempts after observing closed(): a producer may - // have pushed between the failed try_pop and the check, and breaking on - // closed() alone strands that element. Verified separately: the naive form - // exits with size() == 1. - int out = 0; + // The consumer drain loop from the README. The re-attempt after closed() is + // load-bearing twice over: breaking on closed() alone strands an element a + // producer pushed in the window, and breaking only when the re-attempt fails + // discards the one it just retrieved. Reaching the break here requires + // consulting closed(), so a closed() stuck at false trips the spin guard. + std::vector drained; spins = 0; - while (!q.try_pop(out)) { - if (q.closed() && !q.try_pop(out)) { - break; + for (int item = 0;;) { + if (!q.try_pop(item)) { + if (!q.closed()) { + ASSERT_LT(++spins, 1000) << "try_pop loop never terminated"; + continue; + } + if (!q.try_pop(item)) { + break; + } } - ASSERT_LT(++spins, 1000) << "try_pop loop never terminated"; + drained.push_back(item); } - EXPECT_EQ(out, 1) << "the pre-close element must still drain"; + EXPECT_EQ(drained, std::vector{1}) << "the pre-close element must still drain"; EXPECT_EQ(q.size(), 0U); } From 8dd626f62c1293415ab4880ba61cfdcd4dd985fa Mon Sep 17 00:00:00 2001 From: Debra Date: Fri, 28 Aug 2026 14:46:24 +0800 Subject: [PATCH 4/7] docs: the re-attempt matters most on the queue this said it least matters on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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) --- README.md | 38 +++++++++++++++++++++++------------ include/cq/mpmc_queue.hpp | 7 ++++--- include/cq/spsc_queue.hpp | 15 ++++++++------ tests/queue_contract_test.cpp | 5 ++++- 4 files changed, 42 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index dac4302..9813d3e 100644 --- a/README.md +++ b/README.md @@ -103,7 +103,9 @@ to "closed" but does not close it: it too drops the value it was given. **Consumer.** Observing `closed()` is not the same as the queue being empty: a producer may have pushed between the failed `try_pop` and the check. Re-attempt once after observing it, and take whatever that attempt gives — which is what -blocking `pop()` does (`if (closed()) return try_pop(out);`): +`SpscQueue::pop`/`MpmcQueue::pop` do verbatim (`if (closed()) return +try_pop(out);`). `MutexQueue::pop` reaches the same result differently, by +resolving "closed and drained" under a single lock: ```cpp for (T item;;) { @@ -115,18 +117,28 @@ for (T item;;) { } ``` -The re-attempt is load-bearing, not defensive. Breaking on `closed()` alone -drops whatever arrived in the window; so does a form that only breaks when the -re-attempt *fails*, because the successful re-attempt's element is then -overwritten by the next loop condition. Measured against a producer that -pushes twice and closes: those two forms lost an element in 91 of 400 trials, -the loop above in none. - -For `SpscQueue` and `MpmcQueue` the re-attempt is also what orders you after -the producer's last push — their `close()` documents that producers must stop -first, and the re-attempt is how a consumer observes that they have. Under -`MutexQueue` both reads happen under one lock, so "closed and drained" has no -window and the re-attempt is merely harmless. +The re-attempt is load-bearing on all three, not defensive. `try_pop`, +`closed()` and the second `try_pop` are three separate acquisitions — no lock +or ordering spans them — so breaking on `closed()` alone strands whatever +arrived in the window, and a form that breaks only when the re-attempt *fails* +is worse still: the element that attempt just retrieved is overwritten by the +next loop condition. + +Measured against a producer that pushes twice and then closes, per 400 trials: + +| | break on `closed()` alone | the loop above | +|---|---|---| +| `MutexQueue` | 139–178 lost | 0 | +| `SpscQueue` | 4–10 lost | 0 | +| `MpmcQueue` | 2–6 lost | 0 | + +`MutexQueue` has the widest window by roughly thirty times, not the narrowest: +the lock hand-off after its failed `try_pop` is long enough for the producer to +complete both pushes *and* the close before the consumer reacquires. What it +genuinely lacks is a different hazard — a push racing `close()` — which is why +its `close()` carries no stop-producers-first precondition while `SpscQueue`'s +and `MpmcQueue`'s do. For those two, the re-attempt is also what orders the +consumer after the producer's last push. ## The three queues diff --git a/include/cq/mpmc_queue.hpp b/include/cq/mpmc_queue.hpp index 3f5146e..84bbe65 100644 --- a/include/cq/mpmc_queue.hpp +++ b/include/cq/mpmc_queue.hpp @@ -39,8 +39,8 @@ namespace cq { /// unrecoverable. /// /// @tparam T Element type. Must be DefaultConstructible (ring slots are -/// constructed up front), MoveAssignable, and — unlike the other two queues -/// — nothrow-MoveAssignable; see Exceptions above. +/// constructed up front), MoveAssignable, and — unlike the other two +/// queues — nothrow-MoveAssignable; see Exceptions above. template // The "excessive padding" the analyzer flags is deliberate: each position // counter gets a private cache line (see cq/cache_line.hpp). @@ -48,7 +48,8 @@ template class MpmcQueue { // Prose cannot enforce this and the failure is silent: a throwing move // assignment leaves a claimed ticket whose sequence is never re-published, - // so every later operation on that slot spins forever. + // so try_push()/try_pop() report that slot full or empty forever and the + // blocking push()/pop() spin on it. static_assert(std::is_nothrow_move_assignable_v, "MpmcQueue requires a T whose move assignment is noexcept: a throw would " "strand a claimed slot and permanently degrade the queue"); diff --git a/include/cq/spsc_queue.hpp b/include/cq/spsc_queue.hpp index 6c62fda..cde6859 100644 --- a/include/cq/spsc_queue.hpp +++ b/include/cq/spsc_queue.hpp @@ -4,7 +4,6 @@ #include #include #include -#include #include #include "cq/backoff.hpp" @@ -36,11 +35,15 @@ namespace cq { /// Exceptions: for the single-element operations, if T's move assignment /// throws the queue's own invariants hold — its indices do not move, so /// nothing is lost or duplicated and the element count still reconciles. -/// try_push_n()/try_pop_n() cannot offer that: they publish one index for the -/// whole batch, so a throw part-way through would strand the moved elements -/// outside both the span and the queue, or inside both. They therefore -/// static_assert a noexcept move assignment. Element *values* are not protected: a failing -/// push()/try_push() enqueues nothing, but a failing pop()/try_pop() leaves +/// Element *values* are not protected: a failing pop()/try_pop() leaves both +/// out and the still-queued element in valid-but-unspecified states, so +/// retrying the pop may yield a hollowed element rather than the original. +/// +/// try_push_n()/try_pop_n() cannot offer even the first guarantee: they +/// publish one index for the whole batch, so a throw part-way through would +/// strand the moved elements outside both the span and the queue, or inside +/// both. They therefore static_assert a noexcept move assignment. Element *values* are not +/// protected: a failing push()/try_push() enqueues nothing, but a failing pop()/try_pop() leaves /// both out and the still-queued element in valid-but-unspecified states, so /// retrying the pop may yield a hollowed element rather than the original. None /// of this is reachable for a T whose move assignment is noexcept. diff --git a/tests/queue_contract_test.cpp b/tests/queue_contract_test.cpp index e01e3b6..2c8f60a 100644 --- a/tests/queue_contract_test.cpp +++ b/tests/queue_contract_test.cpp @@ -244,7 +244,10 @@ TYPED_TEST(QueueContract, FailedPushConsumesRvaluesAndLeavesLvaluesIntact) { std::string rvalue = "consumed"; EXPECT_FALSE(q.try_push(std::move(rvalue))); - // Reading a moved-from object is the assertion, not an accident. + // Reading a moved-from object is the assertion, not an accident. A + // moved-from std::string is only valid-but-unspecified by the standard, so + // this half is a libstdc++/libc++ observation; the unique_ptr case below is + // the one the standard actually guarantees. // NOLINTNEXTLINE(bugprone-use-after-move) EXPECT_TRUE(rvalue.empty()) << "an rvalue argument is moved from even on failure"; From 08ac784f0b7ce97c32304d95ae04d98690a2d38b Mon Sep 17 00:00:00 2001 From: Debra Date: Fri, 28 Aug 2026 15:02:32 +0800 Subject: [PATCH 5/7] docs: ship the correction in both places it was needed, not one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- README.md | 21 ++++++++++++--------- include/cq/mpmc_queue.hpp | 14 +++++++------- include/cq/spsc_queue.hpp | 17 ++++++++--------- 3 files changed, 27 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 9813d3e..07682ea 100644 --- a/README.md +++ b/README.md @@ -77,8 +77,9 @@ element. `SpscQueue`'s bulk `try_push_n`/`try_pop_n` are the exception: they publish one index per batch, so they reject a throwing `T` at compile time rather than lose or duplicate elements mid-batch. `MpmcQueue` cannot survive it at all: a throw strands a claimed ticket whose -sequence is never re-published, and every later operation on that slot spins -forever. It therefore refuses such a `T` at compile time rather than degrading +sequence is never re-published, so `try_push`/`try_pop` report that slot full +or empty forever and the blocking `push`/`pop` spin on it — the consumer never +advances past the ticket, and the producer stops once the ring wraps onto it. It therefore refuses such a `T` at compile time rather than degrading silently. ### Non-blocking loops @@ -118,13 +119,15 @@ for (T item;;) { ``` The re-attempt is load-bearing on all three, not defensive. `try_pop`, -`closed()` and the second `try_pop` are three separate acquisitions — no lock -or ordering spans them — so breaking on `closed()` alone strands whatever -arrived in the window, and a form that breaks only when the re-attempt *fails* -is worse still: the element that attempt just retrieved is overwritten by the -next loop condition. - -Measured against a producer that pushes twice and then closes, per 400 trials: +`closed()` and the second `try_pop` are three separate steps, and nothing makes +them one — so breaking on `closed()` alone strands whatever arrived in the +window, and a form that breaks only when the re-attempt *fails* is worse still: +the element that attempt just retrieved is overwritten by the next loop +condition. + +Measured on an Apple M2 Pro, Release, against a producer that pushes twice and +then closes, per 400 trials — the ratio between the rows is the point, not the +absolute counts, which move with the machine: | | break on `closed()` alone | the loop above | |---|---|---| diff --git a/include/cq/mpmc_queue.hpp b/include/cq/mpmc_queue.hpp index 84bbe65..d653d97 100644 --- a/include/cq/mpmc_queue.hpp +++ b/include/cq/mpmc_queue.hpp @@ -32,11 +32,11 @@ namespace cq { /// /// Exceptions: if T's move assignment throws while a slot is claimed, that /// slot's sequence is never re-published, and the queue does not merely -/// degrade: the consumer side can never advance past the stranded ticket, and -/// the producer side stops as soon as the ring wraps back onto it. Unlike the -/// locked queue there is no way to return a claimed ticket. Use element types whose move assignment -/// cannot throw — the static_assert below enforces that, because the damage is silent and -/// unrecoverable. +/// degrade: the consumer can never advance past the stranded ticket, and the +/// producer stops as soon as the ring wraps back onto it. Unlike the locked +/// queue there is no way to return a claimed ticket. Use element types +/// whose move assignment cannot throw — the static_assert below enforces +/// it, because the damage is silent and unrecoverable. /// /// @tparam T Element type. Must be DefaultConstructible (ring slots are /// constructed up front), MoveAssignable, and — unlike the other two @@ -48,8 +48,8 @@ template class MpmcQueue { // Prose cannot enforce this and the failure is silent: a throwing move // assignment leaves a claimed ticket whose sequence is never re-published, - // so try_push()/try_pop() report that slot full or empty forever and the - // blocking push()/pop() spin on it. + // so try_push()/try_pop() report that slot full or empty forever and + // the blocking push()/pop() spin on it. static_assert(std::is_nothrow_move_assignable_v, "MpmcQueue requires a T whose move assignment is noexcept: a throw would " "strand a claimed slot and permanently degrade the queue"); diff --git a/include/cq/spsc_queue.hpp b/include/cq/spsc_queue.hpp index cde6859..2798746 100644 --- a/include/cq/spsc_queue.hpp +++ b/include/cq/spsc_queue.hpp @@ -35,18 +35,17 @@ namespace cq { /// Exceptions: for the single-element operations, if T's move assignment /// throws the queue's own invariants hold — its indices do not move, so /// nothing is lost or duplicated and the element count still reconciles. -/// Element *values* are not protected: a failing pop()/try_pop() leaves both -/// out and the still-queued element in valid-but-unspecified states, so -/// retrying the pop may yield a hollowed element rather than the original. +/// Element *values* are not protected: a failing push()/try_push() enqueues +/// nothing, but a failing pop()/try_pop() leaves both out and the +/// still-queued element in valid-but-unspecified states, so retrying the pop +/// may yield a hollowed element rather than the original. None of this is +/// reachable for a T whose move assignment is noexcept. /// -/// try_push_n()/try_pop_n() cannot offer even the first guarantee: they +/// try_push_n()/try_pop_n() cannot offer even the index guarantee: they /// publish one index for the whole batch, so a throw part-way through would /// strand the moved elements outside both the span and the queue, or inside -/// both. They therefore static_assert a noexcept move assignment. Element *values* are not -/// protected: a failing push()/try_push() enqueues nothing, but a failing pop()/try_pop() leaves -/// both out and the still-queued element in valid-but-unspecified states, so -/// retrying the pop may yield a hollowed element rather than the original. None -/// of this is reachable for a T whose move assignment is noexcept. +/// both. They therefore static_assert a noexcept move assignment, which puts +/// the whole paragraph above out of their reach by construction. /// /// @tparam T Element type. Must be DefaultConstructible (ring slots are /// constructed up front) and MoveAssignable; try_push_n()/try_pop_n() From aaa07d3130e82dc5ac52e53ecd6b0ecbf43b9a05 Mon Sep 17 00:00:00 2001 From: Debra Date: Sat, 29 Aug 2026 12:04:34 +0800 Subject: [PATCH 6/7] =?UTF-8?q?docs:=20state=20each=20contract=20once=20?= =?UTF-8?q?=E2=80=94=20class=20note=20for=20by-value=20args,=20shorter=20e?= =?UTF-8?q?xception=20paragraphs,=20drop=20the=20unreproducible=20table?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 --- README.md | 53 +++++++++++------------------------ include/cq/mpmc_queue.hpp | 47 +++++++++++-------------------- include/cq/mutex_queue.hpp | 45 +++++++++++------------------ include/cq/spsc_queue.hpp | 47 +++++++++++-------------------- include/cq/spsc_queue.ipp | 10 ++----- tests/queue_contract_test.cpp | 19 ++++--------- 6 files changed, 74 insertions(+), 147 deletions(-) diff --git a/README.md b/README.md index 07682ea..4defa36 100644 --- a/README.md +++ b/README.md @@ -68,19 +68,13 @@ between them on threading, not on behaviour. Every operation reports whether it succeeded, and every one is `[[nodiscard]]`. -**If `T`'s move assignment can throw**, that is where the three part company. -`MutexQueue` and `SpscQueue` keep their indices intact — nothing is lost or -duplicated, and the count still reconciles — but the element values are not -protected: a failed pop leaves both the destination and the still-queued -element in valid-but-unspecified states, so retrying may yield a hollowed -element. `SpscQueue`'s bulk `try_push_n`/`try_pop_n` are the exception: they -publish one index per batch, so they reject a throwing `T` at compile time -rather than lose or duplicate elements mid-batch. -`MpmcQueue` cannot survive it at all: a throw strands a claimed ticket whose -sequence is never re-published, so `try_push`/`try_pop` report that slot full -or empty forever and the blocking `push`/`pop` spin on it — the consumer never -advances past the ticket, and the producer stops once the ring wraps onto it. It therefore refuses such a `T` at compile time rather than degrading -silently. +**If `T`'s move assignment can throw**, the three part company. `MutexQueue` +and `SpscQueue` keep their indices intact — nothing lost or duplicated — but a +failed pop leaves both the destination and the still-queued element +valid-but-unspecified. `SpscQueue`'s bulk ops publish one index per batch and +would lose or duplicate elements, so they reject a throwing `T` at compile +time. `MpmcQueue` cannot survive a throw at all — it strands a claimed ticket +and the queue stalls on it forever — so it rejects such a `T` at compile time. ### Non-blocking loops @@ -118,30 +112,15 @@ for (T item;;) { } ``` -The re-attempt is load-bearing on all three, not defensive. `try_pop`, -`closed()` and the second `try_pop` are three separate steps, and nothing makes -them one — so breaking on `closed()` alone strands whatever arrived in the -window, and a form that breaks only when the re-attempt *fails* is worse still: -the element that attempt just retrieved is overwritten by the next loop -condition. - -Measured on an Apple M2 Pro, Release, against a producer that pushes twice and -then closes, per 400 trials — the ratio between the rows is the point, not the -absolute counts, which move with the machine: - -| | break on `closed()` alone | the loop above | -|---|---|---| -| `MutexQueue` | 139–178 lost | 0 | -| `SpscQueue` | 4–10 lost | 0 | -| `MpmcQueue` | 2–6 lost | 0 | - -`MutexQueue` has the widest window by roughly thirty times, not the narrowest: -the lock hand-off after its failed `try_pop` is long enough for the producer to -complete both pushes *and* the close before the consumer reacquires. What it -genuinely lacks is a different hazard — a push racing `close()` — which is why -its `close()` carries no stop-producers-first precondition while `SpscQueue`'s -and `MpmcQueue`'s do. For those two, the re-attempt is also what orders the -consumer after the producer's last push. +The re-attempt is load-bearing on all three. `try_pop`, `closed()` and the +second `try_pop` are three separate steps — breaking on `closed()` alone +strands whatever arrived between the first two, and breaking only when the +re-attempt *fails* discards the element it just retrieved. `MutexQueue` is not +exempt: the lock hand-off after a failed `try_pop` is a wide window, not a +narrow one. What it lacks is a different hazard — a push racing `close()` — +which is why its `close()` has no stop-producers-first precondition while the +other two do. For those two, the re-attempt is also what orders the consumer +after the producer's last push. ## The three queues diff --git a/include/cq/mpmc_queue.hpp b/include/cq/mpmc_queue.hpp index d653d97..aca4608 100644 --- a/include/cq/mpmc_queue.hpp +++ b/include/cq/mpmc_queue.hpp @@ -23,33 +23,29 @@ namespace cq { /// advisory snapshots — drive control flow off the push/pop return values /// instead, with one exception: try_push()/try_pop() return false for "not /// now" and for "never again" alike, so a non-blocking retry loop needs -/// closed() to terminate. See try_push() for how such a loop must handle its -/// argument. +/// closed() to terminate. /// /// Lifetime: the queue must outlive every thread using it — call close() and /// join all producers/consumers before destruction. Destroying the queue /// while a thread is spinning in push()/pop() is undefined behavior. /// -/// Exceptions: if T's move assignment throws while a slot is claimed, that -/// slot's sequence is never re-published, and the queue does not merely -/// degrade: the consumer can never advance past the stranded ticket, and the -/// producer stops as soon as the ring wraps back onto it. Unlike the locked -/// queue there is no way to return a claimed ticket. Use element types -/// whose move assignment cannot throw — the static_assert below enforces -/// it, because the damage is silent and unrecoverable. +/// Exceptions: a throwing move assignment would strand a claimed ticket whose +/// sequence is never re-published — the consumer can never advance past it and +/// the producer stalls once the ring wraps onto it — with no way to return the +/// ticket. A static_assert therefore requires a noexcept move assignment. +/// +/// Arguments: push operations take T by value. An rvalue argument is moved +/// from at the call — even when the push fails, in which case the value is +/// discarded. An lvalue argument is copied and left intact. A try_push() +/// retry loop must therefore re-materialise its argument every pass. /// /// @tparam T Element type. Must be DefaultConstructible (ring slots are -/// constructed up front), MoveAssignable, and — unlike the other two -/// queues — nothrow-MoveAssignable; see Exceptions above. +/// constructed up front) and nothrow-MoveAssignable (see Exceptions). template // The "excessive padding" the analyzer flags is deliberate: each position // counter gets a private cache line (see cq/cache_line.hpp). // NOLINTNEXTLINE(clang-analyzer-optin.performance.Padding) class MpmcQueue { - // Prose cannot enforce this and the failure is silent: a throwing move - // assignment leaves a claimed ticket whose sequence is never re-published, - // so try_push()/try_pop() report that slot full or empty forever and - // the blocking push()/pop() spin on it. static_assert(std::is_nothrow_move_assignable_v, "MpmcQueue requires a T whose move assignment is noexcept: a throw would " "strand a claimed slot and permanently degrade the queue"); @@ -67,25 +63,14 @@ class MpmcQueue { MpmcQueue& operator=(MpmcQueue&&) = delete; /// Enqueues a value, spinning while the queue is full. - /// @param value Element to enqueue, taken by value. An rvalue argument is - /// moved from at the call — including when the push fails, in which case - /// the value is discarded. An lvalue argument is copied and left intact. - /// @return false if the queue is closed. The value is dropped: blocking - /// push() narrows the window in which a move-only argument can be lost, - /// but does not close it. + /// @param value Element to enqueue; see the class note on by-value arguments. + /// @return false if the queue is closed; the value is discarded. [[nodiscard]] bool push(T value); /// Enqueues a value without blocking. - /// - /// A retry loop must re-materialise its argument every pass — this is a - /// by-value sink, so a failed attempt has already consumed an rvalue and - /// retrying with the same object pushes a moved-from husk. See the README's - /// "Non-blocking loops" section for the worked idiom and its limits. - /// @param value Element to enqueue, taken by value. An rvalue argument is - /// moved from at the call — including when the push fails, in which case - /// the value is discarded. An lvalue argument is copied and left intact. - /// @return false if the queue is full or closed; closed() tells them apart, - /// and a retry loop needs it to terminate. + /// @param value Element to enqueue; see the class note on by-value arguments. + /// @return false if the queue is full or closed. closed() tells them apart; + /// see the README's "Non-blocking loops" for the retry idiom. [[nodiscard]] bool try_push(T value); /// Dequeues into out, spinning while the queue is empty and open. diff --git a/include/cq/mutex_queue.hpp b/include/cq/mutex_queue.hpp index 2d874f1..1813be1 100644 --- a/include/cq/mutex_queue.hpp +++ b/include/cq/mutex_queue.hpp @@ -21,20 +21,22 @@ namespace cq { /// and size() return advisory snapshots — drive control flow off the /// push/pop return values instead, with one exception: try_push()/try_pop() /// return false for "not now" and for "never again" alike, so a non-blocking -/// retry loop needs closed() to terminate. See try_push() for how such a loop -/// must handle its argument. +/// retry loop needs closed() to terminate. /// /// Lifetime: the queue must outlive every thread using it — call close() /// and join all producers/consumers before destruction. Destroying the /// queue while a thread is blocked in push()/pop() is undefined behavior. /// -/// Exceptions: if T's move assignment throws, the queue's own invariants hold -/// — its indices do not move, so nothing is lost or duplicated and the element -/// count still reconciles. Element *values* are not protected: a failing -/// push()/try_push() enqueues nothing, but a failing pop()/try_pop() leaves -/// both out and the still-queued element in valid-but-unspecified states, so -/// retrying the pop may yield a hollowed element rather than the original. None -/// of this is reachable for a T whose move assignment is noexcept. +/// Exceptions: if T's move assignment throws, the indices do not move — nothing +/// is lost or duplicated — but element values are not protected: a failed +/// push() enqueues nothing, and a failed pop() leaves both out and the +/// still-queued element valid-but-unspecified. Unreachable for a noexcept +/// move assignment. +/// +/// Arguments: push operations take T by value. An rvalue argument is moved +/// from at the call — even when the push fails, in which case the value is +/// discarded. An lvalue argument is copied and left intact. A try_push() +/// retry loop must therefore re-materialise its argument every pass. /// /// @tparam T Element type. Must be DefaultConstructible (ring slots are /// constructed up front) and MoveAssignable. @@ -53,25 +55,14 @@ class MutexQueue { MutexQueue& operator=(MutexQueue&&) = delete; /// Enqueues a value, blocking while the queue is full. - /// @param value Element to enqueue, taken by value. An rvalue argument is - /// moved from at the call — including when the push fails, in which case - /// the value is discarded. An lvalue argument is copied and left intact. - /// @return false if the queue is closed. The value is dropped: blocking - /// push() narrows the window in which a move-only argument can be lost, - /// but does not close it. + /// @param value Element to enqueue; see the class note on by-value arguments. + /// @return false if the queue is closed; the value is discarded. [[nodiscard]] bool push(T value); /// Enqueues a value without blocking. - /// - /// A retry loop must re-materialise its argument every pass — this is a - /// by-value sink, so a failed attempt has already consumed an rvalue and - /// retrying with the same object pushes a moved-from husk. See the README's - /// "Non-blocking loops" section for the worked idiom and its limits. - /// @param value Element to enqueue, taken by value. An rvalue argument is - /// moved from at the call — including when the push fails, in which case - /// the value is discarded. An lvalue argument is copied and left intact. - /// @return false if the queue is full or closed; closed() tells them apart, - /// and a retry loop needs it to terminate. + /// @param value Element to enqueue; see the class note on by-value arguments. + /// @return false if the queue is full or closed. closed() tells them apart; + /// see the README's "Non-blocking loops" for the retry idiom. [[nodiscard]] bool try_push(T value); /// Dequeues into out, blocking while the queue is empty and open. @@ -90,9 +81,7 @@ class MutexQueue { /// indefinitely, and try_push(), which does not wait at all. /// @tparam Rep Arithmetic type of the timeout's tick count. /// @tparam Period std::ratio giving the timeout's tick period. - /// @param value Element to enqueue, taken by value. An rvalue argument is - /// moved from at the call — including when the push fails, in which case - /// the value is discarded. An lvalue argument is copied and left intact. + /// @param value Element to enqueue; see the class note on by-value arguments. /// @param timeout Longest time to wait. A non-positive timeout makes this /// equivalent to try_push(). /// @return false if the timeout elapsed with the queue still full, or if diff --git a/include/cq/spsc_queue.hpp b/include/cq/spsc_queue.hpp index 2798746..2bbd445 100644 --- a/include/cq/spsc_queue.hpp +++ b/include/cq/spsc_queue.hpp @@ -26,26 +26,24 @@ namespace cq { /// snapshots — drive control flow off the push/pop return values instead, /// with one exception: try_push()/try_pop() return false for "not now" and /// for "never again" alike, so a non-blocking retry loop needs closed() to -/// terminate. See try_push() for how such a loop must handle its argument. +/// terminate. /// /// Lifetime: the queue must outlive both threads using it — call close() and /// join the producer/consumer before destruction. Destroying the queue while /// a thread is spinning in push()/pop() is undefined behavior. /// -/// Exceptions: for the single-element operations, if T's move assignment -/// throws the queue's own invariants hold — its indices do not move, so -/// nothing is lost or duplicated and the element count still reconciles. -/// Element *values* are not protected: a failing push()/try_push() enqueues -/// nothing, but a failing pop()/try_pop() leaves both out and the -/// still-queued element in valid-but-unspecified states, so retrying the pop -/// may yield a hollowed element rather than the original. None of this is -/// reachable for a T whose move assignment is noexcept. +/// Exceptions: for single-element operations, if T's move assignment throws +/// the indices do not move — nothing is lost or duplicated — but element +/// values are not protected: a failed push() enqueues nothing, and a failed +/// pop() leaves both out and the still-queued element valid-but-unspecified. +/// Unreachable for a noexcept move assignment. try_push_n()/try_pop_n() +/// publish one index per batch, so a throw part-way through would lose or +/// duplicate elements; they static_assert a noexcept move assignment instead. /// -/// try_push_n()/try_pop_n() cannot offer even the index guarantee: they -/// publish one index for the whole batch, so a throw part-way through would -/// strand the moved elements outside both the span and the queue, or inside -/// both. They therefore static_assert a noexcept move assignment, which puts -/// the whole paragraph above out of their reach by construction. +/// Arguments: push operations take T by value. An rvalue argument is moved +/// from at the call — even when the push fails, in which case the value is +/// discarded. An lvalue argument is copied and left intact. A try_push() +/// retry loop must therefore re-materialise its argument every pass. /// /// @tparam T Element type. Must be DefaultConstructible (ring slots are /// constructed up front) and MoveAssignable; try_push_n()/try_pop_n() @@ -71,25 +69,14 @@ class SpscQueue { /// Enqueues a value, waiting while the queue is full (brief spin, then a /// timed sleep — near-zero CPU while blocked). Producer side. - /// @param value Element to enqueue, taken by value. An rvalue argument is - /// moved from at the call — including when the push fails, in which case - /// the value is discarded. An lvalue argument is copied and left intact. - /// @return false if the queue is closed. The value is dropped: blocking - /// push() narrows the window in which a move-only argument can be lost, - /// but does not close it. + /// @param value Element to enqueue; see the class note on by-value arguments. + /// @return false if the queue is closed; the value is discarded. [[nodiscard]] bool push(T value); /// Enqueues a value without blocking. Producer side. - /// - /// A retry loop must re-materialise its argument every pass — this is a - /// by-value sink, so a failed attempt has already consumed an rvalue and - /// retrying with the same object pushes a moved-from husk. See the README's - /// "Non-blocking loops" section for the worked idiom and its limits. - /// @param value Element to enqueue, taken by value. An rvalue argument is - /// moved from at the call — including when the push fails, in which case - /// the value is discarded. An lvalue argument is copied and left intact. - /// @return false if the queue is full or closed; closed() tells them apart, - /// and a retry loop needs it to terminate. + /// @param value Element to enqueue; see the class note on by-value arguments. + /// @return false if the queue is full or closed. closed() tells them apart; + /// see the README's "Non-blocking loops" for the retry idiom. [[nodiscard]] bool try_push(T value); /// Dequeues into out, waiting while the queue is empty and open (brief diff --git a/include/cq/spsc_queue.ipp b/include/cq/spsc_queue.ipp index 0c77100..0f4e1f4 100644 --- a/include/cq/spsc_queue.ipp +++ b/include/cq/spsc_queue.ipp @@ -15,13 +15,9 @@ namespace cq { -// Both try_push_n and try_pop_n open with the same static_assert. Bulk -// publishes one index for the whole batch, so a throw part-way through would -// leave the moved elements outside both the caller's span and the queue (push) -// or inside both (pop) — the only data loss or duplication anywhere in this -// library. The single-element ops survive a throw and carry no such -// requirement, which is why the check sits on those two bodies and not on the -// class. The message cannot be factored out: C++20 requires a string literal. +// The bulk ops static_assert a noexcept move assignment in their bodies, not +// on the class, so SpscQueue with a throwing T still compiles for +// single-element use. Rationale is in the header's Exceptions paragraph. template SpscQueue::SpscQueue(std::size_t capacity) : buffer_(ring_slots(capacity)) {} diff --git a/tests/queue_contract_test.cpp b/tests/queue_contract_test.cpp index 2c8f60a..d35c2eb 100644 --- a/tests/queue_contract_test.cpp +++ b/tests/queue_contract_test.cpp @@ -207,10 +207,7 @@ TYPED_TEST(QueueContract, NonBlockingRetryLoopsTerminateViaClosed) { ASSERT_LT(++spins, 1000) << "try_push loop never terminated"; } - // The consumer drain loop from the README. The re-attempt after closed() is - // load-bearing twice over: breaking on closed() alone strands an element a - // producer pushed in the window, and breaking only when the re-attempt fails - // discards the one it just retrieved. Reaching the break here requires + // The consumer drain loop from the README. Reaching the break requires // consulting closed(), so a closed() stuck at false trips the spin guard. std::vector drained; spins = 0; @@ -259,16 +256,10 @@ TYPED_TEST(QueueContract, FailedPushConsumesRvaluesAndLeavesLvaluesIntact) { EXPECT_EQ(owned, nullptr) << "retrying with the same object would push a husk"; } -// A throwing move assignment is the one place the three queues genuinely -// differ, so it gets its own suite. MutexQueue and SpscQueue survive it with -// their indices intact; MpmcQueue cannot — a throw strands a claimed ticket -// whose sequence is never re-published — which is why it static_asserts the -// requirement instead of appearing here. That static_assert is its test. -// -// Nothing else in the suite can reach this path: every other T here has a -// noexcept move assignment. The throw is armed by a counter rather than a flag -// on the element, so the element enqueues normally and turns hostile only for -// the dequeue. +// MutexQueue and SpscQueue survive a throwing move assignment; MpmcQueue +// static_asserts it away, so that assert is its test and it is absent here. +// The throw is armed by a counter rather than a flag on the element, so the +// element enqueues normally and turns hostile only for the dequeue. int g_moves_until_throw = -1; // negative: never throw constexpr int kStolenMarker = -999; // what a stolen-from value is left holding constexpr int kSentinel = 99; // pre-loaded into out; the failed pop overwrites it From 9a38f337c5f1ce39e96c1383fc7a12631b9053e8 Mon Sep 17 00:00:00 2001 From: Debra Date: Sat, 29 Aug 2026 12:09:03 +0800 Subject: [PATCH 7/7] docs/tests: shorter assert message, colocate the bulk-assert rationale, drop the implementation-defined string half Co-Authored-By: Claude Fable 5 --- include/cq/mpmc_queue.hpp | 3 +-- include/cq/spsc_queue.hpp | 4 ++-- include/cq/spsc_queue.ipp | 6 ++---- tests/queue_contract_test.cpp | 12 ++---------- 4 files changed, 7 insertions(+), 18 deletions(-) diff --git a/include/cq/mpmc_queue.hpp b/include/cq/mpmc_queue.hpp index aca4608..99be5d8 100644 --- a/include/cq/mpmc_queue.hpp +++ b/include/cq/mpmc_queue.hpp @@ -47,8 +47,7 @@ template // NOLINTNEXTLINE(clang-analyzer-optin.performance.Padding) class MpmcQueue { static_assert(std::is_nothrow_move_assignable_v, - "MpmcQueue requires a T whose move assignment is noexcept: a throw would " - "strand a claimed slot and permanently degrade the queue"); + "MpmcQueue requires a noexcept move assignment"); public: /// @param capacity Fixed number of slots; never resized. diff --git a/include/cq/spsc_queue.hpp b/include/cq/spsc_queue.hpp index 2bbd445..8921025 100644 --- a/include/cq/spsc_queue.hpp +++ b/include/cq/spsc_queue.hpp @@ -46,8 +46,8 @@ namespace cq { /// retry loop must therefore re-materialise its argument every pass. /// /// @tparam T Element type. Must be DefaultConstructible (ring slots are -/// constructed up front) and MoveAssignable; try_push_n()/try_pop_n() -/// additionally require a noexcept move assignment and static_assert it. +/// constructed up front) and MoveAssignable; nothrow for the bulk ops (see +/// Exceptions). template // The "excessive padding" the analyzer flags is deliberate: head_ and tail_ // each get a private cache line (see cq/cache_line.hpp). diff --git a/include/cq/spsc_queue.ipp b/include/cq/spsc_queue.ipp index 0f4e1f4..a663747 100644 --- a/include/cq/spsc_queue.ipp +++ b/include/cq/spsc_queue.ipp @@ -15,10 +15,6 @@ namespace cq { -// The bulk ops static_assert a noexcept move assignment in their bodies, not -// on the class, so SpscQueue with a throwing T still compiles for -// single-element use. Rationale is in the header's Exceptions paragraph. - template SpscQueue::SpscQueue(std::size_t capacity) : buffer_(ring_slots(capacity)) {} @@ -143,6 +139,8 @@ void SpscQueue::dequeue(std::size_t head, T& out) { template std::size_t SpscQueue::try_push_n(std::span items) { + // In the body, not on the class: single-element use of a throwing T must + // still compile. static_assert(std::is_nothrow_move_assignable_v, "SpscQueue bulk transfer requires a noexcept move assignment"); if (items.empty() || closed_.load(std::memory_order_relaxed)) { diff --git a/tests/queue_contract_test.cpp b/tests/queue_contract_test.cpp index d35c2eb..7a22839 100644 --- a/tests/queue_contract_test.cpp +++ b/tests/queue_contract_test.cpp @@ -239,21 +239,13 @@ TYPED_TEST(QueueContract, FailedPushConsumesRvaluesAndLeavesLvaluesIntact) { EXPECT_FALSE(q.try_push(lvalue)); EXPECT_EQ(lvalue, "still here") << "an lvalue argument is copied, not consumed"; - std::string rvalue = "consumed"; - EXPECT_FALSE(q.try_push(std::move(rvalue))); - // Reading a moved-from object is the assertion, not an accident. A - // moved-from std::string is only valid-but-unspecified by the standard, so - // this half is a libstdc++/libc++ observation; the unique_ptr case below is - // the one the standard actually guarantees. - // NOLINTNEXTLINE(bugprone-use-after-move) - EXPECT_TRUE(rvalue.empty()) << "an rvalue argument is moved from even on failure"; - + // unique_ptr is the one type whose moved-from state the standard pins down. typename TestFixture::MoveOnlyQueue mq(1); ASSERT_TRUE(mq.try_push(std::make_unique(1))); auto owned = std::make_unique(2); EXPECT_FALSE(mq.try_push(std::move(owned))); // NOLINTNEXTLINE(bugprone-use-after-move) - EXPECT_EQ(owned, nullptr) << "retrying with the same object would push a husk"; + EXPECT_EQ(owned, nullptr) << "an rvalue argument is moved from even on failure"; } // MutexQueue and SpscQueue survive a throwing move assignment; MpmcQueue