Fix backend mutex poisoned panic on OpenHome stop
Fix backend mutex poisoned panic when stopping playback on OpenHome renderer This commit addresses a panic that occurred when stopping playback on OpenHome renderers, which was caused by: 1. Mutex poisoning from unhandled panics in backend operations 2. A regression revealed after initial fixes, where inconsistent renderer state caused index out of bounds errors Changes include: - Added defensive error handling for mutex operations to prevent poisoning - Implemented bounds-checking for current_index in sync_queue to handle inconsistent renderer states - Removed redundant clear_queue calls that were causing additional failures - Improved error tolerance in queue operations The fix ensures that the backend mutex remains healthy and that inconsistent renderer states are handled gracefully rather than causing panics.
This commit is contained in:
78
Blackboard/Done/fix-backend-mutex-poisoned.md
Normal file
78
Blackboard/Done/fix-backend-mutex-poisoned.md
Normal file
@@ -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 `<TrackList>` 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.
|
||||
152
Blackboard/Report/fix-backend-mutex-poisoned.md
Normal file
152
Blackboard/Report/fix-backend-mutex-poisoned.md
Normal file
@@ -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 `<TrackList>` 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.
|
||||
Reference in New Issue
Block a user