repair: drop the settling ritual, record the binding's timeout leak
Both P3s from the round-4 review. The eight non-blocking mainloop iterations in `Drop` were a ritual, not a barrier: a fixed number of polls cannot guarantee any particular event became ready. They were also unnecessary — PulseAudio's context unlink cancels outstanding operations and tears down the context's socket machinery synchronously, so once `drop(context)` returns the mainloop has no obligation left to service. Removed rather than replaced with a time-bounded drain, since there is no asynchronous obligation for such a drain to wait on. Re-verified live: both field gates and a clean-graph run still exit 0, with no abort. Also recorded, at the constant it depends on: on a request timeout the `Operation` wrapper is dropped while still running, and libpulse-binding 2.30.1 only unrefs the C operation, so the boxed callback and its captured `Rc`s leak until the context cancels the operation at disconnect. Harmless here — `--repair` is a one-shot process that exits immediately after — and it cannot become a use-after-free, since the closure owns its clones and the context clears callbacks before the mainloop is touched. It would NOT be acceptable in the long-lived host, so the note says so where someone would otherwise reuse this module for host-side loading. The review's verdict on the teardown itself: disconnect, destroy the context while the mainloop lives, then the mainloop, is the correct order, and taking the context explicitly makes it independent of field declaration order. 247 tests, clippy clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -58,6 +58,16 @@ use super::plan::ModuleObservation;
|
||||
const CONNECT_BUDGET: Duration = Duration::from_secs(3);
|
||||
|
||||
/// How long any single introspection request may take.
|
||||
///
|
||||
/// ⚠️ On timeout the `Operation` wrapper is dropped while still running. In
|
||||
/// libpulse-binding 2.30.1 that only unrefs the C operation — the boxed callback
|
||||
/// and the `Rc`s it captured leak until the context cancels the operation at
|
||||
/// disconnect. That is bounded and harmless *here*, because `--repair` is a
|
||||
/// one-shot process that exits immediately afterwards, and it cannot become a
|
||||
/// use-after-free (the closure owns its clones, and the context clears callbacks
|
||||
/// before the mainloop is touched). **It would not be acceptable in the long-lived
|
||||
/// host**, so this module must not be reused for host-side loading until that
|
||||
/// binding bug is fixed or worked around; `op.cancel()` does not help.
|
||||
const REQUEST_BUDGET: Duration = Duration::from_secs(3);
|
||||
|
||||
/// How long to sleep between mainloop iterations while waiting. Non-blocking
|
||||
@@ -97,19 +107,20 @@ pub struct PulseSession {
|
||||
|
||||
impl Drop for PulseSession {
|
||||
fn drop(&mut self) {
|
||||
// Disconnect, then destroy the context while the mainloop it registered IO
|
||||
// events with is still alive, then let the mainloop go. Bounded and
|
||||
// best-effort: this runs on the way out, with nobody left to report to.
|
||||
// Disconnect, then destroy the context while the mainloop it registered its
|
||||
// IO events with is still alive. The mainloop then drops after us.
|
||||
//
|
||||
// Nothing is drained afterwards on purpose. An earlier version iterated the
|
||||
// mainloop a few times here to "let teardown settle", which was a ritual
|
||||
// rather than a barrier: a fixed number of non-blocking polls cannot
|
||||
// guarantee that any particular event became ready. It is also unnecessary —
|
||||
// PulseAudio's context unlink cancels outstanding operations and removes the
|
||||
// context's socket machinery synchronously, so by the time `drop(context)`
|
||||
// returns there is no obligation left for the mainloop to service.
|
||||
if let Some(mut context) = self.context.take() {
|
||||
context.disconnect();
|
||||
drop(context);
|
||||
}
|
||||
for _ in 0..8 {
|
||||
if matches!(self.mainloop.iterate(false), IterateResult::Success(_)) {
|
||||
continue;
|
||||
}
|
||||
break;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user