fix: never self-move the parked output tuple
push_outputs ended with
else pending_ = std::move(result);
and the retry path calls it as push_outputs(std::move(*pending_), …), so on
that path `result` is the parked tuple itself. The assignment was a
self-move-assignment. std::tuple's is elementwise, and libstdc++'s
std::vector does not guard against self-move: _M_move_assign swaps its data
into a temporary, which is then destroyed. The vector ends up empty.
So the first park was clean — the argument there is a local temporary — and
the second erased the payload. The value was still delivered, still in
order, still counted, just empty. Downstream cannot distinguish that from a
frame on which the node genuinely found nothing, which is why it would never
surface as an error: in scene-actor-extraction it reads as "no faces in this
frame" and the run completes with a quietly wrong answer.
Scope, stated precisely because I first got it wrong: this needs a node with
*two or more* outputs. With one output the only thing that resubmits a
parked node is that output's own space callback, which by definition fires
when there is room, so the retry always succeeds and never reassigns. With
two, output A draining resubmits the node while output B is still full — the
retry skips A (already delivered, tracked in pending_done_) and fails on B,
and that is the reassignment that eats B's payload.
Every node in the scene-actor-extraction pipeline currently has exactly one
output, and the fanout is a separate class that does not use pending_, so
this is latent there rather than active. It is reachable by any multi-output
node under backpressure, which the library supports and documents.
Verified in both directions: on 6a4f45f the parked payload arrives with size
0; here it arrives intact. The test drives raw channels rather than consumer
nodes so each step is forced rather than raced, and both channels are
capacity 1 — Channel fires the space callback only on the full->not-full
edge, so a roomy channel A would never resubmit the node and the retry would
never happen at all.
This commit is contained in:
@@ -530,7 +530,15 @@ private:
|
||||
push_one_out<Is>(std::get<Is>(std::move(result))),
|
||||
all = all && pending_done_[Is]), ...);
|
||||
if (all) { pending_.reset(); pending_done_.fill(false); }
|
||||
else pending_ = std::move(result);
|
||||
// The retry path calls this as push_outputs(std::move(*pending_), …), so
|
||||
// on that path `result` *is* the parked tuple. Assigning it to itself is
|
||||
// a self-move-assignment, which for std::tuple is elementwise — and
|
||||
// libstdc++'s std::vector does not guard against it: it swaps its data
|
||||
// into a temporary and leaves the vector empty. A value that failed to
|
||||
// push twice would therefore be delivered with its payload silently
|
||||
// erased, which downstream reads as a legitimately empty result rather
|
||||
// than as a loss. Only store when it is not already stored.
|
||||
else if (!pending_ || &result != &*pending_) pending_ = std::move(result);
|
||||
}
|
||||
|
||||
/// Returns false when the ring was full and the value was NOT taken; the
|
||||
@@ -1012,7 +1020,15 @@ private:
|
||||
push_one_out<Is>(std::get<Is>(std::move(result))),
|
||||
all = all && pending_done_[Is]), ...);
|
||||
if (all) { pending_.reset(); pending_done_.fill(false); }
|
||||
else pending_ = std::move(result);
|
||||
// The retry path calls this as push_outputs(std::move(*pending_), …), so
|
||||
// on that path `result` *is* the parked tuple. Assigning it to itself is
|
||||
// a self-move-assignment, which for std::tuple is elementwise — and
|
||||
// libstdc++'s std::vector does not guard against it: it swaps its data
|
||||
// into a temporary and leaves the vector empty. A value that failed to
|
||||
// push twice would therefore be delivered with its payload silently
|
||||
// erased, which downstream reads as a legitimately empty result rather
|
||||
// than as a loss. Only store when it is not already stored.
|
||||
else if (!pending_ || &result != &*pending_) pending_ = std::move(result);
|
||||
}
|
||||
/// Returns false when the ring was full and the value was NOT taken; the
|
||||
/// caller must keep it and retry after the channel signals space.
|
||||
|
||||
Reference in New Issue
Block a user