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

fix: make the submit gate a single atomic

Browse Source 
9c5ce5f established "a node never sleeps with a wake outstanding" and
implemented it as two independent atomics: queued_ for "a firing is in
flight", wake_pending_ for "a wake arrived during one". Two variables cannot
express that invariant, because the release side has to read and write both
and a wake can land in between:

  producer (try_submit)              worker (release_and_recheck)
  ------------------------           ----------------------------
  CAS reads queued_ == true, fails
                                     queued_.store(false)
                                     wake_pending_.exchange(false) -> false
  wake_pending_.store(true)

queued_ false, wake_pending_ true, nothing running and nothing scheduled —
exactly the state the invariant forbids. This is not a memory-ordering
subtlety; the interleaving holds under seq_cst.

SubmitGate replaces both with one atomic over three states, so "idle" and
"wake outstanding" are the same variable and no interleaving can produce
both. A release that finds a recorded wake keeps the claim and hands it to
the next firing, so the node is never momentarily idle while a submission
for it is in flight.

What this does not do is fix a reproducible hang. Every current call site
follows release_and_recheck() with a level re-check — on_input_ready(), or
outputs_have_space() on the parked path — which rediscovers the state a lost
wake would have signalled. The bug is masked, and I could not write a
node-level test that fails before and passes after; claiming otherwise would
be dishonest. The masking is a property of the call sites, not the
mechanism: any future early return that forgets its re-check reintroduces a
silent hang, and the pipeline has already been round that loop twice
(28e0667, then 9c5ce5f, each of which moved the stall rather than removing
it).

So the tests are structural. The state machine is pinned by contract tests,
and the defect it replaces is pinned by demonstration: LegacyGate in the
test file is the old protocol with a seam between the failed CAS and the
wake record, which makes the loss deterministic rather than something to
wait for. It also keeps the defect on record now that the code implementing
it is gone.

