From d7582efad08a522469967938ebb0cf0f1bcfe7e9 Mon Sep 17 00:00:00 2001 From: Ben Wallis Date: Sun, 2 May 2021 16:08:39 +0100 Subject: [PATCH] Moved character creation to use character_updater instead of character_loader. Changed character_loader to use a read-only database connection. --- server/src/character_creator.rs | 8 +- server/src/persistence/character.rs | 18 +-- server/src/persistence/character_loader.rs | 116 +++----------------- server/src/persistence/character_updater.rs | 105 +++++++++++++++--- server/src/persistence/mod.rs | 49 ++++++--- server/src/sys/msg/character_screen.rs | 2 +- 6 files changed, 149 insertions(+), 149 deletions(-) diff --git a/server/src/character_creator.rs b/server/src/character_creator.rs index d967e83447..39df6e851c 100644 --- a/server/src/character_creator.rs +++ b/server/src/character_creator.rs @@ -1,8 +1,8 @@ -use crate::persistence::character_loader::CharacterLoader; +use crate::persistence::character_updater::CharacterUpdater; use common::comp::{ inventory::loadout_builder::LoadoutBuilder, Body, Inventory, Item, SkillSet, Stats, }; -use specs::{Entity, ReadExpect}; +use specs::{Entity, WriteExpect}; const VALID_STARTER_ITEMS: [&str; 6] = [ "common.items.weapons.hammer.starter_hammer", @@ -19,7 +19,7 @@ pub fn create_character( character_alias: String, character_tool: Option, body: Body, - character_loader: &ReadExpect<'_, CharacterLoader>, + character_updater: &mut WriteExpect<'_, CharacterUpdater>, ) { // quick fix whitelist validation for now; eventually replace the // `Option` with an index into a server-provided list of starter @@ -55,7 +55,7 @@ pub fn create_character( let waypoint = None; - character_loader.create_character( + character_updater.create_character( entity, player_uuid, character_alias, diff --git a/server/src/persistence/character.rs b/server/src/persistence/character.rs index a3d06c1e06..27ff3a8292 100644 --- a/server/src/persistence/character.rs +++ b/server/src/persistence/character.rs @@ -26,7 +26,7 @@ use crate::{ }; use common::character::{CharacterId, CharacterItem, MAX_CHARACTERS_PER_PLAYER}; use core::ops::Range; -use rusqlite::{types::Value, ToSql, Transaction, NO_PARAMS}; +use rusqlite::{types::Value, Connection, ToSql, Transaction, NO_PARAMS}; use std::{collections::VecDeque, rc::Rc}; use tracing::{error, trace, warn}; @@ -53,10 +53,7 @@ struct CharacterContainers { /// BFS the inventory/loadout to ensure that each is topologically sorted in the /// sense required by convert_inventory_from_database_items to support recursive /// items -pub fn load_items_bfs( - connection: &mut Transaction, - root: i64, -) -> Result, PersistenceError> { +pub fn load_items_bfs(connection: &Connection, root: i64) -> Result, PersistenceError> { let mut items = Vec::new(); let mut queue = VecDeque::new(); queue.push_front(root); @@ -100,7 +97,7 @@ pub fn load_items_bfs( pub fn load_character_data( requesting_player_uuid: String, char_id: CharacterId, - connection: &mut Transaction, + connection: &Connection, ) -> CharacterDataResult { let character_containers = get_pseudo_containers(connection, char_id)?; let inventory_items = load_items_bfs(connection, character_containers.inventory_container_id)?; @@ -216,10 +213,7 @@ pub fn load_character_data( /// In the event that a join fails, for a character (i.e. they lack an entry for /// stats, body, etc...) the character is skipped, and no entry will be /// returned. -pub fn load_character_list( - player_uuid_: &str, - connection: &mut Transaction, -) -> CharacterListResult { +pub fn load_character_list(player_uuid_: &str, connection: &Connection) -> CharacterListResult { let characters; { #[rustfmt::skip] @@ -619,7 +613,7 @@ fn get_new_entity_ids( /// Fetches the pseudo_container IDs for a character fn get_pseudo_containers( - connection: &mut Transaction, + connection: &Connection, character_id: CharacterId, ) -> Result { let character_containers = CharacterContainers { @@ -639,7 +633,7 @@ fn get_pseudo_containers( } fn get_pseudo_container_id( - connection: &mut Transaction, + connection: &Connection, character_id: CharacterId, pseudo_container_position: &str, ) -> Result { diff --git a/server/src/persistence/character_loader.rs b/server/src/persistence/character_loader.rs index 360802cfee..9b77fc65d4 100644 --- a/server/src/persistence/character_loader.rs +++ b/server/src/persistence/character_loader.rs @@ -1,13 +1,13 @@ use crate::persistence::{ - character::{create_character, delete_character, load_character_data, load_character_list}, + character::{load_character_data, load_character_list}, error::PersistenceError, - establish_connection, DatabaseSettings, PersistedComponents, + establish_connection, ConnectionMode, DatabaseSettings, PersistedComponents, }; use common::character::{CharacterId, CharacterItem}; use crossbeam_channel::{self, TryIter}; -use rusqlite::Transaction; +use rusqlite::Connection; use std::sync::{Arc, RwLock}; -use tracing::{error, trace}; +use tracing::error; pub(crate) type CharacterListResult = Result, PersistenceError>; pub(crate) type CharacterCreationResult = @@ -17,15 +17,6 @@ type CharacterLoaderRequest = (specs::Entity, CharacterLoaderRequestKind); /// Available database operations when modifying a player's character list enum CharacterLoaderRequestKind { - CreateCharacter { - player_uuid: String, - character_alias: String, - persisted_components: PersistedComponents, - }, - DeleteCharacter { - player_uuid: String, - character_id: CharacterId, - }, LoadCharacterList { player_uuid: String, }, @@ -89,38 +80,18 @@ impl CharacterLoader { .spawn(move || { // Unwrap here is safe as there is no code that can panic when the write lock is // taken that could cause the RwLock to become poisoned. - let mut conn = establish_connection(&*settings.read().unwrap()); + // + // This connection -must- remain read-only to avoid lock contention with the + // CharacterUpdater thread. + let mut conn = + establish_connection(&*settings.read().unwrap(), ConnectionMode::ReadOnly); for request in internal_rx { conn.update_log_mode(&settings); - match conn.connection.transaction() { - Ok(mut transaction) => { - let response = - CharacterLoader::process_request(request, &mut transaction); - if !response.is_err() { - match transaction.commit() { - Ok(()) => { - trace!("Commit for character loader completed"); - }, - Err(e) => error!( - "Failed to commit transaction for character loader, \ - error: {:?}", - e - ), - }; - }; - - if let Err(e) = internal_tx.send(response) { - error!(?e, "Could not send character loader response"); - } - }, - Err(e) => { - error!( - "Failed to start transaction for character loader, error: {:?}", - e - ) - }, + let response = CharacterLoader::process_request(request, &conn); + if let Err(e) = internal_tx.send(response) { + error!(?e, "Could not send character loader response"); } } }) @@ -137,41 +108,23 @@ impl CharacterLoader { // CharacterLoaderResponse::is_err() fn process_request( request: CharacterLoaderRequest, - mut transaction: &mut Transaction, + connection: &Connection, ) -> CharacterLoaderResponse { let (entity, kind) = request; CharacterLoaderResponse { entity, result: match kind { - CharacterLoaderRequestKind::CreateCharacter { - player_uuid, - character_alias, - persisted_components, - } => CharacterLoaderResponseKind::CharacterCreation(create_character( - &player_uuid, - &character_alias, - persisted_components, - &mut transaction, - )), - CharacterLoaderRequestKind::DeleteCharacter { - player_uuid, - character_id, - } => CharacterLoaderResponseKind::CharacterList(delete_character( - &player_uuid, - character_id, - &mut transaction, - )), CharacterLoaderRequestKind::LoadCharacterList { player_uuid } => { CharacterLoaderResponseKind::CharacterList(load_character_list( &player_uuid, - &mut transaction, + connection, )) }, CharacterLoaderRequestKind::LoadCharacterData { player_uuid, character_id, } => { - let result = load_character_data(player_uuid, character_id, &mut transaction); + let result = load_character_data(player_uuid, character_id, connection); if result.is_err() { error!( ?result, @@ -184,45 +137,6 @@ impl CharacterLoader { } } - /// Create a new character belonging to the player identified by - /// `player_uuid` - pub fn create_character( - &self, - entity: specs::Entity, - player_uuid: String, - character_alias: String, - persisted_components: PersistedComponents, - ) { - if let Err(e) = self - .update_tx - .send((entity, CharacterLoaderRequestKind::CreateCharacter { - player_uuid, - character_alias, - persisted_components, - })) - { - error!(?e, "Could not send character creation request"); - } - } - - /// Delete a character by `id` and `player_uuid` - pub fn delete_character( - &self, - entity: specs::Entity, - player_uuid: String, - character_id: CharacterId, - ) { - if let Err(e) = self - .update_tx - .send((entity, CharacterLoaderRequestKind::DeleteCharacter { - player_uuid, - character_id, - })) - { - error!(?e, "Could not send character deletion request"); - } - } - /// Loads a list of characters belonging to the player identified by /// `player_uuid` pub fn load_character_list(&self, entity: specs::Entity, player_uuid: String) { diff --git a/server/src/persistence/character_updater.rs b/server/src/persistence/character_updater.rs index f6de8c524d..0311c722ac 100644 --- a/server/src/persistence/character_updater.rs +++ b/server/src/persistence/character_updater.rs @@ -4,7 +4,7 @@ use common::character::CharacterId; use crate::persistence::{ character_loader::{CharacterLoaderResponse, CharacterLoaderResponseKind}, error::PersistenceError, - establish_connection, DatabaseSettings, VelorenConnection, + establish_connection, ConnectionMode, DatabaseSettings, PersistedComponents, VelorenConnection, }; use crossbeam_channel::TryIter; use rusqlite::DropBehavior; @@ -20,8 +20,15 @@ use tracing::{debug, error, info, trace, warn}; pub type CharacterUpdateData = (comp::SkillSet, comp::Inventory, Option); +#[allow(clippy::large_enum_variant)] pub enum CharacterUpdaterEvent { BatchUpdate(Vec<(CharacterId, CharacterUpdateData)>), + CreateCharacter { + entity: Entity, + player_uuid: String, + character_alias: String, + persisted_components: PersistedComponents, + }, DeleteCharacter { entity: Entity, requesting_player_uuid: String, @@ -58,7 +65,8 @@ impl CharacterUpdater { .spawn(move || { // Unwrap here is safe as there is no code that can panic when the write lock is // taken that could cause the RwLock to become poisoned. - let mut conn = establish_connection(&*settings.read().unwrap()); + let mut conn = + establish_connection(&*settings.read().unwrap(), ConnectionMode::ReadWrite); while let Ok(updates) = update_rx.recv() { match updates { CharacterUpdaterEvent::BatchUpdate(updates) => { @@ -80,6 +88,35 @@ impl CharacterUpdater { .store(true, Ordering::Relaxed); }; }, + CharacterUpdaterEvent::CreateCharacter { + entity, + character_alias, + player_uuid, + persisted_components, + } => { + match execute_character_create( + entity, + character_alias, + &player_uuid, + persisted_components, + &mut conn, + ) { + Ok(response) => { + if let Err(e) = response_tx.send(response) { + error!(?e, "Could not send character creation response"); + } else { + debug!( + "Processed character create for player {}", + player_uuid + ); + } + }, + Err(e) => error!( + "Error creating character for player {}, error: {:?}", + player_uuid, e + ), + } + }, CharacterUpdaterEvent::DeleteCharacter { entity, requesting_player_uuid, @@ -165,6 +202,28 @@ impl CharacterUpdater { .load(Ordering::Relaxed) } + pub fn create_character( + &mut self, + entity: Entity, + requesting_player_uuid: String, + alias: String, + persisted_components: PersistedComponents, + ) { + if let Err(e) = + self.update_tx + .as_ref() + .unwrap() + .send(CharacterUpdaterEvent::CreateCharacter { + entity, + player_uuid: requesting_player_uuid, + character_alias: alias, + persisted_components, + }) + { + error!(?e, "Could not send character creation request"); + } + } + pub fn delete_character( &mut self, entity: Entity, @@ -223,22 +282,6 @@ impl CharacterUpdater { } } - /// Updates a single character based on their id and components - pub fn update( - &mut self, - character_id: CharacterId, - skill_set: &comp::SkillSet, - inventory: &comp::Inventory, - waypoint: Option<&comp::Waypoint>, - ) { - self.batch_update(std::iter::once(( - character_id, - skill_set, - inventory, - waypoint, - ))); - } - /// Indicates to the batch update thread that a requested disconnection of /// all clients has been processed pub fn disconnected_success(&mut self) { @@ -274,6 +317,32 @@ fn execute_batch_update( Ok(()) } +fn execute_character_create( + entity: Entity, + alias: String, + requesting_player_uuid: &str, + persisted_components: PersistedComponents, + connection: &mut VelorenConnection, +) -> Result { + let mut transaction = connection.connection.transaction()?; + + let response = CharacterLoaderResponse { + entity, + result: CharacterLoaderResponseKind::CharacterCreation(super::character::create_character( + requesting_player_uuid, + &alias, + persisted_components, + &mut transaction, + )), + }; + + if !response.is_err() { + transaction.commit()?; + }; + + Ok(response) +} + fn execute_character_delete( entity: Entity, requesting_player_uuid: &str, diff --git a/server/src/persistence/mod.rs b/server/src/persistence/mod.rs index 497b5a41e9..0b09177279 100644 --- a/server/src/persistence/mod.rs +++ b/server/src/persistence/mod.rs @@ -13,6 +13,7 @@ use refinery::Report; use rusqlite::{Connection, OpenFlags}; use std::{ fs, + ops::Deref, path::PathBuf, sync::{Arc, RwLock}, time::Duration, @@ -69,6 +70,12 @@ impl VelorenConnection { } } +impl Deref for VelorenConnection { + type Target = Connection; + + fn deref(&self) -> &Connection { &self.connection } +} + fn set_log_mode(connection: &mut Connection, sql_log_mode: SqlLogMode) { // Rusqlite's trace and profile logging are mutually exclusive and cannot be // used together @@ -92,6 +99,12 @@ pub struct DatabaseSettings { pub sql_log_mode: SqlLogMode, } +#[derive(Clone, Copy, PartialEq)] +pub enum ConnectionMode { + ReadOnly, + ReadWrite, +} + #[derive(Clone, Copy, Debug, PartialEq)] pub enum SqlLogMode { /// Logging is disabled @@ -104,7 +117,7 @@ pub enum SqlLogMode { /// Runs any pending database migrations. This is executed during server startup pub fn run_migrations(settings: &DatabaseSettings) { - let mut conn = establish_connection(settings); + let mut conn = establish_connection(settings, ConnectionMode::ReadWrite); diesel_to_rusqlite::migrate_from_diesel(&mut conn) .expect("One-time migration from Diesel to Refinery failed"); @@ -131,22 +144,32 @@ fn rusqlite_profile_callback(log_message: &str, dur: Duration) { info!("{} Duration: {:?}", log_message, dur); } -pub(crate) fn establish_connection(settings: &DatabaseSettings) -> VelorenConnection { +pub(crate) fn establish_connection( + settings: &DatabaseSettings, + connection_mode: ConnectionMode, +) -> VelorenConnection { fs::create_dir_all(&settings.db_dir).expect(&*format!( "Failed to create saves directory: {:?}", &settings.db_dir )); - let connection = Connection::open_with_flags( - &settings.db_dir.join("db.sqlite"), - OpenFlags::SQLITE_OPEN_PRIVATE_CACHE | OpenFlags::default(), - ) - .unwrap_or_else(|err| { - panic!( - "Error connecting to {}, Error: {:?}", - settings.db_dir.join("db.sqlite").display(), - err - ) - }); + + let open_flags = OpenFlags::SQLITE_OPEN_PRIVATE_CACHE + | OpenFlags::SQLITE_OPEN_NO_MUTEX + | match connection_mode { + ConnectionMode::ReadWrite => { + OpenFlags::SQLITE_OPEN_CREATE | OpenFlags::SQLITE_OPEN_READ_WRITE + }, + ConnectionMode::ReadOnly => OpenFlags::SQLITE_OPEN_READ_ONLY, + }; + + let connection = Connection::open_with_flags(&settings.db_dir.join("db.sqlite"), open_flags) + .unwrap_or_else(|err| { + panic!( + "Error connecting to {}, Error: {:?}", + settings.db_dir.join("db.sqlite").display(), + err + ) + }); let mut veloren_connection = VelorenConnection::new(connection); diff --git a/server/src/sys/msg/character_screen.rs b/server/src/sys/msg/character_screen.rs index d5c1f46484..5610bfedf2 100644 --- a/server/src/sys/msg/character_screen.rs +++ b/server/src/sys/msg/character_screen.rs @@ -126,7 +126,7 @@ impl Sys { alias, tool, body, - character_loader, + character_updater, ); } },