fix(audio): prune dead followers in AudioOutputRouter (#3660)#3661
Conversation
Follow-up to #3630. Dead followers (QPointer guard gone null) were never removed from the registry — they lingered as no-op entries iterated on every fan-out. Harmless for the current register-once-at-startup use (two lifetime- owned followers), but it would accumulate unbounded once dynamically created/destroyed followers land (per the audio-sink-factory roadmap). Each follower now carries an optional liveness predicate, set by the QPointer-guarded template overload (raw std::function followers stay always- alive — the caller owns their lifetime). setCurrentDevice() prunes followers whose guard is null after the fan-out, on the live vector, so a follower added re-entrantly during the loop is kept. Test: audio_output_router_test extended to assert a destroyed follower is pruned while a live one is kept and still updated. 14/14. Closes #3660 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Re: the triage on #3660, which suggested doing both (1) prune dead guarded followers and (2) add a handle-based
One shape note: rather than the triage's Net: the documented leak (dead guarded followers accumulating) is fully fixed and covers 100% of current + foreseeable QObject sinks; explicit deregistration is deferred as YAGNI. |
There was a problem hiding this comment.
Reviewed the diff and the surrounding code — this looks good to me. Clean, correct, and well-tested. Thanks @ten9876! 🙏
What I verified
- Re-entrancy is preserved correctly. The fan-out still iterates the
followerssnapshot, andstd::erase_ifruns afterward on the livem_followers. A follower added re-entrantly during the loop isn't in the snapshot but is in the live vector with a non-null guard, so it's kept — matching the comment. Pruning-after-fan-out is the right ordering. - The alive/null distinction is sound. Guarded followers self-skip via
if (guarded)during apply and are pruned when theirQPointergoes null; rawstd::functionfollowers carry a nullalivepredicate and are never pruned (f.alive && !f.alive()short-circuits). That matches the caller-owns-lifetime contract. - No API breakage. Both real call sites (
MainWindow.cpp:1027-1028) use the guardedaddFollower(T*)template overload, and the public signatures are unchanged.std::erase_ifis C++20 and resolves through the already-included<vector>. - The test earns its keep — it asserts the dead follower is pruned (
followerCount2→1) and the live one is still updated, not just "no crash."
One minor, non-blocking observation
Pruning is opportunistic — it only fires inside setCurrentDevice(). In the future dynamic-sink world the PR is preparing for, sinks that are created and destroyed between device changes will linger in m_followers (as harmless no-ops) until the next fan-out. That's bounded by the additions-per-interval rather than truly unbounded, and it's consistent with the deliberate QPointer-no-deregister design, so I think it's the right tradeoff for now — just worth being aware of if a high-churn sink ever lands. No change needed.
Nice incremental hardening ahead of the audio-sink-factory roadmap.
🤖 aethersdr-agent · cost: $4.8619 · model: claude-opus-4-8
jensenpat
left a comment
There was a problem hiding this comment.
Defensive review (as an author of the original AudioOutputRouter #3630) — no blockers; LGTM.
Correctness verified line-by-line:
- Prune predicate
f.alive && !f.alive()short-circuits correctly: rawstd::functionfollowers (emptyalive) are never pruned (caller owns lifetime); QPointer-guarded followers are pruned exactly when the guard nulls. - Prune runs after the snapshot fan-out on the live vector, so a follower added re-entrantly during the loop is alive and kept — the original snapshot/reentrancy contract is preserved.
- Zero behavior change for today's two lifetime-owned followers (their QPointer never nulls during operation); this is pure future-proofing for dynamic followers, as described.
- Public API unchanged; the shared private
registerFollowerpath is clean.
Checks:
std::erase_if(vector)is valid (project is C++20).- All CI green on Windows/macOS/Linux + CodeQL + analyze.
- Built and ran the test locally: 14/14, including the new prune assertion and the pre-existing add-during-fan-out reentrancy case (still passes → prune doesn't regress reentrancy).
Minor non-blocking nit: the template captures guarded in two lambdas (apply + alive); a single shared QPointer would be marginally tidier, but this is correct and negligible. Not worth holding the merge.
Summary
Follow-up to #3630, closes #3660.
AudioOutputRouternever pruned followers whoseQPointerguard had gone null — a destroyed sink lingered inm_followersas a no-op closure iterated (and skipped) on everysetCurrentDevice()fan-out.Harmless for the current register-once-at-startup use (two lifetime-owned followers:
ClientPuduMonitor,QsoRecorder), but it would accumulate unbounded once dynamically created/destroyed followers land — e.g. a future per-receiver or WebSDR-sourced sink per the audio-sink-factory roadmap.How
QPointer-guarded templateaddFollower(T*)sets it ([guarded]{ return !guarded.isNull(); }); rawstd::functionfollowers leave it null and are treated as always alive (the caller owns their lifetime).setCurrentDevice()prunes dead followers withstd::erase_ifafter the fan-out, operating on the live vector — so a follower added re-entrantly during the loop is alive and kept. The snapshot fan-out and re-entrancy behavior are unchanged.Public API (
addFollower(std::function),addFollower(T*)) is unchanged; the two overloads now share an internalregisterFollower(apply, alive).Test
audio_output_router_testextended: registers a live + a heap follower, deletes the heap one, fans out, and asserts the dead follower is pruned (followerCount2 → 1) while the live one is kept and still updated. 14/14 (was 9/9). Full app builds clean.Refs #3306
🤖 Generated with Claude Code