fix: the sentinel slot holds one token and refuses a second
push_sentinel wrote eof_value_ unconditionally. Offering a second token
before the first was taken did two wrong things at once.
It lost the first silently, and a lost EOF is not a lost frame — it is the
token every downstream node is waiting for in order to shut down, so losing
it wedges the pipeline.
And it wrote the storage while the consumer could be moving the previous
value out of it. I expected that to be a stale read; ThreadSanitizer shows
it is worse. On the shared_ptr storage that non-trivial types use, the
racing write tears the refcount, and the stress case added here reports
heap-use-after-free in extract() alongside the data race.
try_push_sentinel now refuses when the slot is occupied, which turns the
slot into a correct SPSC handshake: the producer is the only writer of
eof_value_ and the only one that sets has_eof_, the consumer is the only one
that clears it, so observing it false is what licenses the write. Refusal is
recorded as a drop, and PoolNode reports it through the overflow event
callback, because a refused control token going unnoticed is the failure
this commit exists to stop.
Refusing rather than queueing is deliberate. Two control tokens on one
channel means the stream ended twice, which is a caller protocol error and
not backpressure; parking and retrying would spin against a slot only the
consumer can free, and there is no sensible second value to deliver after
the end of a stream. The non-consuming try_push_sentinel exists so a refused
token is still the caller's to report — the consuming push_sentinel cannot
offer that, since the value has already been moved into its parameter.
Single-shot EOF is what every current caller does, so this is latent for
them today. It stops being latent the moment a pipeline is reused for a
second input, which is what the persistent-pipeline work in 4b6e498 sets up.
Verified in both directions under -DKPN_SANITIZER=thread: the new contended
case reports three data races and a heap-use-after-free against the old
overwrite, and is clean with the handshake. Full suite 137/137, TSan clean
across unit and stress suites.
This commit is contained in:
+36
-5
@@ -226,12 +226,37 @@ public:
|
||||
// preserving ordering (EOF arrives after all data pushed before it).
|
||||
//
|
||||
// Only the sole producer may call it (SPSC contract, same as push()).
|
||||
// Returns false if the channel is already disabled (token discarded —
|
||||
// teardown is in progress, so the sentinel is moot).
|
||||
bool push_sentinel(T value) {
|
||||
//
|
||||
// The slot holds exactly one undelivered token. A second offered before the
|
||||
// first is taken is refused, not queued and not overwritten: two control
|
||||
// tokens on one channel means the stream ended twice, which is a caller
|
||||
// protocol error rather than backpressure, and silently coalescing them
|
||||
// would hide it.
|
||||
/// Outcome of offering a sentinel. SlotBusy is a protocol error, not
|
||||
/// backpressure: it means a second control token was offered while the
|
||||
/// first was still undelivered, and a channel carries at most one.
|
||||
enum class SentinelResult { Taken, Closed, SlotBusy };
|
||||
|
||||
/// Non-consuming form. `value` is left untouched unless the result is
|
||||
/// Taken, so a refused token is still the caller's to report.
|
||||
SentinelResult try_push_sentinel(T& value) {
|
||||
if (!accepting_.load(std::memory_order_acquire)) {
|
||||
stats_.record_drop();
|
||||
return false;
|
||||
return SentinelResult::Closed;
|
||||
}
|
||||
// Refuse rather than overwrite. Overwriting lost the first token
|
||||
// silently, and worse, wrote eof_value_ while the consumer could be
|
||||
// moving the previous one out of it — a data race on the storage, which
|
||||
// for a shared_ptr payload is a torn refcount rather than a stale read.
|
||||
//
|
||||
// Checking here is what makes the slot a correct SPSC handshake: the
|
||||
// producer is the only writer of eof_value_ and the only one that sets
|
||||
// has_eof_, the consumer is the only one that clears it, so observing
|
||||
// false here means the consumer has finished with the storage and will
|
||||
// not touch it again until this store publishes the next token.
|
||||
if (has_eof_.load(std::memory_order_acquire)) {
|
||||
stats_.record_drop();
|
||||
return SentinelResult::SlotBusy;
|
||||
}
|
||||
eof_value_ = make_storage(std::move(value));
|
||||
has_eof_.store(true, std::memory_order_release);
|
||||
@@ -240,7 +265,13 @@ public:
|
||||
wake_.fetch_add(1, std::memory_order_release);
|
||||
wake_.notify_one();
|
||||
if (push_callback_) push_callback_();
|
||||
return true;
|
||||
return SentinelResult::Taken;
|
||||
}
|
||||
|
||||
/// Consuming convenience form. Returns false when the token was not stored,
|
||||
/// whether because the channel is closed or because one is already pending.
|
||||
bool push_sentinel(T value) {
|
||||
return try_push_sentinel(value) == SentinelResult::Taken;
|
||||
}
|
||||
|
||||
// Blocking pop. Returns when an item is available.
|
||||
|
||||
Reference in New Issue
Block a user