Files
KPN/tests/test_channel.cpp
T
dtourolle c9aa246322
🚦 CI / changes (push) Successful in 5s
🚦 CI / docker (push) Has been skipped
🚦 CI / test (push) Successful in 5m0s
🚦 CI / tsan (push) Successful in 3m37s
🚦 CI / docs (push) Has been skipped
fix: a re-offered sentinel is not data loss
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.
2026-08-06 20:44:38 +02:00

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);
}