fix: stop() must not return while a firing is still running

stop() set the flag, disabled the inputs and returned, leaving an executing
fire_once touching input_channels_, stats_ and pending_ while the caller went
on to destroy them. The comment was explicit about it: callers wanting the
guarantee should call scheduler_->drain() first. But ~PoolNode calls stop(),
and a destructor cannot ask its caller to have done that.

A node with a private pool survived by accident, because Node::stop() calls
pool->stop() and that joins the worker. A node sharing a pool — which
make_pool_node exists to create — had nothing joining it, so its own
destructor raced the firing.

stop() now waits on the submit gate, which is claimed for the whole of a
firing and released as its last act. A queued but unstarted firing also holds
it and will run, observe stop_flag_ and release, so the pool must still be
running when stop() is called; that is already the documented order and what
Node/ObjectNode do.

Two ways it declines to wait. It is bounded at five seconds, because a node
function that never returns must not convert teardown into a hang — it warns
and continues. And it returns immediately when called from the firing thread
itself, since an error handler that stops its own node would otherwise wait
for a firing that is waiting for it.

Verified in both directions: with the wait removed, stop() returns while the
node function is still sleeping and the flag it sets on the way out is still
false. 143/143.
This commit is contained in:
2026-08-05 15:46:27 +02:00
parent b9698fae60
commit 97670d8ba3
2 changed files with 156 additions and 3 deletions
+52
View File
@@ -668,3 +668,55 @@ TEST_CASE("a node woken with empty inputs does not stop itself", "[pool_node]")
node.stop();
pool->stop();
}
// Regression: stop() must not return while a firing is still running.
//
// stop() set the flag, disabled the inputs and returned, leaving an executing
// fire_once touching input_channels_, stats_ and pending_ while the caller went
// on to destroy them. The old comment was explicit that callers wanting the
// guarantee should call scheduler_->drain() first — but ~PoolNode calls stop(),
// and a destructor cannot ask its caller to have done that.
//
// A node with a private pool survived by accident: Node::stop() calls
// pool->stop(), which joins the worker. A node sharing a pool, which
// make_pool_node exists to create, had nothing joining it at all, so its own
// destructor raced the firing.
//
// Asserted through an observable side effect rather than by trying to catch the
// use-after-free: if stop() returns before the node function has finished, the
// flag it sets on the way out is still false.
namespace {
struct SlowFiring {
static constexpr std::string_view label() { return "slow_firing"; }
std::atomic<bool>* entered;
std::atomic<bool>* finished;
void operator()(int) {
entered->store(true, std::memory_order_release);
std::this_thread::sleep_for(std::chrono::milliseconds(200));
finished->store(true, std::memory_order_release);
}
};
} // namespace
TEST_CASE("stop waits for a firing already in flight", "[pool_node]") {
std::atomic<bool> entered{false}, finished{false};
auto pool = std::make_shared<ThreadPool>(2);
pool->start();
SlowFiring fn{&entered, &finished};
auto node = make_pool_node(fn, pool, 4);
node.start();
node.input_channel<0>().push(1);
// Stop only once the node is demonstrably inside its function.
while (!entered.load(std::memory_order_acquire))
std::this_thread::sleep_for(std::chrono::milliseconds(1));
node.stop();
CHECK(finished.load(std::memory_order_acquire));
pool->stop();
}