fix: the sentinel must not be delivered ahead of a queued value
pop() and try_pop_now() observe the ring empty and then call take_sentinel().
The producer can push a value *and* publish the sentinel in the window between
those two steps, so the sentinel was delivered with a real value still queued
behind it — breaking the "sentinel is strictly last" contract that downstream
teardown depends on, and losing that value to any consumer which, like the
stress cases here, treats the sentinel as EOF and stops draining.
a0c4bf5 closed the variant where the caller's emptiness check ran against a
stale tail_ snapshot. This is the one where the check is fresh and simply too
early.
take_sentinel now re-checks emptiness *after* observing has_eof_, which is
what makes it sound rather than merely narrower: the producer publishes the
sentinel with a release store after its ring pushes, so a consumer that has
observed has_eof_ has necessarily observed every tail_ advance before it. A
non-empty ring at that point means those values genuinely precede the
sentinel, and returning false hands them over first.
Rates, since this is a race and the numbers are the evidence. The existing
"sentinel is strictly last" stress cases fail about 1 run in 15 on the commit
before this one and 0 in 25 after; they did not fail in 25 runs of the
pre-series baseline, so something in this series widened the window rather
than opened it. I could not pin down which change, and it does not much
matter: the interleaving is reachable from the code as written, and the
narrower version was never correct.
No new test. The two existing stress cases already assert exactly this and
are what caught it; a deterministic reproduction would need a seam inside
pop() that the fix then makes unreachable.
This commit is contained in:
@@ -444,6 +444,24 @@ private:
|
|||||||
// delivered after every value pushed before it.
|
// delivered after every value pushed before it.
|
||||||
bool take_sentinel(T& out) {
|
bool take_sentinel(T& out) {
|
||||||
if (!has_eof_.load(std::memory_order_acquire)) return false;
|
if (!has_eof_.load(std::memory_order_acquire)) return false;
|
||||||
|
// Re-check emptiness *after* observing has_eof_, not before.
|
||||||
|
//
|
||||||
|
// Callers check the ring is empty and then call this, but the producer
|
||||||
|
// can push a value and publish the sentinel in the window between those
|
||||||
|
// two steps — so the sentinel would be delivered with a real value still
|
||||||
|
// queued behind it, breaking the "sentinel is strictly last" contract
|
||||||
|
// that downstream teardown depends on. a0c4bf5 closed the variant where
|
||||||
|
// the caller's emptiness check used a stale tail_ snapshot; this is the
|
||||||
|
// one where the check is fresh but simply too early.
|
||||||
|
//
|
||||||
|
// Checking here is what makes it sound: the producer publishes the
|
||||||
|
// sentinel with a release store *after* its ring pushes, so a consumer
|
||||||
|
// that has observed has_eof_ has also observed every tail_ advance
|
||||||
|
// before it. If the ring is non-empty now, those values genuinely
|
||||||
|
// precede the sentinel and must be delivered first.
|
||||||
|
if (head_.load(std::memory_order_relaxed)
|
||||||
|
!= tail_.load(std::memory_order_acquire))
|
||||||
|
return false;
|
||||||
out = extract(std::move(eof_value_));
|
out = extract(std::move(eof_value_));
|
||||||
has_eof_.store(false, std::memory_order_release);
|
has_eof_.store(false, std::memory_order_release);
|
||||||
stats_.record_pop();
|
stats_.record_pop();
|
||||||
|
|||||||
Reference in New Issue
Block a user