diff --git a/include/kpn/pool_node.hpp b/include/kpn/pool_node.hpp index cf52f2b..d228a84 100644 --- a/include/kpn/pool_node.hpp +++ b/include/kpn/pool_node.hpp @@ -264,7 +264,7 @@ private: disable_inputs(std::make_index_sequence{}); disable_outputs(std::make_index_sequence{}); stats_.exec_start_us.store(0, std::memory_order_relaxed); - // Plain store, not release_and_recheck(): this node is stopping, and + // force_idle, not finish_firing(): this node is stopping, and // honouring a pending wake here would resubmit a dead node. gate_.force_idle(); stop_flag_.store(true, std::memory_order_relaxed); @@ -368,18 +368,50 @@ private: scheduler_->submit([this] { fire_once(); }, priority); } - /// 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) { - if (gate_.release()) - scheduler_->submit([this] { fire_once(); }, priority); - } - // ── Execution ───────────────────────────────────────────────────────────── + /// Decide whether this node should run again, then release the gate — in + /// that order, always. + /// + /// Releasing first is what let two firings of the same node overlap: the + /// moment the gate is free another worker may enter fire_once, while this + /// invocation is still reading pending_ and writing pending_done_. TSan + /// caught it as a race on pending_done_ between a firing submitted by the + /// old release_and_recheck and one submitted by try_submit. It also quietly + /// broke the one-slot park, which is sound only because "at most one + /// fire_once runs per node at a time" — with two, a value can be parked by + /// one firing and overwritten by the other. + /// + /// Everything this reads belongs to the firing that holds the claim, so it + /// is all evaluated first and the release is the last thing the firing does. + void finish_firing() { + bool want_more = false; + float prio = 0.5f; + + if (!stop_flag_.load(std::memory_order_relaxed)) { + bool parked = false; + if constexpr (!std::is_void_v) + parked = pending_.has_value(); + + if (parked) { + // Still holding output: only worth running again once the + // consumer has made room. + want_more = outputs_have_space(std::make_index_sequence{}); + } else { + if constexpr (input_count == 0) { + want_more = true; // sources always run again + } else { + want_more = count_ready(std::make_index_sequence{}) + == input_count; + if (want_more) prio = compute_priority(); + } + } + } + + if (gate_.release()) scheduler_->submit([this] { fire_once(); }, prio); + else if (want_more) try_submit(prio); + } + void fire_once() { if (stop_flag_.load(std::memory_order_relaxed)) { gate_.force_idle(); @@ -398,25 +430,13 @@ private: if constexpr (!std::is_void_v) { if (pending_) { push_outputs(std::move(*pending_), std::make_index_sequence{}); - release_and_recheck(); - if (pending_) { - // Close the lost-wakeup race: a space_callback that fired - // 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{})) - try_submit(0.5f); - return; // parked - } - // Drained: resume normal firing, resubmitting exactly the way - // the normal tail below does. An unconditional try_submit here - // would fire a node whose inputs are empty, and pop_inputs - // reports an empty channel as ChannelClosedError — which this - // node treats as "upstream finished" and self-stops on. That - // is a live node killing itself purely because it was woken by - // *output* space rather than by input arrival. - if constexpr (input_count == 0) try_submit(0.5f); - else on_input_ready(); + // Whether the value went out or is still parked, finish_firing + // reads pending_ and picks the right follow-up: output space if + // still holding, input readiness if drained. Resubmitting + // unconditionally would fire a node whose inputs are empty, and + // pop_one reports an empty channel as ChannelClosedError — which + // this node treats as "upstream finished" and self-stops on. + finish_firing(); return; } } @@ -428,8 +448,9 @@ private: // on_input_ready() resubmits when data actually lands. if constexpr (input_count > 0) { if (count_ready(std::make_index_sequence{}) != input_count) { - release_and_recheck(); - on_input_ready(); // data may have arrived while we checked + // finish_firing re-checks readiness after the work above, so + // data that landed while we looked is not missed. + finish_firing(); return; } } @@ -473,33 +494,11 @@ private: } stats_.exec_start_us.store(0, std::memory_order_relaxed); - release_and_recheck(); - - if (stop_flag_.load(std::memory_order_relaxed)) return; - - // 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 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 - // producer parks on an input channel that never drains. Re-check now - // that the flag is down. - if constexpr (!std::is_void_v) { - if (pending_) { - if (outputs_have_space(std::make_index_sequence{})) - try_submit(0.5f); - return; // parked - } - } - - // Source nodes always resubmit; others resubmit only if inputs are ready. - if constexpr (input_count == 0) { - try_submit(0.5f); - } else { - on_input_ready(); - } + // If the push above parked, finish_firing waits on output space rather + // than input arrival: this firing consumed its input, so an input-level + // check would not resubmit and the node would hold its value forever + // while its consumer waits for exactly that value. + finish_firing(); } // Pop all inputs — safe because we're the sole consumer and fire_once @@ -787,7 +786,7 @@ private: disable_inputs(std::make_index_sequence{}); disable_outputs(std::make_index_sequence{}); stats_.exec_start_us.store(0, std::memory_order_relaxed); - // Plain store, not release_and_recheck(): this node is stopping, and + // force_idle, not finish_firing(): this node is stopping, and // honouring a pending wake here would resubmit a dead node. gate_.force_idle(); stop_flag_.store(true, std::memory_order_relaxed); @@ -884,14 +883,46 @@ private: scheduler_->submit([this] { fire_once(); }, priority); } - /// 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) { - if (gate_.release()) - scheduler_->submit([this] { fire_once(); }, priority); + /// Decide whether this node should run again, then release the gate — in + /// that order, always. + /// + /// Releasing first is what let two firings of the same node overlap: the + /// moment the gate is free another worker may enter fire_once, while this + /// invocation is still reading pending_ and writing pending_done_. TSan + /// caught it as a race on pending_done_ between a firing submitted by the + /// old release_and_recheck and one submitted by try_submit. It also quietly + /// broke the one-slot park, which is sound only because "at most one + /// fire_once runs per node at a time" — with two, a value can be parked by + /// one firing and overwritten by the other. + /// + /// Everything this reads belongs to the firing that holds the claim, so it + /// is all evaluated first and the release is the last thing the firing does. + void finish_firing() { + bool want_more = false; + float prio = 0.5f; + + if (!stop_flag_.load(std::memory_order_relaxed)) { + bool parked = false; + if constexpr (!std::is_void_v) + parked = pending_.has_value(); + + if (parked) { + // Still holding output: only worth running again once the + // consumer has made room. + want_more = outputs_have_space(std::make_index_sequence{}); + } else { + if constexpr (input_count == 0) { + want_more = true; // sources always run again + } else { + want_more = count_ready(std::make_index_sequence{}) + == input_count; + if (want_more) prio = compute_priority(); + } + } + } + + if (gate_.release()) scheduler_->submit([this] { fire_once(); }, prio); + else if (want_more) try_submit(prio); } void fire_once() { @@ -910,25 +941,13 @@ private: if constexpr (!std::is_void_v) { if (pending_) { push_outputs(std::move(*pending_), std::make_index_sequence{}); - release_and_recheck(); - if (pending_) { - // Close the lost-wakeup race: a space_callback that fired - // 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{})) - try_submit(0.5f); - return; // parked - } - // Drained: resume normal firing, resubmitting exactly the way - // the normal tail below does. An unconditional try_submit here - // would fire a node whose inputs are empty, and pop_inputs - // reports an empty channel as ChannelClosedError — which this - // node treats as "upstream finished" and self-stops on. That - // is a live node killing itself purely because it was woken by - // *output* space rather than by input arrival. - if constexpr (input_count == 0) try_submit(0.5f); - else on_input_ready(); + // Whether the value went out or is still parked, finish_firing + // reads pending_ and picks the right follow-up: output space if + // still holding, input readiness if drained. Resubmitting + // unconditionally would fire a node whose inputs are empty, and + // pop_one reports an empty channel as ChannelClosedError — which + // this node treats as "upstream finished" and self-stops on. + finish_firing(); return; } } @@ -938,8 +957,9 @@ private: // into pop_inputs on an empty channel. if constexpr (input_count > 0) { if (count_ready(std::make_index_sequence{}) != input_count) { - release_and_recheck(); - on_input_ready(); + // finish_firing re-checks readiness after the work above, so + // data that landed while we looked is not missed. + finish_firing(); return; } } @@ -980,28 +1000,11 @@ private: } stats_.exec_start_us.store(0, std::memory_order_relaxed); - release_and_recheck(); - if (stop_flag_.load(std::memory_order_relaxed)) return; - - // 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 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 - // producer parks on an input channel that never drains. Re-check now - // that the flag is down. - if constexpr (!std::is_void_v) { - if (pending_) { - if (outputs_have_space(std::make_index_sequence{})) - try_submit(0.5f); - return; // parked - } - } - - if constexpr (input_count == 0) try_submit(0.5f); - else on_input_ready(); + // If the push above parked, finish_firing waits on output space rather + // than input arrival: this firing consumed its input, so an input-level + // check would not resubmit and the node would hold its value forever + // while its consumer waits for exactly that value. + finish_firing(); } template