fix: a filter or router must not drop on a full output
RouterNode and FilterNode were the last nodes on a data path still using the
throwing push() and swallowing the result:
try { out_ch_->push(val); } catch (const ChannelOverflowError&) {}
6595e6e made node outputs lossless, 28e0667 stopped them parking a worker,
a8cfe73 did the same for FanoutNode. These two were in none of them.
For ordinary values that is the familiar silent-loss problem: a dropped item
does not degrade a downstream result, it silently changes one, and the
consumer cannot tell it happened.
For a sentinel it is a hang. EOF is what tells every downstream node to shut
down and there is nothing after it to retry, so a filter that passes EOF by
predicate but drops it by backpressure produces a pipeline that never
terminates. scene-actor-extraction's decimator is exactly that shape —
`if (f.eof) return true;` in the predicate, feeding a chain whose slowest
node is an ONNX embedder, so the output is reliably full when EOF arrives.
Everything downstream then waits forever for a token that was discarded, and
the run has to be killed.
Both now route sentinels out-of-band via push_sentinel, which consumes no
ring capacity and cannot overflow, and retry ordinary values until taken.
Like FanoutNode and unlike a pool node, these own a private thread, so
waiting costs no scheduler worker and needs no space-callback park;
stop_flag_ is rechecked every pass so teardown cannot hang on a full output.
Time spent parked is charged to blocked rather than exec, so a held-up node
does not report as busy. An out-of-range router selector still drops by
design — that item was routed nowhere, which is not the same as lost.
is_sentinel_value moves from pool_node.hpp to traits.hpp. Every node type
that forwards a value needs it; these two not having it is the bug.
Verified in both directions. On c73edff the new case delivers 6 of 40 values
and never sets saw_eof; here it delivers 40 and terminates. EOF is emitted
exactly once, as a real source does — a test source that re-offered it would
mask the bug, since a later attempt could find the channel drained.
Note for downstream: the decimator is now a backpressure point rather than a
relief valve, so the source throttles to the face branch instead of quietly
thinning it. That is the intended behaviour, but it changes the shape of a
loaded run and is worth a benchmark comparison on a known clip.
This commit is contained in:
@@ -322,3 +322,100 @@ TEST_CASE("a fanout absorbs an unequal pair by slowing, not dropping",
|
||||
// the difference is the loss.
|
||||
CHECK(fast_seen - slow_seen < 200);
|
||||
}
|
||||
|
||||
// Regression: a filter must not drop an EOF sentinel into a full output.
|
||||
//
|
||||
// RouterNode and FilterNode were the last nodes on a data path still using the
|
||||
// throwing push() and swallowing the result:
|
||||
//
|
||||
// try { out_ch_->push(val); } catch (const ChannelOverflowError&) {}
|
||||
//
|
||||
// 6595e6e made node outputs lossless, 28e0667 stopped them parking a worker,
|
||||
// a8cfe73 did the same for FanoutNode. These two were in none of them.
|
||||
//
|
||||
// For ordinary values that is the familiar silent-loss problem. For a sentinel
|
||||
// it is a hang. EOF is what tells every downstream node to shut down, and
|
||||
// nothing comes after it to retry — so a filter that passes EOF by predicate
|
||||
// but drops it by backpressure produces a pipeline that never terminates. The
|
||||
// scene-actor-extraction decimator is exactly this shape: `if (f.eof) return
|
||||
// true;` in the predicate, feeding a chain whose slowest node is an ONNX
|
||||
// embedder, so the output is reliably full at the moment EOF arrives.
|
||||
//
|
||||
// The test forces that state rather than racing for it: the sink is slow enough
|
||||
// that the filter's output channel is saturated for the whole run, so EOF meets
|
||||
// a full ring with certainty.
|
||||
//
|
||||
// Both assertions are needed. `saw_eof` alone would pass on an implementation
|
||||
// that dropped every ordinary value and delivered only the sentinel; `count`
|
||||
// alone would pass on the broken one, which delivers plenty of values and loses
|
||||
// only the token that matters.
|
||||
namespace {
|
||||
|
||||
struct EofFrame {
|
||||
int seq{0};
|
||||
bool eof{false};
|
||||
};
|
||||
|
||||
// EOF is emitted exactly once, as a real source does. Everything after it is a
|
||||
// filler frame the predicate rejects, which keeps the node alive without
|
||||
// re-offering the sentinel — a source that retried EOF would mask the bug,
|
||||
// since a later attempt could find the channel drained.
|
||||
struct EofSource {
|
||||
static constexpr std::string_view label() { return "eof_source"; }
|
||||
int n{0};
|
||||
int total{0};
|
||||
EofFrame operator()() {
|
||||
if (n > total) {
|
||||
std::this_thread::sleep_for(std::chrono::milliseconds(1));
|
||||
return {-1, false}; // filler: dropped by the predicate
|
||||
}
|
||||
EofFrame f{n, n == total};
|
||||
++n;
|
||||
return f;
|
||||
}
|
||||
};
|
||||
|
||||
struct EofSink {
|
||||
static constexpr std::string_view label() { return "eof_sink"; }
|
||||
std::atomic<int>* count;
|
||||
std::atomic<bool>* saw_eof;
|
||||
void operator()(EofFrame f) {
|
||||
std::this_thread::sleep_for(std::chrono::microseconds(200));
|
||||
if (f.eof) saw_eof->store(true, std::memory_order_release);
|
||||
else count->fetch_add(1, std::memory_order_relaxed);
|
||||
}
|
||||
};
|
||||
|
||||
} // namespace
|
||||
|
||||
TEST_CASE("a filter delivers EOF into a saturated output", "[backpressure][filter]") {
|
||||
std::atomic<int> count{0};
|
||||
std::atomic<bool> saw_eof{false};
|
||||
|
||||
EofSource src_fn{0, 40};
|
||||
EofSink sink_fn{&count, &saw_eof};
|
||||
|
||||
// Every real frame passes the predicate, so the only thing between source
|
||||
// and sink is backpressure. Small channels keep the output saturated.
|
||||
auto filt = kpn::make_filter<EofFrame>(
|
||||
[](const EofFrame& f) { return f.seq >= 0; }, 4);
|
||||
|
||||
kpn::ObjectNode<EofSource, kpn::in<>, kpn::out<"f">, "eof_source", 0> s(src_fn, 4);
|
||||
kpn::ObjectNode<EofSink, kpn::in<"f">, kpn::out<>, "eof_sink", 0> k(sink_fn, 4);
|
||||
|
||||
auto net = kpn::make_network(
|
||||
kpn::edge(s.output<"f">(), filt.input<0>()),
|
||||
kpn::edge(filt.output<0>(), k.input<"f">())
|
||||
);
|
||||
net.start();
|
||||
|
||||
// Generous relative to 41 frames at 200 us, and this is a liveness test:
|
||||
// the broken implementation never sets saw_eof no matter how long it runs.
|
||||
for (int i = 0; i < 200 && !saw_eof.load(std::memory_order_acquire); ++i)
|
||||
std::this_thread::sleep_for(std::chrono::milliseconds(10));
|
||||
net.stop();
|
||||
|
||||
INFO("values delivered: " << count.load() << " of 40");
|
||||
CHECK(saw_eof.load(std::memory_order_acquire));
|
||||
CHECK(count.load(std::memory_order_relaxed) == 40);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user