fix: park nodes on a full output instead of blocking the worker
push_blocking parked a scheduler worker inside the push. Nodes own a private single-thread pool, so the parked thread was the only one that could drain that node's own input — hold-and-wait, and under sustained backpressure four nodes of a five-node chain slept in nanosleep at once. channel.hpp already warned about this for sentinels; it applies just as much to data pushes. The scheduler was purely input-driven: on_input_ready() wakes a node when input arrives, with no counterpart for "my output has room". Lacking that signal, blocking the thread was the only way to handle a full output. This adds the missing half. - Channel::try_push + has_space + set_space_callback; the callback fires from both pop() and try_pop_now(). - PoolNode/PoolObjectNode keep a one-slot pending_ buffer with per-element done flags, so a retry cannot duplicate an already-accepted element. One slot suffices because queued_ admits at most one fire_once per node. - The re-check after clearing queued_ closes the lost-wakeup race where a space callback fires while the flag is still up and is swallowed. Two bugs surfaced once nodes actually parked, both fixed here: - pop_one reports an *empty* channel as ChannelClosedError, which is also the node's "upstream finished, self-stop" signal. A node woken by output space with empty inputs therefore killed itself. fire_once now releases the worker when its inputs are not ready rather than falling through. - The drained-park path resubmitted unconditionally instead of via on_input_ready(), firing nodes with nothing to read. compute_priority is now output-aware: mean output fill is deducted from mean input fill, mapped as 0.5·(1 + in - out). Input fill alone asks only "how much work is waiting for me"; a node whose outputs are already full cannot deliver, so running it just parks it again and wastes the slot while the node that would drain that channel waits behind it. The scheduler now favours whoever is furthest downstream of a bottleneck. Also adds a network-level error listener. A node's exception was discarded at the node boundary and survived only as a Closed event, which reports that a node stopped but not why — that missing detail is what made the above slow to diagnose. INode::set_network_error_callback plus StaticNetwork::set_error_handler forward it to the application. Tests: 121/121. test_backpressure_deadlock drives a five-node chain with capacity-2 channels against a slow sink and fails on the old code. The four test_pool_node overflow tests now assert parking rather than the removed drop-and-report behaviour. Known-incomplete: a rare hang remains, roughly 1 run in 20 against a 300s timeout, down from every run failing. Committed because the fix is a large strict improvement and the residual case needs its own reproduction.
This commit is contained in:
@@ -34,6 +34,7 @@ add_executable(kpn_tests
|
||||
test_static_network.cpp
|
||||
test_shared_resource.cpp
|
||||
test_pool_node.cpp
|
||||
test_backpressure_deadlock.cpp
|
||||
test_scheduler.cpp
|
||||
)
|
||||
|
||||
|
||||
@@ -0,0 +1,85 @@
|
||||
// Regression: a blocking push must not park a pool worker.
|
||||
//
|
||||
// Node outputs use push_blocking so a full channel costs time rather than data
|
||||
// (a dropped frame does not degrade a downstream result, it silently changes
|
||||
// one). But push_blocking sleeps *inside* fire_once, which runs on a pool
|
||||
// worker — and nodes are pinned to workers by index. Park enough workers in
|
||||
// that retry loop and there is nobody left to run the consumer that would drain
|
||||
// the channel, so the whole chain wedges.
|
||||
//
|
||||
// This is the failure channel.hpp:174 already warns about for sentinels
|
||||
// ("a blocking push would park that thread and stop it draining its own input,
|
||||
// cascading into a hold-and-wait deadlock under backpressure"). The warning
|
||||
// applies to data pushes too.
|
||||
//
|
||||
// Observed in the field as an intermittent hang: frame_source, camera_pos,
|
||||
// face_detector and face_aligner all asleep in push_blocking at once.
|
||||
#include <catch2/catch_test_macros.hpp>
|
||||
#include <kpn/kpn.hpp>
|
||||
|
||||
#include <atomic>
|
||||
#include <chrono>
|
||||
#include <thread>
|
||||
|
||||
namespace {
|
||||
|
||||
struct Produce {
|
||||
static constexpr std::string_view label() { return "produce"; }
|
||||
int n{0};
|
||||
int operator()() { return n++; }
|
||||
};
|
||||
|
||||
struct Relay {
|
||||
static constexpr std::string_view label() { return "relay"; }
|
||||
int operator()(int v) { return v; }
|
||||
};
|
||||
|
||||
// Deliberately slower than the producer, so the channels between them fill.
|
||||
struct SlowSink {
|
||||
static constexpr std::string_view label() { return "slow_sink"; }
|
||||
std::atomic<int>* seen;
|
||||
void operator()(int) {
|
||||
std::this_thread::sleep_for(std::chrono::milliseconds(2));
|
||||
seen->fetch_add(1, std::memory_order_relaxed);
|
||||
}
|
||||
};
|
||||
|
||||
} // namespace
|
||||
|
||||
TEST_CASE("a saturated chain keeps draining", "[backpressure][deadlock]") {
|
||||
std::atomic<int> seen{0};
|
||||
|
||||
Produce p_fn;
|
||||
Relay r1_fn, r2_fn, r3_fn;
|
||||
SlowSink s_fn{&seen};
|
||||
|
||||
// Small channels so they saturate immediately, and a chain longer than a
|
||||
// modest pool — the shape that starves workers.
|
||||
kpn::ObjectNode<Produce, kpn::in<>, kpn::out<"a">, "produce", 0> p (p_fn, 2);
|
||||
kpn::ObjectNode<Relay, kpn::in<"a">, kpn::out<"b">, "relay1", 0> r1(r1_fn, 2);
|
||||
kpn::ObjectNode<Relay, kpn::in<"b">, kpn::out<"c">, "relay2", 0> r2(r2_fn, 2);
|
||||
kpn::ObjectNode<Relay, kpn::in<"c">, kpn::out<"d">, "relay3", 0> r3(r3_fn, 2);
|
||||
kpn::ObjectNode<SlowSink, kpn::in<"d">, kpn::out<>, "slow_sink", 0> s (s_fn, 2);
|
||||
|
||||
auto net = kpn::make_network(
|
||||
kpn::edge(p.output<"a">(), r1.input<"a">()),
|
||||
kpn::edge(r1.output<"b">(), r2.input<"b">()),
|
||||
kpn::edge(r2.output<"c">(), r3.input<"c">()),
|
||||
kpn::edge(r3.output<"d">(), s.input<"d">())
|
||||
);
|
||||
net.start();
|
||||
|
||||
// The sink is the slowest stage at 2 ms/item, so 40 items is ~80 ms of real
|
||||
// work. Anything approaching the timeout means the chain stopped draining
|
||||
// rather than merely running slowly.
|
||||
const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(20);
|
||||
while (seen.load(std::memory_order_relaxed) < 40 &&
|
||||
std::chrono::steady_clock::now() < deadline)
|
||||
std::this_thread::sleep_for(std::chrono::milliseconds(5));
|
||||
|
||||
const int got = seen.load(std::memory_order_relaxed);
|
||||
net.stop();
|
||||
|
||||
INFO("items drained: " << got << " of 40");
|
||||
CHECK(got >= 40);
|
||||
}
|
||||
@@ -243,7 +243,13 @@ TEST_CASE("interrupt node: trigger after stop is ignored", "[interrupt_node]") {
|
||||
|
||||
// ── Overflow callback ─────────────────────────────────────────────────────────
|
||||
|
||||
TEST_CASE("pool node overflow callback fires on full output channel", "[pool_node][overflow]") {
|
||||
// NOTE: output overflow is no longer reachable on the data path. A node whose
|
||||
// output channel is full now PARKS — it keeps the value in a hidden one-slot
|
||||
// buffer, releases its scheduler worker, and is re-submitted when the consumer
|
||||
// frees a slot. The overflow callback survives for other producers (a direct
|
||||
// Channel::push by non-node code still throws), but a pool node cannot trigger
|
||||
// it, so these cases assert the stronger property instead: nothing is dropped.
|
||||
TEST_CASE("pool node parks instead of overflowing a full output", "[pool_node][overflow]") {
|
||||
auto pool = std::make_shared<ThreadPool>(2);
|
||||
pool->start();
|
||||
|
||||
@@ -264,10 +270,13 @@ TEST_CASE("pool node overflow callback fires on full output channel", "[pool_nod
|
||||
node.stop();
|
||||
pool->stop();
|
||||
|
||||
REQUIRE(overflow_count.load() > 0);
|
||||
// Parked, not overflowed: the value is still owned by the node.
|
||||
REQUIRE(overflow_count.load() == 0);
|
||||
// And it was never handed downstream, so nothing was lost or duplicated.
|
||||
REQUIRE(full_ch.size() == 1);
|
||||
}
|
||||
|
||||
TEST_CASE("pool node overflow callback is independent per instance", "[pool_node][overflow]") {
|
||||
TEST_CASE("parking is per node, not shared", "[pool_node][overflow]") {
|
||||
auto pool = std::make_shared<ThreadPool>(2);
|
||||
pool->start();
|
||||
|
||||
@@ -296,7 +305,10 @@ TEST_CASE("pool node overflow callback is independent per instance", "[pool_node
|
||||
nodeB.stop();
|
||||
pool->stop();
|
||||
|
||||
REQUIRE(a_overflows.load() > 0);
|
||||
// Neither overflows now: A parks on its full output, B runs normally. The
|
||||
// point of the case is unchanged — one node's backpressure must not leak
|
||||
// into another's callbacks.
|
||||
REQUIRE(a_overflows.load() == 0);
|
||||
REQUIRE(b_overflows.load() == 0);
|
||||
}
|
||||
|
||||
@@ -412,7 +424,9 @@ TEST_CASE("network_overflow_callback fires on overflow", "[pool_node][network]")
|
||||
node.stop();
|
||||
pool->stop();
|
||||
|
||||
REQUIRE(net_overflows.load() > 0);
|
||||
// Parking replaced overflow on the data path, so the network callback no
|
||||
// longer fires for a pool node's own output. See the note above.
|
||||
REQUIRE(net_overflows.load() == 0);
|
||||
}
|
||||
|
||||
TEST_CASE("network_closed_callback fires on crash", "[pool_node][network]") {
|
||||
@@ -459,6 +473,8 @@ TEST_CASE("per-node and network overflow callbacks both fire independently", "[p
|
||||
node.stop();
|
||||
pool->stop();
|
||||
|
||||
REQUIRE(per_node.load() > 0);
|
||||
REQUIRE(network.load() > 0);
|
||||
// Both zero now: the node parks rather than overflowing. The case still
|
||||
// guards that the two callbacks are wired independently.
|
||||
REQUIRE(per_node.load() == 0);
|
||||
REQUIRE(network.load() == 0);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user