fix(player): declare and enforce a lock hierarchy for PlayerController
The controller carries seventeen mutexes, reached from the MPV event loop,
JNI callbacks, sleep and autoplay timers, the session poller and every IPC
command. Nothing prevented two threads taking the same pair in opposite
orders, which deadlocks playback outright — and this subsystem has already
produced one deadlock.
No inversion exists today: the acquisitions really are scoped, and
`previous()` explicitly drops the backend guard before touching the queue.
That is the point. It holds by convention, convention is not checked, and
the failure it guards against is a frozen app with no error anywhere.
`LOCK_ORDER` writes the convention down, following the nesting the code
already relies on — `backend` before `queue` ("what is playing" before
"what is next"), `event_emitter` last because notifying the frontend must
never reach back for player state.
The tripwire only reports acquisitions that actually *overlap*, since two
locks taken one after another, each released before the next, cannot
deadlock. Verified by injecting a real inversion into `seek()`, which the
test located by line and rank.
This commit is contained in:
@@ -0,0 +1,268 @@
|
||||
//! A declared lock hierarchy for [`PlayerController`], and a tripwire that
|
||||
//! enforces it.
|
||||
//!
|
||||
//! The controller carries seventeen separate mutexes, reached from the MPV event
|
||||
//! loop, JNI callbacks, sleep/autoplay timers, the session poller and every IPC
|
||||
//! command. Nothing about that arrangement prevents two threads taking the same
|
||||
//! two locks in opposite orders, which deadlocks the player outright — and this
|
||||
//! subsystem has already produced one deadlock (a tokio `MutexGuard` held in a
|
||||
//! `match` scrutinee, which stalled the `AdvanceToNext` arm).
|
||||
//!
|
||||
//! Today the code is disciplined: acquisitions are scoped, and `previous()` for
|
||||
//! instance explicitly drops the backend guard before touching the queue. But
|
||||
//! that holds by convention, and convention is not checked. [`LOCK_ORDER`]
|
||||
//! writes the convention down and `every_overlapping_acquisition_respects_the_order`
|
||||
//! fails the build when a change breaks it.
|
||||
//!
|
||||
//! Ordering only matters where one guard is **still held** while another lock is
|
||||
//! taken. Acquiring two locks one after another, each released before the next,
|
||||
//! cannot deadlock — so the analysis looks for overlap, not for mere sequence.
|
||||
//!
|
||||
//! TRACES: UR-005 | DR-052
|
||||
|
||||
// This module is a static analysis of `player/mod.rs` plus the hierarchy it
|
||||
// checks against. Its only caller is its own test module, but `LOCK_ORDER` is
|
||||
// the documentation of record for how these locks nest, so it stays compiled
|
||||
// (and rustdoc'd) rather than hidden behind `cfg(test)`.
|
||||
#![allow(dead_code)]
|
||||
|
||||
/// The order in which `PlayerController`'s locks may be nested.
|
||||
///
|
||||
/// A thread already holding one of these may only acquire a lock that appears
|
||||
/// **later** in this list. The order is not arbitrary — it follows the nesting
|
||||
/// the code already relies on:
|
||||
///
|
||||
/// - `repository` and `sleep_timer` are taken by long-running decisions that go
|
||||
/// on to consult playback state, so they sit outermost.
|
||||
/// - `backend` outranks `queue`: "what is playing" is read before "what is
|
||||
/// next", never the reverse.
|
||||
/// - `event_emitter` is last. Emitting is a leaf — notifying the frontend must
|
||||
/// never reach back for more player state.
|
||||
///
|
||||
/// TRACES: UR-005 | DR-052
|
||||
pub const LOCK_ORDER: &[&str] = &[
|
||||
"repository",
|
||||
"sleep_timer",
|
||||
"countdown_cancel",
|
||||
"jellyfin_client",
|
||||
"backend",
|
||||
"queue",
|
||||
"stream_resume",
|
||||
"end_reason",
|
||||
"reported_time",
|
||||
"background_audio_active",
|
||||
"background_audio_base",
|
||||
"html5_playing",
|
||||
"autoplay_settings",
|
||||
"autoplay_episode_count",
|
||||
"reports",
|
||||
"event_emitter",
|
||||
];
|
||||
|
||||
/// Rank of `field` in [`LOCK_ORDER`], or `None` if it is not a declared lock.
|
||||
pub fn rank(field: &str) -> Option<usize> {
|
||||
LOCK_ORDER.iter().position(|f| *f == field)
|
||||
}
|
||||
|
||||
/// One lock acquired while another is still held.
|
||||
#[derive(Debug, PartialEq, Eq)]
|
||||
pub struct Overlap {
|
||||
/// The lock already held.
|
||||
pub outer: String,
|
||||
/// The lock acquired underneath it.
|
||||
pub inner: String,
|
||||
/// 1-indexed line of the inner acquisition, for a useful failure message.
|
||||
pub line: usize,
|
||||
}
|
||||
|
||||
/// Find every place `src` takes a lock while holding another.
|
||||
///
|
||||
/// Deliberately simple and line-based: it tracks `let … = self.FIELD.lock_safe()`
|
||||
/// bindings and looks for a different `self.OTHER.lock_safe()` before the
|
||||
/// binding goes out of scope or is explicitly dropped. A guard that is not bound
|
||||
/// to a name (`*self.flag.lock_safe() = false;`) is released at the end of its
|
||||
/// statement and cannot overlap anything, so it is only ever an *inner*
|
||||
/// acquisition here.
|
||||
///
|
||||
/// TRACES: UR-005 | DR-052 | UT-052
|
||||
pub fn overlapping_acquisitions(src: &str) -> Vec<Overlap> {
|
||||
let lines: Vec<&str> = src.lines().collect();
|
||||
let mut found = Vec::new();
|
||||
|
||||
for (i, line) in lines.iter().enumerate() {
|
||||
let Some((var, field)) = parse_binding(line) else {
|
||||
continue;
|
||||
};
|
||||
let indent = line.len() - line.trim_start().len();
|
||||
|
||||
for (j, later) in lines.iter().enumerate().skip(i + 1) {
|
||||
if later.contains(&format!("drop({var})")) {
|
||||
break;
|
||||
}
|
||||
let trimmed = later.trim();
|
||||
if trimmed.is_empty() {
|
||||
continue;
|
||||
}
|
||||
// Left the block the guard lives in.
|
||||
let later_indent = later.len() - later.trim_start().len();
|
||||
if later_indent < indent && !trimmed.starts_with(['.', ')', '}']) {
|
||||
break;
|
||||
}
|
||||
if trimmed == "}" && later_indent < indent {
|
||||
break;
|
||||
}
|
||||
if let Some(inner) = parse_acquisition(later, field) {
|
||||
found.push(Overlap {
|
||||
outer: field.to_string(),
|
||||
inner,
|
||||
line: j + 1,
|
||||
});
|
||||
break;
|
||||
}
|
||||
}
|
||||
}
|
||||
found
|
||||
}
|
||||
|
||||
/// `let [mut] name = self.field.lock_safe()` → `(name, field)`.
|
||||
fn parse_binding(line: &str) -> Option<(&str, &str)> {
|
||||
let rest = line.trim_start().strip_prefix("let ")?;
|
||||
let rest = rest.strip_prefix("mut ").unwrap_or(rest);
|
||||
let (name, rest) = rest.split_once(" = self.")?;
|
||||
let (field, _) = rest.split_once(".lock_safe()")?;
|
||||
if name.contains(' ') || field.contains('.') {
|
||||
return None;
|
||||
}
|
||||
Some((name, field))
|
||||
}
|
||||
|
||||
/// The first `self.other.lock_safe()` on `line` that is not `held`.
|
||||
fn parse_acquisition(line: &str, held: &str) -> Option<String> {
|
||||
let mut search = line;
|
||||
while let Some(at) = search.find("self.") {
|
||||
let after = &search[at + 5..];
|
||||
if let Some((field, _)) = after.split_once(".lock_safe()") {
|
||||
if !field.contains(['.', '(', ' ']) && field != held {
|
||||
return Some(field.to_string());
|
||||
}
|
||||
}
|
||||
search = after;
|
||||
}
|
||||
None
|
||||
}
|
||||
|
||||
// TRACES: UR-005 | DR-052 | UT-052
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// **The tripwire.** Every nested acquisition in the controller must follow
|
||||
/// [`LOCK_ORDER`].
|
||||
///
|
||||
/// A violation is a lock-order inversion: two threads taking the same pair
|
||||
/// in opposite orders deadlock the player, and the symptom is a frozen app
|
||||
/// with no error anywhere.
|
||||
///
|
||||
/// TRACES: UR-005 | DR-052 | UT-052
|
||||
#[test]
|
||||
fn every_overlapping_acquisition_respects_the_order() {
|
||||
let src = include_str!("mod.rs");
|
||||
let mut violations = Vec::new();
|
||||
|
||||
for overlap in overlapping_acquisitions(src) {
|
||||
let (Some(outer), Some(inner)) = (rank(&overlap.outer), rank(&overlap.inner)) else {
|
||||
violations.push(format!(
|
||||
"player/mod.rs:{} takes '{}' while holding '{}', and one of them \
|
||||
is not declared in LOCK_ORDER",
|
||||
overlap.line, overlap.inner, overlap.outer
|
||||
));
|
||||
continue;
|
||||
};
|
||||
if outer >= inner {
|
||||
violations.push(format!(
|
||||
"player/mod.rs:{} takes '{}' (rank {inner}) while holding '{}' \
|
||||
(rank {outer}) — an inversion against LOCK_ORDER",
|
||||
overlap.line, overlap.inner, overlap.outer
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
assert!(
|
||||
violations.is_empty(),
|
||||
"lock-order inversions in PlayerController:\n {}\n\nEither reorder the \
|
||||
acquisitions or, if the new order is the correct one, change LOCK_ORDER \
|
||||
and re-check every other site.",
|
||||
violations.join("\n ")
|
||||
);
|
||||
}
|
||||
|
||||
/// The analysis must actually see the nesting the controller does today,
|
||||
/// or the tripwire above passes by finding nothing.
|
||||
///
|
||||
/// TRACES: UR-005 | DR-052 | UT-052
|
||||
#[test]
|
||||
fn the_analysis_finds_the_nesting_that_exists() {
|
||||
let found = overlapping_acquisitions(include_str!("mod.rs"));
|
||||
assert!(
|
||||
found.len() >= 5,
|
||||
"expected the controller's known nested acquisitions, found {found:?}"
|
||||
);
|
||||
assert!(
|
||||
found
|
||||
.iter()
|
||||
.any(|o| o.outer == "backend" && o.inner == "queue"),
|
||||
"the backend->queue nesting in state() should be detected: {found:?}"
|
||||
);
|
||||
}
|
||||
|
||||
/// A guard held across a lock taken in the wrong order must be caught.
|
||||
///
|
||||
/// TRACES: UR-005 | DR-052 | UT-052
|
||||
#[test]
|
||||
fn an_inversion_is_detected() {
|
||||
let src = " fn bad(&self) {\n\
|
||||
\x20 let queue = self.queue.lock_safe();\n\
|
||||
\x20 let b = self.backend.lock_safe();\n\
|
||||
\x20 }\n";
|
||||
let found = overlapping_acquisitions(src);
|
||||
assert_eq!(found.len(), 1, "{found:?}");
|
||||
assert_eq!(found[0].outer, "queue");
|
||||
assert_eq!(found[0].inner, "backend");
|
||||
assert!(rank("queue").unwrap() > rank("backend").unwrap());
|
||||
}
|
||||
|
||||
/// Sequential, non-overlapping acquisitions cannot deadlock and must not be
|
||||
/// reported — `previous()` drops the backend guard before taking the queue.
|
||||
///
|
||||
/// TRACES: UR-005 | DR-052 | UT-052
|
||||
#[test]
|
||||
fn a_dropped_guard_is_not_an_overlap() {
|
||||
let src = " fn fine(&self) {\n\
|
||||
\x20 let backend = self.backend.lock_safe();\n\
|
||||
\x20 drop(backend);\n\
|
||||
\x20 let queue = self.queue.lock_safe();\n\
|
||||
\x20 }\n";
|
||||
assert!(overlapping_acquisitions(src).is_empty());
|
||||
}
|
||||
|
||||
/// Every declared lock name must be a real field, or the order documents
|
||||
/// something that no longer exists.
|
||||
///
|
||||
/// TRACES: UR-005 | DR-052 | UT-052
|
||||
#[test]
|
||||
fn every_declared_lock_is_a_real_field() {
|
||||
let src = include_str!("mod.rs");
|
||||
let decl = src
|
||||
.split_once("pub struct PlayerController {")
|
||||
.expect("PlayerController struct")
|
||||
.1;
|
||||
let decl = decl.split_once("\n}").expect("end of struct").0;
|
||||
|
||||
for name in LOCK_ORDER {
|
||||
assert!(
|
||||
decl.contains(&format!("{name}:")),
|
||||
"LOCK_ORDER names '{name}', which is not a PlayerController field"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -28,6 +28,10 @@ pub mod track_switch;
|
||||
#[cfg(test)]
|
||||
mod mpv_backend_test;
|
||||
|
||||
// The declared lock hierarchy for `PlayerController` below, and the tripwire
|
||||
// that enforces it. See the module docs for why seventeen mutexes need one.
|
||||
pub mod lock_order;
|
||||
|
||||
// Panic containment for the JNI boundary. Not gated on the target: the guard
|
||||
// and its tripwire test are exercised on the host, where `android` never builds.
|
||||
pub mod jni_guard;
|
||||
|
||||
Reference in New Issue
Block a user