dtourolle/KPN
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: start and stop in the topological order that was computed

Browse Source 
make_network computes Topo for the cycle check and then discarded it. The
node vector was filled in edge-declaration order — and then named
user_nodes_topo_ and relied upon as if it were sorted. halt() stops in its
reverse, and shutdown() walks it forwards stopping each node and draining its
outputs before the next, which is a graceful drain only if the order really
is sources-first.

It held for every network in this tree because edges happen to be declared in
pipeline order, so the two coincided. Declared any other way — which is legal
and which make_network otherwise accepts in silence — shutdown stops a
consumer before its producer and discards whatever was queued in front of it.

The order now comes from Topo::topo, which is already sources-first. Fanout
nodes appear there too and are skipped, since they are owned separately in
fanout_storage; they are still started after the user nodes and stopped
before them, so a fanout sitting between two user nodes is not staged
precisely during a drain. That is a smaller gap than the one being closed and
is left alone rather than restructured on the way past.

The test declares the sink edge first and the source edge last, and asserts
through shutdown() rather than by reading the order back — the order is
private, and what it buys is the point. A sources-first shutdown lets the
backlog queued in front of the slow relay reach the sink; stopping the relay
first discards all of it.

Worth recording how this went, because it is the more useful half: the new
test segfaulted, 12 runs in 20. Not a fault in the ordering change — it was
stopping sources first that finally put a live consumer behind a dead
producer, which is the condition the previous commit's crash needs. The
ordering fix did not introduce that bug, it made it reachable.

Verified in both directions: with declaration order the sink receives nothing
after shutdown begins; with topological order it receives the whole backlog.
This commit is contained in:
dtourolle
2026-08-05 15:14:35 +02:00
parent abbb2d4770
commit 87c5f98d04
2 changed files with 115 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files
include/kpn/static_network.hpp
+32 -23
View File
@@ -374,29 +374,6 @@ auto make_network(Edges&&... edges) {
// 4. Construct owned fanout storage on the heap (FanoutNode has jthread — not moveable)
auto fanout_storage = std::make_unique<FanoutSto>();
// 5. Collect unique user node pointers + their display names, in edge-declaration order
std::vector<INode*> user_node_ptrs;
std::vector<std::string> user_node_names;
auto collect = [&](auto& e) {
using SrcT = std::decay_t<decltype(e.src)>;
using DstT = std::decay_t<decltype(e.dst)>;
auto* s = static_cast<INode*>(&e.src);
auto* d = static_cast<INode*>(&e.dst);
if (std::find(user_node_ptrs.begin(), user_node_ptrs.end(), s) == user_node_ptrs.end()) {
auto sname = node_display_name<SrcT>();
user_node_ptrs.push_back(s);
user_node_names.push_back(sname);
s->set_name(sname);
}
if (std::find(user_node_ptrs.begin(), user_node_ptrs.end(), d) == user_node_ptrs.end()) {
auto dname = node_display_name<DstT>();
user_node_ptrs.push_back(d);
user_node_names.push_back(dname);
d->set_name(dname);
}
};
(collect(edges), ...);
// 5. Wire all expanded SimpleEdges.
// find_node<NodeT>: searches fanout storage then user edge pack, returns NodeT*.
// Uses if constexpr in a fold so mismatched types never reach assignment.
@@ -421,6 +398,38 @@ auto make_network(Edges&&... edges) {
return ptr;
};
// 5. Collect user node pointers + display names in *topological* order.
//
// Topo is computed above for the cycle check and used to be discarded,
// while this vector was filled in edge-declaration order — and then named
// user_nodes_topo_ and relied upon as if it were sorted. halt() stops in
// its reverse, and shutdown() walks it forwards stopping each node and
// draining its outputs before the next, which is only a graceful drain if
// the order really is sources-first. It held for every network in the tree
// because edges happen to be declared in pipeline order, and would have
// broken silently for one that was not.
//
// Fanout nodes appear in Topo too; they are skipped here because they are
// owned separately, in fanout_storage.
std::vector<INode*> user_node_ptrs;
std::vector<std::string> user_node_names;
[&]<typename... Ns>(tmp::TypeList<Ns...>) {
([&]<typename NodeT>() {
if constexpr (!requires { NodeT::is_fanout_node; }) {
if (auto* p = find_node.template operator()<NodeT>()) {
auto* n = static_cast<INode*>(p);
if (std::find(user_node_ptrs.begin(), user_node_ptrs.end(), n)
== user_node_ptrs.end()) {
auto nm = node_display_name<NodeT>();
user_node_ptrs.push_back(n);
user_node_names.push_back(nm);
n->set_name(nm);
}
}
}
}.template operator()<Ns>(), ...);
}(typename Topo::topo{});
// Pre-pass: build fanout_id → source display name map so fanout nodes
// can be named after the node feeding them (e.g. "capture_fanout").
std::map<std::size_t, std::string> fanout_src_name;