Also ignores build-*/ so a sanitizer build tree cannot be committed by
accident, which this commit did on its first attempt.
This commit is contained in:
dtourolle
2026-08-05 13:22:03 +02:00
parent 5628447ea8
commit f53af260a2
5 changed files with 328 additions and 58 deletions
Download Patch File Download Diff File Expand all files Collapse all files
include/kpn/pool_node.hpp
+51 -58
View File
@@ -5,6 +5,7 @@
#include "inode.hpp"
#include "port.hpp"
#include "scheduler.hpp"
#include "submit_gate.hpp"
#include "traits.hpp"
#include <array>
@@ -31,7 +32,7 @@ namespace kpn {
// Reactive alternative to Node<>. Instead of owning a blocked thread, the node
// is submitted to a shared IScheduler whenever all its input channels become
// non-empty. A single fire_once() call pops all inputs, executes the function,
// and pushes outputs. At most one fire_once() runs at a time (queued_ flag).
// and pushes outputs. At most one fire_once() runs at a time (see SubmitGate).
//
// Source nodes (input_count == 0) submit themselves immediately on start() and
// resubmit after each fire_once().
@@ -83,7 +84,7 @@ public:
void start() override {
enable_inputs(std::make_index_sequence<input_count>{});
stop_flag_.store(false, std::memory_order_relaxed);
queued_.store(false, std::memory_order_relaxed);
gate_.force_idle();
register_callbacks(std::make_index_sequence<input_count>{});
if constexpr (input_count == 0)
try_submit(0.5f);
@@ -145,8 +146,8 @@ public:
total_ms > 0 ? 100.0 * exec_ms / total_ms : 0.0,
qwait_ms,
stats_.total_exec_us.load(std::memory_order_relaxed) / 1000.0,
queued_.load(std::memory_order_relaxed),
wake_pending_.load(std::memory_order_relaxed),
gate_.queued(),
gate_.wake_pending(),
};
}
@@ -258,7 +259,7 @@ private:
stats_.exec_start_us.store(0, std::memory_order_relaxed);
// Plain store, not release_and_recheck(): this node is stopping, and
// honouring a pending wake here would resubmit a dead node.
queued_.store(false, std::memory_order_release);
gate_.force_idle();
stop_flag_.store(true, std::memory_order_relaxed);
}
@@ -346,39 +347,35 @@ private:
: 0.5f), ...);
}
/// Submit unless already queued. A wake that arrives while this node is
/// queued or running is *recorded*, never dropped.
/// Submit unless a firing is already in flight. A wake that arrives while
/// one is is *recorded* against it, never dropped.
///
/// Wakes are edge-triggered: a channel fires its space callback on the
/// transition, once. If that lands while queued_ is up, the CAS below fails
/// and — before wake_pending_ — the wake was gone. A node could then park a
/// transition, once. A dropped one never returns, so a node could park a
/// value, release its worker, and sleep forever holding output its consumer
/// was waiting for, with every worker idle in cond_wait and nothing left to
/// re-trigger it. Recording the drop turns the signal level-triggered: the
/// invariant is that a node never sleeps with a wake outstanding, enforced
/// by release_and_recheck() at every point that releases the node.
/// re-trigger it. SubmitGate makes "idle" and "wake outstanding" the same
/// variable, so the two cannot both be true — see submit_gate.hpp.
void try_submit(float priority) {
bool expected = false;
if (queued_.compare_exchange_strong(expected, true, std::memory_order_acq_rel))
if (gate_.claim())
scheduler_->submit([this] { fire_once(); }, priority);
else
wake_pending_.store(true, std::memory_order_release);
}
/// Clear queued_, then honour any wake that was dropped while it was up.
/// Every path that finishes or parks a firing must release the node through
/// here rather than storing queued_ directly.
/// End this firing, honouring any wake recorded during it. Every path that
/// finishes or parks a firing must release the node through here rather
/// than touching the gate directly. When a wake was recorded the gate stays
/// claimed and is handed to the next firing, so the node is never
/// momentarily idle with work outstanding.
void release_and_recheck(float priority = 0.5f) {
queued_.store(false, std::memory_order_release);
if (wake_pending_.exchange(false, std::memory_order_acq_rel))
try_submit(priority);
if (gate_.release())
scheduler_->submit([this] { fire_once(); }, priority);
}
// ── Execution ─────────────────────────────────────────────────────────────
void fire_once() {
if (stop_flag_.load(std::memory_order_relaxed)) {
queued_.store(false, std::memory_order_release);
gate_.force_idle();
return;
}
@@ -397,7 +394,7 @@ private:
release_and_recheck();
if (pending_) {
// Close the lost-wakeup race: a space_callback that fired
// between the failed push and clearing queued_ was
// between the failed push and releasing the gate was
// swallowed, and nothing else will wake this node. Re-check
// now that the flag is down.
if (outputs_have_space(std::make_index_sequence<output_count>{}))
@@ -475,8 +472,8 @@ private:
// Parked by the push above. Same situation as the retry path at the top
// of fire_once — and the same lost-wakeup race, which that path closes
// and this one did not. A space callback that fired while queued_ was
// still up got swallowed by try_submit's CAS, and the resubmit below
// and this one did not. A space callback that fired while the gate was
// still claimed is recorded there, and the resubmit below
// cannot cover it: this firing consumed its input, so inputs are empty
// and on_input_ready() will not resubmit. The node would then hold its
// value forever while its consumer waits for exactly that value and its
@@ -499,7 +496,7 @@ private:
}
// Pop all inputs — safe because we're the sole consumer and fire_once
// is guarded by queued_ (only one fire_once runs at a time).
// is guarded by the submit gate (only one fire_once runs at a time).
template<std::size_t... Is>
args_tuple pop_inputs(std::index_sequence<Is...>) {
return {pop_one<Is>()...};
@@ -584,9 +581,9 @@ private:
input_channels_t input_channels_;
output_channels_t output_channels_{};
std::atomic<bool> stop_flag_{true};
std::atomic<bool> queued_{false};
/// A wake that arrived while queued_ was up. See try_submit.
std::atomic<bool> wake_pending_{false};
/// Serialises firings and records wakes that arrive during one. See
/// submit_gate.hpp for why this cannot be two separate flags.
SubmitGate gate_;
/// The hidden one-slot output buffer (see push_outputs). Holding the value
/// here is what lets a node stop running without dropping it or occupying a
@@ -652,7 +649,7 @@ public:
void start() override {
enable_inputs(std::make_index_sequence<input_count>{});
stop_flag_.store(false, std::memory_order_relaxed);
queued_.store(false, std::memory_order_relaxed);
gate_.force_idle();
register_callbacks(std::make_index_sequence<input_count>{});
if constexpr (input_count == 0)
try_submit(0.5f);
@@ -694,8 +691,8 @@ public:
total_ms > 0 ? 100.0 * exec_ms / total_ms : 0.0,
qwait_ms,
stats_.total_exec_us.load(std::memory_order_relaxed) / 1000.0,
queued_.load(std::memory_order_relaxed),
wake_pending_.load(std::memory_order_relaxed),
gate_.queued(),
gate_.wake_pending(),
};
}
@@ -774,7 +771,7 @@ private:
stats_.exec_start_us.store(0, std::memory_order_relaxed);
// Plain store, not release_and_recheck(): this node is stopping, and
// honouring a pending wake here would resubmit a dead node.
queued_.store(false, std::memory_order_release);
gate_.force_idle();
stop_flag_.store(true, std::memory_order_relaxed);
}
@@ -855,37 +852,33 @@ private:
: 0.5f), ...);
}
/// Submit unless already queued. A wake that arrives while this node is
/// queued or running is *recorded*, never dropped.
/// Submit unless a firing is already in flight. A wake that arrives while
/// one is is *recorded* against it, never dropped.
///
/// Wakes are edge-triggered: a channel fires its space callback on the
/// transition, once. If that lands while queued_ is up, the CAS below fails
/// and — before wake_pending_ — the wake was gone. A node could then park a
/// transition, once. A dropped one never returns, so a node could park a
/// value, release its worker, and sleep forever holding output its consumer
/// was waiting for, with every worker idle in cond_wait and nothing left to
/// re-trigger it. Recording the drop turns the signal level-triggered: the
/// invariant is that a node never sleeps with a wake outstanding, enforced
/// by release_and_recheck() at every point that releases the node.
/// re-trigger it. SubmitGate makes "idle" and "wake outstanding" the same
/// variable, so the two cannot both be true — see submit_gate.hpp.
void try_submit(float priority) {
bool expected = false;
if (queued_.compare_exchange_strong(expected, true, std::memory_order_acq_rel))
if (gate_.claim())
scheduler_->submit([this] { fire_once(); }, priority);
else
wake_pending_.store(true, std::memory_order_release);
}
/// Clear queued_, then honour any wake that was dropped while it was up.
/// Every path that finishes or parks a firing must release the node through
/// here rather than storing queued_ directly.
/// End this firing, honouring any wake recorded during it. Every path that
/// finishes or parks a firing must release the node through here rather
/// than touching the gate directly. When a wake was recorded the gate stays
/// claimed and is handed to the next firing, so the node is never
/// momentarily idle with work outstanding.
void release_and_recheck(float priority = 0.5f) {
queued_.store(false, std::memory_order_release);
if (wake_pending_.exchange(false, std::memory_order_acq_rel))
try_submit(priority);
if (gate_.release())
scheduler_->submit([this] { fire_once(); }, priority);
}
void fire_once() {
if (stop_flag_.load(std::memory_order_relaxed)) {
queued_.store(false, std::memory_order_release);
gate_.force_idle();
return;
}
auto t0 = clock_t::now();
@@ -902,7 +895,7 @@ private:
release_and_recheck();
if (pending_) {
// Close the lost-wakeup race: a space_callback that fired
// between the failed push and clearing queued_ was
// between the failed push and releasing the gate was
// swallowed, and nothing else will wake this node. Re-check
// now that the flag is down.
if (outputs_have_space(std::make_index_sequence<output_count>{}))
@@ -974,8 +967,8 @@ private:
// Parked by the push above. Same situation as the retry path at the top
// of fire_once — and the same lost-wakeup race, which that path closes
// and this one did not. A space callback that fired while queued_ was
// still up got swallowed by try_submit's CAS, and the resubmit below
// and this one did not. A space callback that fired while the gate was
// still claimed is recorded there, and the resubmit below
// cannot cover it: this firing consumed its input, so inputs are empty
// and on_input_ready() will not resubmit. The node would then hold its
// value forever while its consumer waits for exactly that value and its
@@ -1054,9 +1047,9 @@ private:
input_channels_t input_channels_;
output_channels_t output_channels_{};
std::atomic<bool> stop_flag_{true};
std::atomic<bool> queued_{false};
/// A wake that arrived while queued_ was up. See try_submit.
std::atomic<bool> wake_pending_{false};
/// Serialises firings and records wakes that arrive during one. See
/// submit_gate.hpp for why this cannot be two separate flags.
SubmitGate gate_;
/// The hidden one-slot output buffer (see push_outputs). Holding the value
/// here is what lets a node stop running without dropping it or occupying a