From: Ian Jackson Date: Fri, 13 Nov 2020 21:15:58 +0000 (+0000) Subject: fix lock order X-Git-Tag: otter-0.2.0~527 X-Git-Url: https://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git?a=commitdiff_plain;h=8827f260653a0fb511ff0f575454d7ca574d94d2;p=otter.git fix lock order Signed-off-by: Ian Jackson --- diff --git a/src/accounts.rs b/src/accounts.rs index 71fd276b..b0239a10 100644 --- a/src/accounts.rs +++ b/src/accounts.rs @@ -309,6 +309,7 @@ impl AccountsGuard { F: FnOnce(&mut AccountRecord, AccountId) -> T >( &mut self, + games: &mut GamesGuard, key: K, auth: Authorisation, set_access: Option, @@ -320,6 +321,7 @@ impl AccountsGuard { if let Some(new_access) = set_access { process_all_players_for_account( + games, acctid, |ig, player| ig.invalidate_tokens(player) )?; @@ -360,6 +362,7 @@ impl AccountsGuard { #[throws(MgmtError)] pub fn remove_entry(&mut self, + games: &mut GamesGuard, account: &AccountName, _auth: Authorisation) { @@ -369,7 +372,7 @@ impl AccountsGuard { then { (accounts, acctid) } else { throw!(AccountNotFound) } }; - process_all_players_for_account(acctid, |ig,player| { + process_all_players_for_account(games, acctid, |ig,player| { ig.players_remove(&[player].iter().cloned().collect())?; Ok::<_,ME>(()) })?; diff --git a/src/bin/daemon-otter.rs b/src/bin/daemon-otter.rs index 5b197c1f..490ba9d3 100644 --- a/src/bin/daemon-otter.rs +++ b/src/bin/daemon-otter.rs @@ -102,6 +102,9 @@ fn resource<'r>(leaf : CheckedResourceLeaf) -> impl Responder<'r> { #[throws(StartupError)] fn main() { + // xxx test suite for cli at least + // todo test suite for web api + let config_filename = env::args().nth(1); ServerConfig::read(config_filename.as_ref().map(String::as_str))?; @@ -119,7 +122,7 @@ fn main() { shapelib::load()?; load_accounts()?; - load_games(&mut AccountsGuard::lock())?; + load_games(&mut AccountsGuard::lock(), &mut games_lock())?; let cl = CommandListener::new()?; cl.spawn()?; diff --git a/src/cmdlistener.rs b/src/cmdlistener.rs index f73a5a2e..c029da90 100644 --- a/src/cmdlistener.rs +++ b/src/cmdlistener.rs @@ -108,9 +108,10 @@ fn execute(cs: &mut CommandStream, cmd: MgmtCommand) -> MgmtResponse { UpdateAccont(AccountDetails { account, nick, timezone, access }) => { let mut ag = AccountsGuard::lock(); + let mut games = games_lock(); let auth = authorise_for_account(cs, &ag, &account)?; let access = cs.accountrecord_from_spec(access)?; - ag.with_entry_mut(&account, auth, access, |record, _acctid|{ + ag.with_entry_mut(&mut games, &account, auth, access, |record, _acctid|{ fn update_from(spec: Option, record: &mut T) { if let Some(new) = spec { *record = new; } } @@ -124,8 +125,9 @@ fn execute(cs: &mut CommandStream, cmd: MgmtCommand) -> MgmtResponse { DeleteAccount(account) => { let mut ag = AccountsGuard::lock(); + let mut games = games_lock(); let auth = authorise_for_account(cs, &ag, &account)?; - ag.remove_entry(&account, auth)?; + ag.remove_entry(&mut games, &account, auth)?; Fine } @@ -141,6 +143,7 @@ fn execute(cs: &mut CommandStream, cmd: MgmtCommand) -> MgmtResponse { CreateGame { game, insns } => { let mut ag = AccountsGuard::lock(); + let mut games = games_lock(); let auth = authorise_by_account(cs, &ag, &game)?; let gs = crate::gamestate::GameState { @@ -153,7 +156,7 @@ fn execute(cs: &mut CommandStream, cmd: MgmtCommand) -> MgmtResponse { }; let acl = default(); - let gref = Instance::new(game, gs, acl, auth)?; + let gref = Instance::new(game, gs, &mut games, acl, auth)?; let ig = gref.lock()?; let mut ig = Unauthorised::of(ig); @@ -164,7 +167,7 @@ fn execute(cs: &mut CommandStream, cmd: MgmtCommand) -> MgmtResponse { let ig = ig.by(Authorisation::authorise_any()); let name = ig.name.clone(); let InstanceGuard { c, .. } = ig; - Instance::destroy_game(c, auth) + Instance::destroy_game(&mut games, c, auth) .unwrap_or_else(|e| warn!( "failed to tidy up failecd creation of {:?}: {:?}", &name, &e @@ -199,10 +202,11 @@ fn execute(cs: &mut CommandStream, cmd: MgmtCommand) -> MgmtResponse { DestroyGame { game } => { let mut ag = AccountsGuard::lock(); + let mut games = games_lock(); let auth = authorise_by_account(cs, &mut ag, &game)?; let gref = Instance::lookup_by_name(&game, auth)?; let ig = gref.lock_even_poisoned(); - Instance::destroy_game(ig, auth)?; + Instance::destroy_game(&mut games, ig, auth)?; Fine } diff --git a/src/global.rs b/src/global.rs index 0c95e29f..b480129c 100644 --- a/src/global.rs +++ b/src/global.rs @@ -188,7 +188,7 @@ pub struct Global { // slow global locks: save_area_lock : Mutex>, // <- accounts::accounts -> - games : RwLock,InstanceRef>>, + games_table: RwLock, // per-game lock: // <- InstanceContainer -> @@ -203,6 +203,9 @@ pub struct Global { clients : RwLock>, } +pub type GamesGuard = RwLockWriteGuard<'static, GamesTable>; +pub type GamesTable = HashMap,InstanceRef>; + #[derive(Debug)] pub struct InstanceContainer { live : bool, @@ -285,6 +288,7 @@ impl Instance { #[allow(clippy::new_ret_no_self)] #[throws(MgmtError)] pub fn new(name: InstanceName, gs: GameState, + games: &mut GamesGuard, acl: LoadedAcl, _: Authorisation) -> InstanceRef { let name = Arc::new(name); @@ -309,9 +313,6 @@ impl Instance { let gref = InstanceRef(Arc::new(Mutex::new(cont))); let mut ig = gref.lock()?; - // We hold the GLOBAL.games lock while we save the new game. - // That lock is not on the hot path. - let mut games = GLOBAL.games.write().unwrap(); let entry = games.entry(name); use hash_map::Entry::*; @@ -335,7 +336,7 @@ impl Instance { -> Unauthorised { Unauthorised::of( - GLOBAL.games.read().unwrap() + GLOBAL.games_table.read().unwrap() .get(name) .ok_or(MgmtError::GameNotFound)? .clone() @@ -350,18 +351,17 @@ impl Instance { } #[throws(InternalError)] - pub fn destroy_game(mut g: MutexGuard, + pub fn destroy_game(games: &mut GamesGuard, + mut g: MutexGuard, _: Authorisation) { let a_savefile = savefilename(&g.g.name, "a-", ""); - let mut gw = GLOBAL.games.write().unwrap(); - // xxx lock order wrong, this function should unlock and then relock let g_file = savefilename(&g.g.name, "g-", ""); fs::remove_file(&g_file).context("remove").context(g_file)?; (||{ // Infallible: g.live = false; - gw.remove(&g.g.name); + games.remove(&g.g.name); InstanceGuard::forget_all_tokens(&mut g.g.tokens_clients); InstanceGuard::forget_all_tokens(&mut g.g.tokens_players); })(); // <- No ?, ensures that IEFE is infallible (barring panics) @@ -380,7 +380,7 @@ impl Instance { pub fn list_names(account: Option<&AccountName>, _: Authorisation) -> Vec> { - let games = GLOBAL.games.read().unwrap(); + let games = GLOBAL.games_table.read().unwrap(); let out : Vec> = games.keys() .filter(|k| account == None || account == Some(&k.account)) @@ -390,6 +390,10 @@ impl Instance { } } +pub fn games_lock() -> RwLockWriteGuard<'static, GamesTable> { + GLOBAL.games_table.write().unwrap() +} + // ---------- Simple trait implementations ---------- impl Deref for InstanceGuard<'_> { @@ -898,6 +902,7 @@ impl InstanceGuard<'_> { #[throws(StartupError)] fn load_game(accounts: &AccountsGuard, + games: &mut GamesGuard, name: InstanceName) -> Option { { let mut st = GLOBAL.save_area_lock.lock().unwrap(); @@ -1007,14 +1012,15 @@ impl InstanceGuard<'_> { } } drop(global); drop(g); - GLOBAL.games.write().unwrap().insert(name.clone(), gref.clone()); + games.insert(name.clone(), gref.clone()); info!("loadewd {:?}", &name); Some(gref) } } #[throws(anyhow::Error)] -pub fn load_games(accounts: &mut AccountsGuard) { +pub fn load_games(accounts: &mut AccountsGuard, + games: &mut GamesGuard) { enum AFState { Found(PathBuf), Used }; use AFState::*; use SavefilenameParseResult::*; @@ -1037,7 +1043,7 @@ pub fn load_games(accounts: &mut AccountsGuard) { ); }, GameFile { access_leaf, name } => { - InstanceGuard::load_game(accounts, name)?; + InstanceGuard::load_game(accounts, games, name)?; a_leaves.insert(access_leaf, Used); }, } @@ -1142,9 +1148,8 @@ pub fn process_all_players_for_account< E: Error, F: FnMut(&mut InstanceGuard<'_>, PlayerId) -> Result<(),E> > - (acctid: AccountId, mut f: F) + (games: &mut GamesGuard, acctid: AccountId, mut f: F) { - let games = GLOBAL.games.write().unwrap(); for gref in games.values() { let c = gref.lock_even_poisoned(); let remove : Vec<_> = c.g.iplayers.iter().filter_map(|(player,pr)| { @@ -1281,7 +1286,7 @@ fn client_expire_old_clients() { fn old(&mut self, client: ClientId) -> Option; } - for gref in GLOBAL.games.read().unwrap().values() { + for gref in GLOBAL.games_table.read().unwrap().values() { struct Any; impl ClientIterator for Any { type Ret = (); @@ -1328,7 +1333,7 @@ fn global_expire_old_logs() { let mut want_expire = vec![]; - let read = GLOBAL.games.read().unwrap(); + let read = GLOBAL.games_table.read().unwrap(); for gref in read.values() { if gref.lock_even_poisoned().g.gs.want_expire_some_logs(cutoff) { want_expire.push(gref.clone()) diff --git a/src/imports.rs b/src/imports.rs index 68cdfeac..c0f3c489 100644 --- a/src/imports.rs +++ b/src/imports.rs @@ -10,7 +10,8 @@ pub use std::fmt::Formatter; pub use std::fmt::{self,Display,Debug}; pub use std::thread::{self,sleep}; pub use std::time::Duration; -pub use std::sync::{Arc,Mutex,MutexGuard,RwLock,RwLockReadGuard,Condvar}; +pub use std::sync::{Arc,Mutex,MutexGuard,Condvar}; +pub use std::sync::{RwLock,RwLockReadGuard,RwLockWriteGuard}; pub use std::collections::{HashMap,hash_map,HashSet}; pub use std::hash::Hash; pub use std::borrow::Borrow;