diff --git a/Blackboard/Done/fix-backend-mutex-poisoned.md b/Blackboard/Done/fix-backend-mutex-poisoned.md new file mode 100644 index 00000000..6e2da5a2 --- /dev/null +++ b/Blackboard/Done/fix-backend-mutex-poisoned.md @@ -0,0 +1,78 @@ +# Fix: Backend Mutex Poisoned Panic sur OpenHome Stop + +**Statut** : Terminé + +## Problème initial + +Lors de l'arrêt de la lecture sur un lecteur OpenHome, l'erreur suivante apparaissait : + +``` +Impossible d'arrêter la lecture: Internal task error: task 66013 panicked with message "Backend mutex poisoned: PoisonError { .. }" +``` + +## Crate concernée + +- **pmocontrol** (`pmocontrol/src/`) + +## Analyse de la cause racine + +### Round 1 : Mutex empoisonné + +Le mutex backend était empoisonné par des panics non gérés lors d'opérations sur les renderers. Les appels `.unwrap()` et `.expect()` sur le mutex propageaient les panics au lieu de les gérer gracieusement. + +### Round 2 : Régression révélée par Round 1 + +Les correctifs du Round 1 ont révélé un problème plus profond. La chaîne d'échecs était : + +1. **DeleteAll échoue avec erreur 501** : Le renderer OpenHome rejette l'action `DeleteAll` pendant la lecture active +2. **Le code continue** (grâce aux correctifs Round 1 qui tolèrent les erreurs) +3. **État incohérent du renderer** : OpenHome retourne 5 IDs via `IdArray` mais une `` vide via `ReadList` +4. **Panic "index out of bounds"** : `sync_queue()` accède à `items[4]` alors que `items.len() == 0` +5. **Mutex empoisonné** : Le panic dans le thread empoisonne le mutex + +Preuve dans les logs : +``` +OpenHome Playlist IdArray returned ... id_count=5 +OpenHome Playlist tracks read ... track_count=0 expected_count=5 +``` + +## Corrections apportées + +### 1. Tolérance des erreurs clear_queue + +**Fichier** : `pmocontrol/src/music_renderer/musicrenderer.rs` + +La méthode `clear_for_playlist_attach()` tolère maintenant les erreurs de `clear_queue()` au lieu de propager l'erreur. Le `DeleteAll` n'est pas critique car `sync_queue()` remplacera de toute façon le contenu de la queue. + +### 2. Suppression du clear_queue redondant + +**Fichier** : `pmocontrol/src/control_point.rs` + +Suppression de l'appel `renderer.clear_queue()?` dans `attach_queue_to_playlist_internal()`. Ce `clear_queue()` était redondant car `clear_for_playlist_attach()` le fait déjà, et causait un second échec `DeleteAll`. + +### 3. Bounds-check pour current_index + +**Fichier** : `pmocontrol/src/queue/openhome.rs` + +Ajout d'une vérification de bornes dans `sync_queue()` pour gérer l'état incohérent du renderer OpenHome. Gère le cas où le renderer retourne un état incohérent (IDs sans données de track correspondantes). + +## Fichiers modifiés + +| Fichier | Modification | +|---------|-------------| +| `pmocontrol/src/music_renderer/musicrenderer.rs` | Tolérance des erreurs `clear_queue()` dans `clear_for_playlist_attach()` | +| `pmocontrol/src/control_point.rs` | Suppression du `clear_queue()` redondant | +| `pmocontrol/src/queue/openhome.rs` | Bounds-check pour `current_index` + import `warn` | + +## Comportement après correction + +1. **DeleteAll échoue** : Warning loggé, le code continue +2. **État incohérent détecté** : Warning loggé, traité comme "pas de track courante" +3. **sync_queue réussit** : La playlist est correctement attachée au renderer +4. **Pas de panic** : Le mutex reste sain + +## Leçons apprises + +La correction d'erreurs (Round 1) peut révéler des bugs latents. Le code supposait que l'état du renderer OpenHome était toujours cohérent. En réalité, certains renderers peuvent retourner des IDs de tracks sans les données correspondantes, notamment lorsqu'une opération `DeleteAll` est rejetée pendant la lecture. + +**Approche défensive adoptée** : Plutôt que de supposer un état cohérent, le code vérifie les bornes et traite les incohérences comme des cas dégradés plutôt que de paniquer. diff --git a/Blackboard/Report/fix-backend-mutex-poisoned.md b/Blackboard/Report/fix-backend-mutex-poisoned.md new file mode 100644 index 00000000..2879cd4a --- /dev/null +++ b/Blackboard/Report/fix-backend-mutex-poisoned.md @@ -0,0 +1,152 @@ +# Rapport: Fix Backend Mutex Poisoned + +## Résumé + +Ce rapport documente la correction du bug "Backend mutex poisoned" qui se manifestait lors de l'arrêt de la lecture sur un renderer OpenHome, ainsi que la régression Round 2 découverte après les premiers correctifs. + +## Analyse de la cause racine + +### Round 1 : Mutex empoisonné + +Le mutex backend était empoisonné par des panics non gérés lors d'opérations sur les renderers. Les appels `.unwrap()` et `.expect()` sur le mutex propageaient les panics au lieu de les gérer gracieusement. + +### Round 2 : Régression après Round 1 + +Les correctifs du Round 1 ont révélé un problème plus profond. La chaîne d'échecs était : + +1. **DeleteAll échoue avec erreur 501** : Le renderer OpenHome rejette l'action `DeleteAll` pendant la lecture active +2. **Le code continue** (grâce aux correctifs Round 1 qui tolèrent les erreurs) +3. **État incohérent du renderer** : OpenHome retourne 5 IDs via `IdArray` mais une `` vide via `ReadList` +4. **Panic "index out of bounds"** : `sync_queue()` accède à `items[4]` alors que `items.len() == 0` +5. **Mutex empoisonné** : Le panic dans le thread empoisonne le mutex + +Preuve dans les logs : +``` +OpenHome Playlist IdArray returned ... id_count=5 +OpenHome Playlist tracks read ... track_count=0 expected_count=5 +``` + +## Corrections apportées + +### 1. Tolérance des erreurs clear_queue (`musicrenderer.rs`) + +**Fichier** : `pmocontrol/src/music_renderer/musicrenderer.rs` + +**Modification** : La méthode `clear_for_playlist_attach()` tolère maintenant les erreurs de `clear_queue()` au lieu de propager l'erreur. + +```rust +pub fn clear_for_playlist_attach(&self) -> Result<(), ControlPointError> { + let mut backend = self.lock_backend_for("clear_for_playlist_attach"); + + // Clear the queue first (ignore errors - queue will be replaced anyway by sync_queue) + // Some backends (OpenHome) may reject DeleteAll if currently playing + if let Err(err) = backend.clear_queue() { + warn!( + renderer = self.id().0.as_str(), + error = %err, + "Clear queue failed when preparing for playlist attach (continuing anyway)" + ); + } + + // Then stop playback (ignore errors if already stopped) + backend.stop().or_else(|err| { + warn!( + renderer = self.id().0.as_str(), + error = %err, + "Stop failed when preparing for playlist attach (continuing anyway)" + ); + Ok(()) + }) +} +``` + +**Justification** : Le `DeleteAll` n'est pas critique car `sync_queue()` remplacera de toute façon le contenu de la queue. + +### 2. Suppression du clear_queue redondant (`control_point.rs`) + +**Fichier** : `pmocontrol/src/control_point.rs` + +**Modification** : Suppression de l'appel `renderer.clear_queue()?` dans `attach_queue_to_playlist_internal()`. + +Avant : +```rust +// Clear the local queue (detach binding + clear runtime queue structure) +self.detach_playlist_binding(renderer_id, "attach_new_playlist"); +renderer.clear_queue()?; +``` + +Après : +```rust +// Detach any existing binding (local queue will be replaced by sync_queue later) +self.detach_playlist_binding(renderer_id, "attach_new_playlist"); +``` + +**Justification** : Ce `clear_queue()` était redondant car `clear_for_playlist_attach()` le fait déjà, et causait un second échec `DeleteAll`. + +### 3. Bounds-check pour current_index (`openhome.rs`) + +**Fichier** : `pmocontrol/src/queue/openhome.rs` + +**Modification** : Ajout d'une vérification de bornes dans `sync_queue()` pour gérer l'état incohérent du renderer OpenHome. + +```rust +let snapshot = self.queue_snapshot()?; +// Note: current_index may point to an index that doesn't exist in items +// if the OpenHome renderer is in an inconsistent state (e.g., IdArray returns +// IDs but ReadList returns empty TrackList). We must bounds-check here. +let playing_info = snapshot.current_index.and_then(|idx| { + if idx < snapshot.items.len() { + Some(( + idx, + snapshot.items[idx].backend_id, + snapshot.items[idx].uri.clone(), + snapshot.items[idx].didl_id.clone(), + )) + } else { + warn!( + renderer = self.renderer_id.0.as_str(), + current_index = idx, + items_len = snapshot.items.len(), + "OpenHome renderer in inconsistent state: current_index out of bounds, treating as no current track" + ); + None + } +}); +``` + +**Justification** : Gère le cas où le renderer OpenHome retourne un état incohérent (IDs sans données de track correspondantes). + +### 4. Ajout de l'import warn (`openhome.rs`) + +**Fichier** : `pmocontrol/src/queue/openhome.rs` + +**Modification** : Ajout de `warn` à l'import tracing. + +```rust +use tracing::{debug, warn}; +``` + +## Fichiers modifiés + +| Fichier | Modification | +|---------|-------------| +| `pmocontrol/src/music_renderer/musicrenderer.rs` | Tolérance des erreurs `clear_queue()` dans `clear_for_playlist_attach()` | +| `pmocontrol/src/control_point.rs` | Suppression du `clear_queue()` redondant | +| `pmocontrol/src/queue/openhome.rs` | Bounds-check + import `warn` | + +## Comportement attendu après correction + +1. **DeleteAll échoue** : Warning loggé, le code continue +2. **État incohérent détecté** : Warning loggé, traité comme "pas de track courante" +3. **sync_queue réussit** : La playlist est correctement attachée au renderer +4. **Pas de panic** : Le mutex reste sain + +## Tests effectués + +- L'utilisateur a confirmé que la correction fonctionne ("Ok ça marche") + +## Notes techniques + +Les correctifs du Round 1 (gestion d'erreur sur mutex) ont révélé un bug préexistant : le code supposait que l'état du renderer OpenHome était toujours cohérent. En réalité, certains renderers peuvent retourner des IDs de tracks sans les données correspondantes, notamment lorsqu'une opération `DeleteAll` est rejetée pendant la lecture. + +La solution adoptée est défensive : plutôt que de supposer un état cohérent, le code vérifie les bornes et traite les incohérences comme des cas dégradés (pas de track courante) plutôt que de paniquer. diff --git a/Cargo.lock b/Cargo.lock index 0fdd53a8..4497244d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4,7 +4,7 @@ version = 4 [[package]] name = "PMOMusic" -version = "0.3.7" +version = "0.3.8" dependencies = [ "axum 0.8.7", "console-subscriber", diff --git a/PMOMusic/Cargo.toml b/PMOMusic/Cargo.toml index 88936a2b..71f7bba4 100644 --- a/PMOMusic/Cargo.toml +++ b/PMOMusic/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "PMOMusic" -version = "0.3.7" +version = "0.3.8" edition = "2024" [dependencies] diff --git a/pmocontrol/src/control_point.rs b/pmocontrol/src/control_point.rs index adca73c0..1c534779 100644 --- a/pmocontrol/src/control_point.rs +++ b/pmocontrol/src/control_point.rs @@ -1117,13 +1117,12 @@ impl ControlPoint { // Sync backend state to local cache (backend-agnostic) renderer.sync_queue_state()?; - // Clear the local queue (detach binding + clear runtime queue structure) + // Detach any existing binding (local queue will be replaced by sync_queue later) self.detach_playlist_binding(renderer_id, "attach_new_playlist"); - renderer.clear_queue()?; debug!( renderer = renderer_id.0.as_str(), - "Cleared renderer and local queue for new playlist" + "Prepared renderer for new playlist attachment" ); let binding = PlaylistBinding { diff --git a/pmocontrol/src/music_renderer/musicrenderer.rs b/pmocontrol/src/music_renderer/musicrenderer.rs index 7a6de226..a0df13c9 100644 --- a/pmocontrol/src/music_renderer/musicrenderer.rs +++ b/pmocontrol/src/music_renderer/musicrenderer.rs @@ -223,7 +223,16 @@ impl MusicRenderer { // Determine the watch strategy based on backend type let strategy = { - let backend = self.backend.lock().expect("Backend mutex poisoned"); + let backend = match self.backend.lock() { + Ok(guard) => guard, + Err(poisoned) => { + warn!( + renderer = self.info.friendly_name(), + "Backend mutex poisoned when starting watcher, recovering" + ); + poisoned.into_inner() + } + }; WatchStrategy::for_backend(&*backend) }; @@ -436,13 +445,28 @@ impl MusicRenderer { renderer = self.info.friendly_name(), "Renderer stopped after queue-driven playback; advancing" ); - if let Err(err) = self.play_next_from_queue() { - error!( - renderer = self.info.friendly_name(), - error = %err, - "Auto-advance failed; clearing queue playback state" - ); - self.set_playback_source(PlaybackSource::None); + // Use catch_unwind to prevent panics in play_next_from_queue + // from poisoning the backend mutex + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + self.play_next_from_queue() + })); + match result { + Ok(Ok(())) => {} + Ok(Err(err)) => { + error!( + renderer = self.info.friendly_name(), + error = %err, + "Auto-advance failed; clearing queue playback state" + ); + self.set_playback_source(PlaybackSource::None); + } + Err(_panic) => { + error!( + renderer = self.info.friendly_name(), + "Auto-advance panicked; clearing queue playback state" + ); + self.set_playback_source(PlaybackSource::None); + } } } else { debug!( @@ -464,6 +488,34 @@ impl MusicRenderer { } } + /// Acquires the backend mutex, recovering from poisoned state if necessary. + /// + /// This method handles the case where the mutex was poisoned by a panic in another thread. + /// Instead of panicking, it recovers the inner data and logs a warning with context. + /// + /// # Arguments + /// * `context` - A description of the operation attempting to acquire the lock + fn lock_backend_for(&self, context: &str) -> std::sync::MutexGuard<'_, MusicRendererBackend> { + match self.backend.lock() { + Ok(guard) => guard, + Err(poisoned) => { + warn!( + renderer = self.info.friendly_name(), + context = context, + "Backend mutex was poisoned, recovering" + ); + poisoned.into_inner() + } + } + } + + /// Acquires the backend mutex with a default context message. + /// + /// Convenience wrapper around `lock_backend_for` for simple cases. + fn lock_backend(&self) -> std::sync::MutexGuard<'_, MusicRendererBackend> { + self.lock_backend_for("unknown operation") + } + pub fn info(&self) -> &RendererInfo { &self.info } @@ -474,42 +526,42 @@ impl MusicRenderer { } pub fn is_upnp(&self) -> bool { - match &*self.backend.lock().expect("Backend mutex poisoned") { + match &*self.lock_backend_for("is_upnp") { MusicRendererBackend::Upnp(_) => true, _ => false, } } pub fn is_openhome(&self) -> bool { - match &*self.backend.lock().expect("Backend mutex poisoned") { + match &*self.lock_backend_for("is_openhome") { MusicRendererBackend::OpenHome(_) => true, _ => false, } } pub fn is_linkplay(&self) -> bool { - match &*self.backend.lock().expect("Backend mutex poisoned") { + match &*self.lock_backend_for("is_linkplay") { MusicRendererBackend::LinkPlay(_) => true, _ => false, } } pub fn is_arylictcp(&self) -> bool { - match &*self.backend.lock().expect("Backend mutex poisoned") { + match &*self.lock_backend_for("is_arylictcp") { MusicRendererBackend::ArylicTcp(_) => true, _ => false, } } pub fn is_chromecast(&self) -> bool { - match &*self.backend.lock().expect("Backend mutex poisoned") { + match &*self.lock_backend_for("is_chromecast") { MusicRendererBackend::Chromecast(_) => true, _ => false, } } pub fn is_hybridupnparylic(&self) -> bool { - match &*self.backend.lock().expect("Backend mutex poisoned") { + match &*self.lock_backend_for("is_hybridupnparylic") { MusicRendererBackend::HybridUpnpArylic { .. } => true, _ => false, } @@ -522,34 +574,32 @@ impl MusicRenderer { /// Prepare the renderer for attaching a new playlist by clearing the queue and stopping playback. pub fn clear_for_playlist_attach(&self) -> Result<(), ControlPointError> { - // Clear the queue first - self.backend - .lock() - .expect("Backend mutex poisoned") - .clear_queue()?; + let mut backend = self.lock_backend_for("clear_for_playlist_attach"); + + // Clear the queue first (ignore errors - queue will be replaced anyway by sync_queue) + // Some backends (OpenHome) may reject DeleteAll if currently playing + if let Err(err) = backend.clear_queue() { + warn!( + renderer = self.id().0.as_str(), + error = %err, + "Clear queue failed when preparing for playlist attach (continuing anyway)" + ); + } // Then stop playback (ignore errors if already stopped) - self.backend - .lock() - .expect("Backend mutex poisoned") - .stop() - .or_else(|err| { - warn!( - renderer = self.id().0.as_str(), - error = %err, - "Stop failed when preparing for playlist attach (continuing anyway)" - ); - Ok(()) - }) + backend.stop().or_else(|err| { + warn!( + renderer = self.id().0.as_str(), + error = %err, + "Stop failed when preparing for playlist attach (continuing anyway)" + ); + Ok(()) + }) } /// Get the current queue snapshot. pub fn queue_snapshot(&self) -> Result { - let mut snapshot = self - .backend - .lock() - .expect("Backend mutex poisoned") - .queue_snapshot()?; + let mut snapshot = self.lock_backend_for("queue_snapshot").queue_snapshot()?; // Enrich snapshot with playlist_id from binding if available if let Some(binding) = self @@ -568,18 +618,12 @@ impl MusicRenderer { /// Get the current queue item without advancing. /// Returns the item and count of remaining items after current. pub fn peek_current(&self) -> Result, ControlPointError> { - self.backend - .lock() - .expect("Backend mutex poisoned") - .peek_current() + self.lock_backend_for("peek_current").peek_current() } /// Get the count of items remaining after the current index. pub fn upcoming_len(&self) -> Result { - self.backend - .lock() - .expect("Backend mutex poisoned") - .upcoming_len() + self.lock_backend_for("upcoming_len").upcoming_len() } /// Play the current item from the queue. @@ -589,9 +633,7 @@ impl MusicRenderer { // The flag will be set back to true when PLAYING state is detected. self.clear_has_played_flag(); - self.backend - .lock() - .expect("Backend mutex poisoned") + self.lock_backend_for("play_current_from_queue") .play_from_queue() } @@ -602,10 +644,7 @@ impl MusicRenderer { // The flag will be set back to true when PLAYING state is detected. self.clear_has_played_flag(); - self.backend - .lock() - .expect("Backend mutex poisoned") - .play_next()?; + self.lock_backend_for("play_next_from_queue").play_next()?; self.emit_queue_updated(); Ok(()) } @@ -617,9 +656,7 @@ impl MusicRenderer { // The flag will be set back to true when PLAYING state is detected. self.clear_has_played_flag(); - self.backend - .lock() - .expect("Backend mutex poisoned") + self.lock_backend_for("play_from_index") .play_from_index(index)?; self.emit_queue_updated(); Ok(()) @@ -631,7 +668,7 @@ impl MusicRenderer { /// joue le track courant de la queue automatiquement (comportement unifié pour tous les backends). pub fn play(&self) -> Result<(), ControlPointError> { // Vérifier si on a une queue non vide - let backend = self.backend.lock().expect("Backend mutex poisoned"); + let backend = self.lock_backend_for("play"); let queue_not_empty = backend.len().unwrap_or(0) > 0; if queue_not_empty { @@ -646,7 +683,7 @@ impl MusicRenderer { /// Transport control: pause pub fn pause(&self) -> Result<(), ControlPointError> { - self.backend.lock().expect("Backend mutex poisoned").pause() + self.lock_backend_for("pause").pause() } /// Transport control: stop @@ -657,7 +694,7 @@ impl MusicRenderer { // transient STOPPED states during track initialization. self.clear_has_played_flag(); - self.backend.lock().expect("Backend mutex poisoned").stop() + self.lock_backend_for("stop").stop() } /// Set the next URI for gapless playback (UPnP AVTransport only). @@ -665,7 +702,7 @@ impl MusicRenderer { /// Returns Ok if the backend supports this feature and it succeeded. /// Returns Err if not supported or if it failed. pub fn set_next_uri(&self, uri: &str, metadata: &str) -> Result<(), ControlPointError> { - let backend = self.backend.lock().expect("Backend mutex poisoned"); + let backend = self.lock_backend_for("set_next_uri"); match &*backend { MusicRendererBackend::Upnp(upnp) => upnp.set_next_uri(uri, metadata), @@ -678,10 +715,7 @@ impl MusicRenderer { /// Transport control: seek to relative time pub fn seek_rel_time(&self, hhmmss: &str) -> Result<(), ControlPointError> { - self.backend - .lock() - .expect("Backend mutex poisoned") - .seek_rel_time(hhmmss) + self.lock_backend_for("seek_rel_time").seek_rel_time(hhmmss) } /// Seek to a specific position in seconds @@ -696,46 +730,32 @@ impl MusicRenderer { /// Volume control: get current volume pub fn volume(&self) -> Result { - self.backend - .lock() - .expect("Backend mutex poisoned") - .volume() + self.lock_backend_for("volume").volume() } /// Volume control: set volume pub fn set_volume(&self, vol: u16) -> Result<(), ControlPointError> { - self.backend - .lock() - .expect("Backend mutex poisoned") - .set_volume(vol) + self.lock_backend_for("set_volume").set_volume(vol) } /// Volume control: get mute state pub fn mute(&self) -> Result { - self.backend.lock().expect("Backend mutex poisoned").mute() + self.lock_backend_for("mute").mute() } /// Volume control: set mute state pub fn set_mute(&self, m: bool) -> Result<(), ControlPointError> { - self.backend - .lock() - .expect("Backend mutex poisoned") - .set_mute(m) + self.lock_backend_for("set_mute").set_mute(m) } /// Get playback state pub fn playback_state(&self) -> Result { - self.backend - .lock() - .expect("Backend mutex poisoned") - .playback_state() + self.lock_backend_for("playback_state").playback_state() } /// Get playback position pub fn playback_position(&self) -> Result { - self.backend - .lock() - .expect("Backend mutex poisoned") + self.lock_backend_for("playback_position") .playback_position() } @@ -869,8 +889,7 @@ impl MusicRenderer { /// Returns the number of items in the queue. pub fn len(&self) -> Result { - let backend = self.backend.lock().expect("Backend mutex poisoned"); - backend.len() + self.lock_backend_for("len").len() } /// Add items to the queue using the specified enqueue mode. @@ -879,7 +898,7 @@ impl MusicRenderer { items: Vec, mode: EnqueueMode, ) -> Result<(), ControlPointError> { - let mut backend = self.backend.lock().expect("Backend mutex poisoned"); + let mut backend = self.lock_backend_for("enqueue_items"); backend.enqueue_items(items, mode)?; drop(backend); self.emit_queue_updated(); @@ -893,7 +912,7 @@ impl MusicRenderer { /// - If the current track is NOT in the new items, it's preserved as the first item /// - If there's no current track, the queue is simply replaced pub fn sync_queue(&self, items: Vec) -> Result<(), ControlPointError> { - let mut backend = self.backend.lock().expect("Backend mutex poisoned"); + let mut backend = self.lock_backend_for("sync_queue"); backend.sync_queue(items)?; drop(backend); self.emit_queue_updated(); @@ -903,13 +922,12 @@ impl MusicRenderer { /// Set the current queue index (for advanced use). /// Note: This does NOT start playback. Use select_queue_track() to play. pub fn set_queue_index(&self, index: Option) -> Result<(), ControlPointError> { - let mut backend = self.backend.lock().expect("Backend mutex poisoned"); - backend.set_index(index) + self.lock_backend_for("set_queue_index").set_index(index) } /// Clears the renderer's queue using the generic QueueBackend trait. pub fn clear_queue(&self) -> Result<(), ControlPointError> { - let mut backend = self.backend.lock().expect("Backend mutex poisoned"); + let mut backend = self.lock_backend_for("clear_queue"); backend.clear_queue()?; drop(backend); self.emit_queue_updated(); @@ -918,14 +936,12 @@ impl MusicRenderer { /// Dequeues and returns the next item from the queue. pub fn dequeue_next(&self) -> Result, ControlPointError> { - let mut backend = self.backend.lock().expect("Backend mutex poisoned"); - backend.dequeue_next() + self.lock_backend_for("dequeue_next").dequeue_next() } /// Sets the current index in the queue. pub fn set_index(&self, index: Option) -> Result<(), ControlPointError> { - let mut backend = self.backend.lock().expect("Backend mutex poisoned"); - backend.set_index(index) + self.lock_backend_for("set_index").set_index(index) } /// Replaces the entire queue with new items and sets the current index. @@ -935,7 +951,7 @@ impl MusicRenderer { items: Vec, current_index: Option, ) -> Result<(), ControlPointError> { - let mut backend = self.backend.lock().expect("Backend mutex poisoned"); + let mut backend = self.lock_backend_for("replace_queue"); backend.replace_queue(items, current_index)?; drop(backend); self.emit_queue_updated(); @@ -967,16 +983,20 @@ impl MusicRenderer { /// Selects and plays a specific track from the queue by ID. /// /// Converts the track ID to a position using the generic QueueBackend trait, - /// then plays the track. + /// then plays the track. The operation is atomic (single lock). pub fn select_queue_track(&self, track_id: u32) -> Result<(), ControlPointError> { - // Convert track_id to index - let index = { - let backend = self.backend.lock().expect("Backend mutex poisoned"); - backend.id_to_position(track_id)? - }; + // Reset the has_played flag before starting playback + self.clear_has_played_flag(); - // Play from that index - self.play_from_index(index) + let backend = self.lock_backend_for("select_queue_track"); + + // Convert track_id to index and play atomically + let index = backend.id_to_position(track_id)?; + backend.play_from_index(index)?; + + drop(backend); + self.emit_queue_updated(); + Ok(()) } /// Synchronizes the queue state with the backend. @@ -984,9 +1004,8 @@ impl MusicRenderer { /// For backends with persistent queues (OpenHome), this refreshes the local view. /// For others, this is essentially a no-op (just reads the current state). pub fn sync_queue_state(&self) -> Result<(), ControlPointError> { - let backend = self.backend.lock().expect("Backend mutex poisoned"); // Calling queue_snapshot() triggers a refresh for backends that need it - let _ = backend.queue_snapshot()?; + let _ = self.lock_backend_for("sync_queue_state").queue_snapshot()?; Ok(()) } @@ -994,17 +1013,24 @@ impl MusicRenderer { /// /// This is primarily for backends with persistent queues (OpenHome). pub fn play_current_from_backend_queue(&self) -> Result<(), ControlPointError> { - let backend = self.backend.lock().expect("Backend mutex poisoned"); + // Reset the has_played flag before starting playback + self.clear_has_played_flag(); - // Get current track ID using generic QueueBackend trait + let backend = self.lock_backend_for("play_current_from_backend_queue"); + + // Get current track ID and convert to index atomically let track_id = backend .current_track()? .ok_or_else(|| ControlPointError::QueueError("No current track".to_string()))?; - drop(backend); + let index = backend.id_to_position(track_id)?; - // Play it using select_queue_track - self.select_queue_track(track_id) + // Play from that index while still holding the lock + backend.play_from_index(index)?; + + drop(backend); + self.emit_queue_updated(); + Ok(()) } /// Plays from the queue at the current position. @@ -1016,8 +1042,7 @@ impl MusicRenderer { // The flag will be set back to true when PLAYING state is detected. self.clear_has_played_flag(); - let backend = self.backend.lock().expect("Backend mutex poisoned"); - backend.play_from_queue() + self.lock_backend_for("play_from_queue").play_from_queue() } // --- Playback State Management --- diff --git a/pmocontrol/src/music_renderer/openhome_renderer.rs b/pmocontrol/src/music_renderer/openhome_renderer.rs index 1a818e45..e4cba46a 100644 --- a/pmocontrol/src/music_renderer/openhome_renderer.rs +++ b/pmocontrol/src/music_renderer/openhome_renderer.rs @@ -351,12 +351,17 @@ pub(crate) fn map_openhome_state(raw: &str) -> PlaybackState { impl QueueTransportControl for OpenHomeRenderer { fn play_from_queue(&self) -> Result<(), ControlPointError> { { - let queue = self.queue.lock().unwrap(); + let queue = self + .queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))?; if queue.current_index()?.is_none() { if queue.len()? > 0 { drop(queue); - let mut queue = self.queue.lock().unwrap(); + let mut queue = self.queue.lock().map_err(|_| { + ControlPointError::QueueError("Queue mutex poisoned".into()) + })?; queue.set_index(Some(0))?; } else { return Err(ControlPointError::QueueError("Queue is empty".into())); @@ -370,7 +375,10 @@ impl QueueTransportControl for OpenHomeRenderer { fn play_next(&self) -> Result<(), ControlPointError> { { - let mut queue = self.queue.lock().unwrap(); + let mut queue = self + .queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))?; if !queue.advance()? { return Err(ControlPointError::QueueError("No next track".into())); } @@ -381,7 +389,10 @@ impl QueueTransportControl for OpenHomeRenderer { fn play_previous(&self) -> Result<(), ControlPointError> { { - let mut queue = self.queue.lock().unwrap(); + let mut queue = self + .queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))?; if !queue.rewind()? { return Err(ControlPointError::QueueError("No previous track".into())); } @@ -393,7 +404,10 @@ impl QueueTransportControl for OpenHomeRenderer { fn play_from_index(&self, index: usize) -> Result<(), ControlPointError> { // For OpenHome, we need to convert index to track_id let track_id = { - let queue = self.queue.lock().unwrap(); + let queue = self + .queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))?; queue.position_to_id(index)? }; @@ -403,7 +417,10 @@ impl QueueTransportControl for OpenHomeRenderer { // Update local queue index { - let mut queue = self.queue.lock().unwrap(); + let mut queue = self + .queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))?; queue.set_index(Some(index))?; } @@ -415,35 +432,59 @@ impl QueueTransportControl for OpenHomeRenderer { impl QueueBackend for OpenHomeRenderer { fn len(&self) -> Result { - self.queue.lock().unwrap().len() + self.queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? + .len() } fn track_ids(&self) -> Result, ControlPointError> { - self.queue.lock().unwrap().track_ids() + self.queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? + .track_ids() } fn id_to_position(&self, id: u32) -> Result { - self.queue.lock().unwrap().id_to_position(id) + self.queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? + .id_to_position(id) } fn position_to_id(&self, id: usize) -> Result { - self.queue.lock().unwrap().position_to_id(id) + self.queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? + .position_to_id(id) } fn current_track(&self) -> Result, ControlPointError> { - self.queue.lock().unwrap().current_track() + self.queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? + .current_track() } fn current_index(&self) -> Result, ControlPointError> { - self.queue.lock().unwrap().current_index() + self.queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? + .current_index() } fn queue_snapshot(&self) -> Result { - self.queue.lock().unwrap().queue_snapshot() + self.queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? + .queue_snapshot() } fn set_index(&mut self, index: Option) -> Result<(), ControlPointError> { - self.queue.lock().unwrap().set_index(index) + self.queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? + .set_index(index) } fn replace_queue( @@ -453,20 +494,29 @@ impl QueueBackend for OpenHomeRenderer { ) -> Result<(), ControlPointError> { self.queue .lock() - .unwrap() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? .replace_queue(items, current_index) } fn sync_queue(&mut self, items: Vec) -> Result<(), ControlPointError> { - self.queue.lock().unwrap().sync_queue(items) + self.queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? + .sync_queue(items) } fn get_item(&self, index: usize) -> Result, ControlPointError> { - self.queue.lock().unwrap().get_item(index) + self.queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? + .get_item(index) } fn replace_item(&mut self, index: usize, item: PlaybackItem) -> Result<(), ControlPointError> { - self.queue.lock().unwrap().replace_item(index, item) + self.queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? + .replace_item(index, item) } fn enqueue_items( @@ -474,6 +524,9 @@ impl QueueBackend for OpenHomeRenderer { items: Vec, mode: EnqueueMode, ) -> Result<(), ControlPointError> { - self.queue.lock().unwrap().enqueue_items(items, mode) + self.queue + .lock() + .map_err(|_| ControlPointError::QueueError("Queue mutex poisoned".into()))? + .enqueue_items(items, mode) } } diff --git a/pmocontrol/src/queue/openhome.rs b/pmocontrol/src/queue/openhome.rs index 534361c6..fb925912 100644 --- a/pmocontrol/src/queue/openhome.rs +++ b/pmocontrol/src/queue/openhome.rs @@ -1,7 +1,7 @@ use std::usize; use quick_xml::escape::escape; -use tracing::debug; +use tracing::{debug, warn}; use crate::errors::ControlPointError; use crate::upnp_clients::{ @@ -619,13 +619,26 @@ impl QueueBackend for OpenHomeQueue { // differences. Without this, any drift between our cache and the renderer // (e.g., manual edits from another control point) would keep the stale items. let snapshot = self.queue_snapshot()?; + // Note: current_index may point to an index that doesn't exist in items + // if the OpenHome renderer is in an inconsistent state (e.g., IdArray returns + // IDs but ReadList returns empty TrackList). We must bounds-check here. let playing_info = snapshot.current_index.and_then(|idx| { - Some(( - idx, - snapshot.items[idx].backend_id, - snapshot.items[idx].uri.clone(), - snapshot.items[idx].didl_id.clone(), - )) + if idx < snapshot.items.len() { + Some(( + idx, + snapshot.items[idx].backend_id, + snapshot.items[idx].uri.clone(), + snapshot.items[idx].didl_id.clone(), + )) + } else { + warn!( + renderer = self.renderer_id.0.as_str(), + current_index = idx, + items_len = snapshot.items.len(), + "OpenHome renderer in inconsistent state: current_index out of bounds, treating as no current track" + ); + None + } }); debug!( diff --git a/version.txt b/version.txt index 0f826853..66784322 100644 --- a/version.txt +++ b/version.txt @@ -1 +1 @@ -0.3.7 +0.3.8