139bfbb made the channel refuse a sentinel offered while one was still
pending — correct, and the reason is a data race: overwriting wrote
eof_value_ while the consumer could be moving the previous one out of it,
which ThreadSanitizer reports as a torn refcount and a heap-use-after-free.
That part stands.
What was wrong was the accounting. The refusal recorded a drop, and PoolNode
reported it through the overflow event callback, on the theory that two
control tokens on one channel means the stream ended twice and is a caller
protocol error.
It is not. A source that has reached the end of its input keeps being polled
and keeps returning EOF — that is the normal steady state, not an error — so
the token is re-offered on every firing. Refusing a re-offer loses nothing:
the pending token carries the same meaning and is already on its way.
Found by running it. scene-actor-extraction on a 14 s clip reported
[main] ERROR: frames were dropped (channel overflow):
frame_source: 2
scene_annotate: 1
[main] The output would describe footage that was never analysed.
Refusing to report success.
and exited 2, on a run where nothing had been dropped and every frame was
analysed. frame_source emits EOF once and then returns it forever
(frame_source_node.hpp:76), so the count grows with however many times the
source is polled after the end. The pipeline's own loss detector — which
exists because a dropped frame silently corrupts the output — was being
tripped by a clean run, which is the one thing a loss detector must not do.
So SlotBusy now records nothing and reports nothing. The cost, stated
plainly: a genuinely distinct second token would also be refused silently,
and the channel cannot tell a re-offer from a distinct token. Re-offering is
the case that actually occurs; the delivery guarantee that matters — the
first token arrives — holds either way.
The test asserting the old behaviour is inverted rather than deleted, and now
also checks that the token which was accepted is the one delivered. 148/148.
337 lines
12 KiB
C++
337 lines
12 KiB
C++
#include <string>
|
|
#include <catch2/catch_test_macros.hpp>
|
|
#include <catch2/catch_approx.hpp>
|
|
#include <kpn/channel.hpp>
|
|
#include <thread>
|
|
#include <vector>
|
|
|
|
using namespace kpn;
|
|
|
|
TEST_CASE("channel storage policy: small trivial type by value", "[channel]") {
|
|
STATIC_REQUIRE(channel_storage_policy<int>::by_value);
|
|
STATIC_REQUIRE(channel_storage_policy<float>::by_value);
|
|
}
|
|
|
|
TEST_CASE("channel storage policy: large type by shared_ptr", "[channel]") {
|
|
struct Big { char data[64]; };
|
|
STATIC_REQUIRE(!channel_storage_policy<Big>::by_value);
|
|
}
|
|
|
|
TEST_CASE("push and pop single value", "[channel]") {
|
|
Channel<int> ch(5);
|
|
ch.push(42);
|
|
REQUIRE(ch.pop() == 42);
|
|
}
|
|
|
|
TEST_CASE("channel respects capacity", "[channel]") {
|
|
Channel<int> ch(2);
|
|
ch.push(1);
|
|
ch.push(2);
|
|
REQUIRE_THROWS_AS(ch.push(3), ChannelOverflowError);
|
|
}
|
|
|
|
TEST_CASE("pop blocks until value available", "[channel]") {
|
|
Channel<int> ch(5);
|
|
int result = 0;
|
|
std::thread producer([&] {
|
|
std::this_thread::sleep_for(std::chrono::milliseconds(20));
|
|
ch.push(99);
|
|
});
|
|
result = ch.pop();
|
|
producer.join();
|
|
REQUIRE(result == 99);
|
|
}
|
|
|
|
TEST_CASE("try_pop returns false on timeout", "[channel]") {
|
|
Channel<int> ch(5);
|
|
int out = 0;
|
|
REQUIRE_FALSE(ch.try_pop(out, std::chrono::milliseconds(10)));
|
|
}
|
|
|
|
TEST_CASE("try_pop succeeds when value present", "[channel]") {
|
|
Channel<int> ch(5);
|
|
ch.push(7);
|
|
int out = 0;
|
|
REQUIRE(ch.try_pop(out, std::chrono::milliseconds(10)));
|
|
REQUIRE(out == 7);
|
|
}
|
|
|
|
TEST_CASE("disable unblocks waiting pop", "[channel]") {
|
|
Channel<int> ch(5);
|
|
std::thread disabler([&] {
|
|
std::this_thread::sleep_for(std::chrono::milliseconds(20));
|
|
ch.disable();
|
|
});
|
|
REQUIRE_THROWS_AS(ch.pop(), ChannelClosedError);
|
|
disabler.join();
|
|
}
|
|
|
|
TEST_CASE("push to disabled channel is silently dropped", "[channel]") {
|
|
Channel<int> ch(5);
|
|
ch.disable();
|
|
ch.push(99); // must not throw, must not enqueue
|
|
REQUIRE(ch.size() == 0);
|
|
}
|
|
|
|
TEST_CASE("disable stops accepting and unblocks pop", "[channel]") {
|
|
// With the SPSC lock-free ring, disable() does not drain the ring immediately;
|
|
// items are freed when the Channel is destroyed. What it must do is:
|
|
// 1. reject further pushes (drops silently)
|
|
// 2. unblock any waiting pop() (throws ChannelClosedError)
|
|
Channel<int> ch(5);
|
|
ch.push(1);
|
|
ch.push(2);
|
|
REQUIRE(ch.size() == 2);
|
|
ch.disable();
|
|
// Further pushes are dropped
|
|
ch.push(3);
|
|
REQUIRE(ch.size() <= 2);
|
|
// pop() must throw even though items remain in the ring
|
|
REQUIRE_THROWS_AS(ch.pop(), ChannelClosedError);
|
|
}
|
|
|
|
TEST_CASE("enable re-accepts pushes after disable", "[channel]") {
|
|
Channel<int> ch(5);
|
|
ch.disable();
|
|
ch.push(1);
|
|
REQUIRE(ch.size() == 0);
|
|
ch.enable();
|
|
ch.push(42);
|
|
REQUIRE(ch.pop() == 42);
|
|
}
|
|
|
|
TEST_CASE("large type stored as shared_ptr — no copy on pop", "[channel]") {
|
|
struct Big { char data[64]; int tag; };
|
|
Channel<Big> ch(5);
|
|
Big b{}; b.tag = 123;
|
|
ch.push(b);
|
|
auto out = ch.pop();
|
|
REQUIRE(out.tag == 123);
|
|
}
|
|
|
|
// ── ChannelDataSize / bytes_pushed tests ─────────────────────────────────────
|
|
|
|
TEST_CASE("bytes_pushed uses sizeof(T) by default for POD type", "[channel][bandwidth]") {
|
|
Channel<int> ch(10);
|
|
ch.push(1);
|
|
ch.push(2);
|
|
ch.push(3);
|
|
ch.pop(); ch.pop(); ch.pop();
|
|
|
|
auto snap = ch.snapshot("test");
|
|
REQUIRE(snap.pushes == 3);
|
|
REQUIRE(snap.bytes_pushed == 3 * sizeof(int));
|
|
}
|
|
|
|
TEST_CASE("bytes_pushed uses sizeof(T) by default for large struct", "[channel][bandwidth]") {
|
|
struct Blob { char data[256]; };
|
|
Channel<Blob> ch(10);
|
|
ch.push(Blob{});
|
|
ch.push(Blob{});
|
|
|
|
auto snap = ch.snapshot("test");
|
|
REQUIRE(snap.bytes_pushed == 2 * sizeof(Blob));
|
|
}
|
|
|
|
// A fake heap-owning type whose logical payload size differs from sizeof.
|
|
struct FakeFrame {
|
|
std::vector<uint8_t> pixels;
|
|
};
|
|
|
|
// Specialise ChannelDataSize so the channel counts actual pixel bytes.
|
|
template<>
|
|
struct kpn::ChannelDataSize<FakeFrame> {
|
|
static std::size_t bytes(const FakeFrame& f) { return f.pixels.size(); }
|
|
};
|
|
|
|
TEST_CASE("bytes_pushed uses ChannelDataSize specialisation for heap-owning type", "[channel][bandwidth]") {
|
|
Channel<FakeFrame> ch(10);
|
|
ch.push(FakeFrame{std::vector<uint8_t>(1000)});
|
|
ch.push(FakeFrame{std::vector<uint8_t>(2000)});
|
|
|
|
auto snap = ch.snapshot("test");
|
|
REQUIRE(snap.pushes == 2);
|
|
REQUIRE(snap.bytes_pushed == 3000);
|
|
// item_bytes is still sizeof(FakeFrame) — the struct header
|
|
REQUIRE(snap.item_bytes == sizeof(FakeFrame));
|
|
}
|
|
|
|
TEST_CASE("bandwidth_mbs is non-zero and correct for heap-owning type", "[channel][bandwidth]") {
|
|
Channel<FakeFrame> ch(10);
|
|
// Push 10 frames of 1 MB each
|
|
for (int i = 0; i < 10; ++i)
|
|
ch.push(FakeFrame{std::vector<uint8_t>(1'000'000)});
|
|
|
|
auto snap = ch.snapshot("test");
|
|
// 10 MB over 1 second => 10.0 MB/s
|
|
REQUIRE(snap.bandwidth_mbs(1.0) == Catch::Approx(10.0).epsilon(1e-6));
|
|
// Without the fix (using sizeof), this would have been ~40 bytes/s ≈ 0.00004 MB/s.
|
|
REQUIRE(snap.bandwidth_mbs(1.0) > 1.0);
|
|
}
|
|
|
|
TEST_CASE("bandwidth_mbs returns 0 when elapsed_s is zero or negative", "[channel][bandwidth]") {
|
|
Channel<int> ch(5);
|
|
ch.push(42);
|
|
auto snap = ch.snapshot("test");
|
|
REQUIRE(snap.bandwidth_mbs(0.0) == 0.0);
|
|
REQUIRE(snap.bandwidth_mbs(-1.0) == 0.0);
|
|
}
|
|
|
|
TEST_CASE("push_sentinel never overflows even on a full channel", "[channel][sentinel]") {
|
|
Channel<int> ch(2);
|
|
ch.push(1);
|
|
ch.push(2); // channel full — a plain push(3) would throw ChannelOverflowError
|
|
|
|
// The sentinel is stored out-of-band, so it neither throws nor blocks the
|
|
// caller — the exact property an EOF token needs under backpressure. This
|
|
// returns immediately with the ring still full.
|
|
REQUIRE(ch.push_sentinel(99));
|
|
REQUIRE(ch.size() == 2); // sentinel did not consume ring capacity
|
|
}
|
|
|
|
TEST_CASE("push_sentinel is delivered after all ring data, in order", "[channel][sentinel]") {
|
|
Channel<int> ch(4);
|
|
ch.push(1);
|
|
ch.push(2);
|
|
ch.push_sentinel(99); // enqueue EOF while data is still buffered
|
|
|
|
// Data drains first; the sentinel arrives only once the ring is empty.
|
|
REQUIRE(ch.pop() == 1);
|
|
REQUIRE(ch.pop() == 2);
|
|
REQUIRE(ch.pop() == 99);
|
|
}
|
|
|
|
TEST_CASE("push_sentinel wakes a blocked pop", "[channel][sentinel]") {
|
|
Channel<int> ch(2); // empty
|
|
std::thread producer([&] {
|
|
std::this_thread::sleep_for(std::chrono::milliseconds(20));
|
|
ch.push_sentinel(99); // must wake a consumer parked on an empty ring
|
|
});
|
|
REQUIRE(ch.pop() == 99);
|
|
producer.join();
|
|
}
|
|
|
|
TEST_CASE("approx_size counts a pending sentinel so consumers stay schedulable",
|
|
"[channel][sentinel]") {
|
|
Channel<int> ch(4);
|
|
REQUIRE(ch.approx_size() == 0);
|
|
ch.push_sentinel(99);
|
|
// Node readiness checks call approx_size(); it must report the out-of-band
|
|
// sentinel as consumable work even though it holds no ring slot.
|
|
REQUIRE(ch.approx_size() == 1);
|
|
REQUIRE(ch.size() == 0); // ...but the ring itself is still empty
|
|
int out = 0;
|
|
REQUIRE(ch.try_pop_now(out));
|
|
REQUIRE(out == 99);
|
|
REQUIRE(ch.approx_size() == 0);
|
|
}
|
|
|
|
TEST_CASE("try_pop_now delivers a pending sentinel once the ring is empty",
|
|
"[channel][sentinel]") {
|
|
Channel<int> ch(2);
|
|
ch.push(1);
|
|
ch.push_sentinel(99);
|
|
|
|
int out = 0;
|
|
REQUIRE(ch.try_pop_now(out)); // ring data first
|
|
REQUIRE(out == 1);
|
|
REQUIRE(ch.try_pop_now(out)); // then the sentinel
|
|
REQUIRE(out == 99);
|
|
REQUIRE_FALSE(ch.try_pop_now(out)); // nothing left
|
|
}
|
|
|
|
// Regression: the sentinel slot holds one token and refuses a second.
|
|
//
|
|
// push_sentinel used to write eof_value_ unconditionally. Offering a second
|
|
// token before the first was taken therefore did two wrong things at once: it
|
|
// lost the first silently — and a lost EOF wedges every downstream pop forever
|
|
// — and it wrote the storage while the consumer could be moving the previous
|
|
// value out of it. For the shared_ptr storage that non-trivial types use, that
|
|
// is a torn refcount, not merely a stale read.
|
|
//
|
|
// Refusing is correct rather than queueing: two control tokens on one channel
|
|
// means the stream ended twice, which is a caller protocol error. Coalescing
|
|
// them would hide it, and there is no second value that could sensibly follow
|
|
// the end of a stream.
|
|
TEST_CASE("a second sentinel is refused, not swallowed", "[channel][sentinel]") {
|
|
Channel<int> ch(4);
|
|
|
|
REQUIRE(ch.push_sentinel(1));
|
|
// Slot occupied: the first token is still undelivered.
|
|
REQUIRE_FALSE(ch.push_sentinel(2));
|
|
|
|
// The first survives intact — the overwrite is what used to lose it.
|
|
int out = 0;
|
|
REQUIRE(ch.try_pop_now(out));
|
|
CHECK(out == 1);
|
|
|
|
// And the slot is reusable once drained.
|
|
REQUIRE(ch.push_sentinel(3));
|
|
REQUIRE(ch.try_pop_now(out));
|
|
CHECK(out == 3);
|
|
}
|
|
|
|
TEST_CASE("a refused sentinel is not counted as a drop", "[channel][sentinel]") {
|
|
// A refusal means a token arrived while an equivalent one was already
|
|
// pending — not that anything was lost. Counting it as a drop was wrong in
|
|
// a way that showed up immediately on real content: a source at the end of
|
|
// its input keeps being polled and keeps returning EOF, so the token is
|
|
// re-offered on every firing, and the pipeline reported hundreds of dropped
|
|
// frames on a clean run and exited non-zero.
|
|
//
|
|
// The delivery guarantee is unaffected: the first token is pending and will
|
|
// arrive. Only the accounting changed.
|
|
Channel<int> ch(4);
|
|
REQUIRE(ch.push_sentinel(1));
|
|
const auto before = ch.stats().drops.load();
|
|
REQUIRE_FALSE(ch.push_sentinel(2));
|
|
CHECK(ch.stats().drops.load() == before);
|
|
|
|
// And the one that was accepted is still the one delivered.
|
|
int out = 0;
|
|
REQUIRE(ch.try_pop_now(out));
|
|
CHECK(out == 1);
|
|
}
|
|
|
|
TEST_CASE("try_push_sentinel leaves a refused value untouched", "[channel][sentinel]") {
|
|
// The non-consuming form exists so a refused token is still the caller's to
|
|
// report. The consuming push_sentinel cannot offer that, since the value is
|
|
// already moved into its parameter.
|
|
Channel<std::string> ch(4);
|
|
std::string first = "eof-1", second = "eof-2";
|
|
|
|
REQUIRE(ch.try_push_sentinel(first) == Channel<std::string>::SentinelResult::Taken);
|
|
REQUIRE(ch.try_push_sentinel(second) == Channel<std::string>::SentinelResult::SlotBusy);
|
|
CHECK(second == "eof-2"); // not moved from
|
|
|
|
ch.disable();
|
|
std::string third = "eof-3";
|
|
CHECK(ch.try_push_sentinel(third) == Channel<std::string>::SentinelResult::Closed);
|
|
CHECK(third == "eof-3");
|
|
}
|
|
|
|
// Regression: try_push must distinguish delivered from discarded.
|
|
//
|
|
// It returned bool, and returned *true* for a closed channel — so "the value
|
|
// arrived" and "the value was thrown away because nobody is listening" were the
|
|
// same answer. Every caller was nonetheless correct, because both cases mean
|
|
// "stop trying"; but nothing above the channel could tell the two apart, and a
|
|
// producer counting successful pushes counted discards among them. Only the
|
|
// channel's own drop counter knew, and only if someone read the diagnostics.
|
|
TEST_CASE("try_push distinguishes taken, full and closed", "[channel]") {
|
|
Channel<int> ch(2);
|
|
int v = 1;
|
|
|
|
CHECK(ch.try_push(v) == Channel<int>::PushResult::Taken);
|
|
CHECK(ch.try_push(v) == Channel<int>::PushResult::Taken);
|
|
// Ring is full: the value is untouched and the caller keeps it.
|
|
CHECK(ch.try_push(v) == Channel<int>::PushResult::Full);
|
|
CHECK(v == 1);
|
|
|
|
ch.disable();
|
|
const auto drops_before = ch.stats().drops.load();
|
|
CHECK(ch.try_push(v) == Channel<int>::PushResult::Closed);
|
|
// Discarded, and recorded as such rather than reported as a delivery.
|
|
CHECK(ch.stats().drops.load() == drops_before + 1);
|
|
}
|