Fix a deadlock between SpircManager and Player.

The player was invoking its observers with the state locked, and the
SpircManager’s registered observer would try to lock it again.

Instead, the observers are invoked with a cloned state, and the SpircManager
avoids locking the state again.
This commit is contained in:
Paul Lietar 2016-02-22 05:41:37 +00:00
parent 95d1dfd774
commit c700315b30
2 changed files with 17 additions and 8 deletions

View file

@ -20,6 +20,7 @@ pub struct Player {
commands: mpsc::Sender<PlayerCommand>, commands: mpsc::Sender<PlayerCommand>,
} }
#[derive(Clone)]
pub struct PlayerState { pub struct PlayerState {
status: PlayStatus, status: PlayStatus,
position_ms: u32, position_ms: u32,
@ -38,6 +39,7 @@ struct PlayerInternal {
commands: mpsc::Receiver<PlayerCommand>, commands: mpsc::Receiver<PlayerCommand>,
} }
#[derive(Debug)]
enum PlayerCommand { enum PlayerCommand {
Load(SpotifyId, bool, u32), Load(SpotifyId, bool, u32),
Play, Play,
@ -102,8 +104,8 @@ impl Player {
self.command(PlayerCommand::Seek(position_ms)); self.command(PlayerCommand::Seek(position_ms));
} }
pub fn state(&self) -> MutexGuard<PlayerState> { pub fn state(&self) -> PlayerState {
self.state.lock().unwrap() self.state.lock().unwrap().clone()
} }
pub fn volume(&self, vol: u16) { pub fn volume(&self, vol: u16) {
@ -315,8 +317,11 @@ impl PlayerInternal {
let observers = self.observers.lock().unwrap(); let observers = self.observers.lock().unwrap();
if update { if update {
guard.update_time = util::now_ms(); guard.update_time = util::now_ms();
let state = guard.clone();
drop(guard);
for observer in observers.iter() { for observer in observers.iter() {
observer(&guard); observer(&state);
} }
} }
} }

View file

@ -8,6 +8,7 @@ use util::version::version_string;
use mercury::{MercuryRequest, MercuryMethod}; use mercury::{MercuryRequest, MercuryMethod};
use player::{Player, PlayerState}; use player::{Player, PlayerState};
use std::borrow::Cow;
use std::sync::{Mutex, Arc}; use std::sync::{Mutex, Arc};
use std::collections::HashMap; use std::collections::HashMap;
@ -420,8 +421,11 @@ impl<'a> CommandSender<'a> {
} }
fn send(self) { fn send(self) {
let internal_player_state = self.spirc_internal.player.state(); let state = self.player_state.map_or_else(|| {
let s = self.player_state.unwrap_or(&*internal_player_state); Cow::Owned(self.spirc_internal.player.state())
}, |s| {
Cow::Borrowed(s)
});
let mut pkt = protobuf_init!(protocol::spirc::Frame::new(), { let mut pkt = protobuf_init!(protocol::spirc::Frame::new(), {
version: 1, version: 1,
@ -432,12 +436,12 @@ impl<'a> CommandSender<'a> {
recipient: RepeatedField::from_vec( recipient: RepeatedField::from_vec(
self.recipient.map(|r| vec![r.to_owned()] ).unwrap_or(vec![]) self.recipient.map(|r| vec![r.to_owned()] ).unwrap_or(vec![])
), ),
device_state: self.spirc_internal.device_state(s), device_state: self.spirc_internal.device_state(&state),
state_update_id: s.update_time() as i64, state_update_id: state.update_time()
}); });
if self.spirc_internal.is_active { if self.spirc_internal.is_active {
pkt.set_state(self.spirc_internal.spirc_state(s)); pkt.set_state(self.spirc_internal.spirc_state(&state));
} }
self.spirc_internal self.spirc_internal