From 488bf48890c3af492f2889610cf98c0b6e5a84f3 Mon Sep 17 00:00:00 2001 From: Artem Fedorov Date: Tue, 25 Nov 2025 00:51:11 +0100 Subject: [PATCH 1/8] Implement global ID generation --- game/src/packets/from_client/restart.rs | 2 +- game/src/packets/to_client/char_info.rs | 2 +- .../to_client/char_move_to_location.rs | 2 +- game/src/packets/to_client/char_selected.rs | 5 +- .../to_client/extended/equipped_items.rs | 5 +- .../to_client/extended/inventory_weight.rs | 5 +- .../to_client/extended/premium_state.rs | 2 +- .../packets/to_client/extended/rotation.rs | 2 +- game/src/packets/to_client/move_to.rs | 2 +- .../src/packets/to_client/relation_changed.rs | 2 +- game/src/packets/to_client/user_info.rs | 7 ++- l2-core/src/data/char_template.rs | 9 +-- l2-core/src/game_objects/item/_item.rs | 19 +++++- l2-core/src/game_objects/player/_player.rs | 24 ++++++-- l2-core/src/game_objects/player/paper_doll.rs | 15 ++--- l2-core/src/game_objects/player/party.rs | 6 +- l2-core/src/game_objects/player/quest.rs | 1 + l2-core/src/id_factory.rs | 61 +++++++++++++++++++ l2-core/src/lib.rs | 1 + 19 files changed, 134 insertions(+), 38 deletions(-) create mode 100644 l2-core/src/id_factory.rs diff --git a/game/src/packets/from_client/restart.rs b/game/src/packets/from_client/restart.rs index 557b03b..edf090e 100644 --- a/game/src/packets/from_client/restart.rs +++ b/game/src/packets/from_client/restart.rs @@ -34,7 +34,7 @@ impl Message for PlayerClient { let p = CharSelectionInfo::new(&user_name.clone(), session_id, &self.controller, chars)?; let player = self.try_get_selected_char()?.clone(); // we clone it to avoid borrow checker issues with reference to self self.controller.broadcast_packet_with_filter( - DeleteObject::new(player.get_id())?, + DeleteObject::new(player.get_object_id())?, Some(Box::new(move |acc, _| !acc.eq(&user_name))), ); self.send_packet(RestartResponse::ok()?).await?; diff --git a/game/src/packets/to_client/char_info.rs b/game/src/packets/to_client/char_info.rs index 103b8b1..df706bb 100644 --- a/game/src/packets/to_client/char_info.rs +++ b/game/src/packets/to_client/char_info.rs @@ -55,7 +55,7 @@ impl CharInfo { inst.buffer.write_i32(p.get_z())?; // Confirmed inst.buffer .write_i32(p.get_vehicle_object_id().unwrap_or(0))?; // Confirmed - inst.buffer.write_i32(p.char_model.id)?; // Confirmed + inst.buffer.write_i32(p.get_object_id())?; // Confirmed inst.buffer .write_c_utf16le_string(Some(p.get_visible_name()))?; // Confirmed inst.buffer.write_i16(p.char_model.race_id)?; // Confirmed diff --git a/game/src/packets/to_client/char_move_to_location.rs b/game/src/packets/to_client/char_move_to_location.rs index 60946b8..24fa5c3 100644 --- a/game/src/packets/to_client/char_move_to_location.rs +++ b/game/src/packets/to_client/char_move_to_location.rs @@ -18,7 +18,7 @@ impl CharMoveToLocation { buffer: SendablePacketBuffer::new(), }; inst.buffer.write(Self::PACKET_ID)?; - inst.buffer.write_i32(p.get_id())?; // 1-7 increase force, level + inst.buffer.write_i32(p.get_object_id())?; // 1-7 increase force, level // Target position inst.buffer.write_i32(target_x)?; diff --git a/game/src/packets/to_client/char_selected.rs b/game/src/packets/to_client/char_selected.rs index 2d8f53d..8ea19b6 100644 --- a/game/src/packets/to_client/char_selected.rs +++ b/game/src/packets/to_client/char_selected.rs @@ -20,7 +20,7 @@ impl CharSelected { inst.buffer.write(Self::PACKET_ID)?; inst.buffer .write_c_utf16le_string(Some(&player.char_model.name))?; - inst.buffer.write_i32(player.char_model.id)?; + inst.buffer.write_i32(player.get_object_id())?; inst.buffer .write_c_utf16le_string(player.char_model.title.as_deref())?; inst.buffer.write_i32(session_id)?; @@ -86,7 +86,8 @@ mod tests { }; let templates = ClassTemplates::load(); let temp = templates.try_get_template(inst.class_id).unwrap().clone(); - let char = Player::new(inst, vec![], temp); + let mut char = Player::new(inst, vec![], temp); + char.object_id = char.char_model.id; let mut packet = CharSelected::new(&char, 9998, 286).unwrap(); assert_eq!( [ diff --git a/game/src/packets/to_client/extended/equipped_items.rs b/game/src/packets/to_client/extended/equipped_items.rs index 0424baa..8e5f5ec 100644 --- a/game/src/packets/to_client/extended/equipped_items.rs +++ b/game/src/packets/to_client/extended/equipped_items.rs @@ -21,7 +21,7 @@ impl EquippedItems { }; inst.buffer.write(Self::PACKET_ID)?; inst.buffer.write_u16(Self::EX_PACKET_ID)?; - inst.buffer.write_i32(p.char_model.id)?; + inst.buffer.write_i32(p.get_object_id())?; let mut bitmask = BitMask::new(40); //5 bytes, 50 bits let slots = InventorySlot::iter(); let num_slots = slots.len(); @@ -82,7 +82,8 @@ mod tests { .class_templates .try_get_template(Class::try_from(char.class_id).unwrap()) .unwrap(); - let player = Player::new(char, vec![], template.clone()); + let mut player = Player::new(char, vec![], template.clone()); + player.object_id = player.char_model.id; let p = EquippedItems::new(&player, true).unwrap(); assert_eq!( [ diff --git a/game/src/packets/to_client/extended/inventory_weight.rs b/game/src/packets/to_client/extended/inventory_weight.rs index 0ff3e70..cdc80d3 100644 --- a/game/src/packets/to_client/extended/inventory_weight.rs +++ b/game/src/packets/to_client/extended/inventory_weight.rs @@ -17,7 +17,7 @@ impl InventoryWeight { }; inst.buffer.write(Self::PACKET_ID)?; inst.buffer.write_u16(Self::EX_PACKET_ID)?; - inst.buffer.write_i32(p.char_model.id)?; + inst.buffer.write_i32(p.get_object_id())?; inst.buffer.write_i32(p.inventory.get_current_load())?; inst.buffer.write_i32(p.inventory.get_max_load())?; Ok(inst) @@ -53,7 +53,8 @@ mod tests { .class_templates .try_get_template(Class::try_from(char.class_id).unwrap()) .unwrap(); - let player = Player::new(char, vec![], template.clone()); + let mut player = Player::new(char, vec![], template.clone()); + player.object_id = player.char_model.id; let p = InventoryWeight::new(&player).unwrap(); assert_eq!( [254, 102, 1, 44, 159, 0, 16, 0, 0, 0, 0, 108, 24, 3, 0], diff --git a/game/src/packets/to_client/extended/premium_state.rs b/game/src/packets/to_client/extended/premium_state.rs index d24cbbc..efb1229 100644 --- a/game/src/packets/to_client/extended/premium_state.rs +++ b/game/src/packets/to_client/extended/premium_state.rs @@ -15,7 +15,7 @@ impl PremiumState { let mut buffer = SendablePacketBuffer::new(); buffer.write(Self::PACKET_ID)?; buffer.write_u16(Self::EX_PACKET_ID)?; - buffer.write_i32(p.char_model.id)?; + buffer.write_i32(p.get_object_id())?; buffer.write(p.has_premium())?; Ok(Self { buffer }) } diff --git a/game/src/packets/to_client/extended/rotation.rs b/game/src/packets/to_client/extended/rotation.rs index acaee8e..5db4af3 100644 --- a/game/src/packets/to_client/extended/rotation.rs +++ b/game/src/packets/to_client/extended/rotation.rs @@ -18,7 +18,7 @@ impl Rotation { }; inst.buffer.write(Self::PACKET_ID)?; inst.buffer.write_u16(Self::EX_PACKET_ID)?; - inst.buffer.write_i32(player.char_model.id)?; + inst.buffer.write_i32(player.get_object_id())?; inst.buffer.write_i32(player.get_location().heading)?; Ok(inst) } diff --git a/game/src/packets/to_client/move_to.rs b/game/src/packets/to_client/move_to.rs index 18d01a7..40a0785 100644 --- a/game/src/packets/to_client/move_to.rs +++ b/game/src/packets/to_client/move_to.rs @@ -14,7 +14,7 @@ impl MoveTo { pub fn new(p: &Player, loc: &Location) -> anyhow::Result { let mut buffer = SendablePacketBuffer::new(); buffer.write(Self::PACKET_ID)?; - buffer.write_i32(p.char_model.id)?; + buffer.write_i32(p.get_object_id())?; let p_loc = p.get_location(); buffer.write_i32(p_loc.x)?; buffer.write_i32(p_loc.y)?; diff --git a/game/src/packets/to_client/relation_changed.rs b/game/src/packets/to_client/relation_changed.rs index adafabf..89357eb 100644 --- a/game/src/packets/to_client/relation_changed.rs +++ b/game/src/packets/to_client/relation_changed.rs @@ -45,7 +45,7 @@ impl RelationChangedBuilder { pub fn add_relation(mut self, player: &Player, relation: u32, auto_attackable: bool) -> Self { if !player.is_invisible() { let relation = Relation { - obj_id: player.char_model.id, + obj_id: player.get_object_id(), rel: relation, auto_attackable, reputation: player.char_model.reputation, diff --git a/game/src/packets/to_client/user_info.rs b/game/src/packets/to_client/user_info.rs index 406b5d4..b470b17 100644 --- a/game/src/packets/to_client/user_info.rs +++ b/game/src/packets/to_client/user_info.rs @@ -52,7 +52,7 @@ impl UserInfo { title: String::new(), }; inst.buffer.write(Self::PACKET_ID)?; - inst.buffer.write_i32(player.char_model.id)?; + inst.buffer.write_i32(player.get_object_id())?; inst.buffer.write_u32(inst.block_size)?; inst.buffer.write_u16(23u16)?; inst.buffer.write_bytes(inst.mask.flags())?; @@ -425,7 +425,7 @@ impl UserInfo { let mut relation = 0; if let Some(pt) = p.party.as_ref() { relation |= 0x08; - if pt.get_leader_id() == p.char_model.id { + if pt.get_leader_id() == p.get_object_id() { relation |= 0x10; } } @@ -492,7 +492,8 @@ mod tests { .class_templates .try_get_template(Class::try_from(char.class_id).unwrap()) .unwrap(); - let player = Player::new(char, vec![], template.clone()); + let mut player = Player::new(char, vec![], template.clone()); + player.object_id = 268_476_204; let p = UserInfo::new(&player, UserInfoType::all(), &controller) .await .unwrap(); diff --git a/l2-core/src/data/char_template.rs b/l2-core/src/data/char_template.rs index 0e3ff8f..de99667 100644 --- a/l2-core/src/data/char_template.rs +++ b/l2-core/src/data/char_template.rs @@ -139,12 +139,9 @@ impl CharTemplate { target.base_class_id = self.class_id as i8; target.access_level = 0; target.race_id = self.class_id.get_class().race as i8; - let base_max_hp = - self.get_base_max_parameter(target.level, &CreatureParameter::HP)?; - let base_max_mp = - self.get_base_max_parameter(target.level, &CreatureParameter::MP)?; - let base_max_cp = - self.get_base_max_parameter(target.level, &CreatureParameter::CP)?; + let base_max_hp = self.get_base_max_parameter(target.level, &CreatureParameter::HP)?; + let base_max_mp = self.get_base_max_parameter(target.level, &CreatureParameter::MP)?; + let base_max_cp = self.get_base_max_parameter(target.level, &CreatureParameter::CP)?; let base_con = base_stats.con_bonus(self.static_data.base_con)?; let base_men = base_stats.con_bonus(self.static_data.base_men)?; target.max_hp = f64::from(base_max_hp) * base_con; diff --git a/l2-core/src/game_objects/item/_item.rs b/l2-core/src/game_objects/item/_item.rs index c4c684f..2206166 100644 --- a/l2-core/src/game_objects/item/_item.rs +++ b/l2-core/src/game_objects/item/_item.rs @@ -1,4 +1,5 @@ use crate::game_objects::item::attribute::Attribute; +use crate::id_factory::IdFactory; use anyhow::bail; use entities::dao::item::ItemVariables; use entities::entities::item::Model; @@ -40,6 +41,7 @@ impl TryFrom for ItemListType { #[derive(Clone, Debug)] pub struct ItemObject { + pub object_id: i32, pub item_model: Model, } @@ -48,7 +50,16 @@ impl ItemObject { pub fn from_items(items: Vec) -> HashMap { items .into_iter() - .map(|item| (item.id, ItemObject { item_model: item })) + .map(|item| { + let object_id = IdFactory::instance().get_next_id(); + ( + object_id, + ItemObject { + object_id, + item_model: item, + }, + ) + }) .collect() } #[must_use] @@ -178,3 +189,9 @@ impl ItemObject { mask } } + +impl Drop for ItemObject { + fn drop(&mut self) { + IdFactory::instance().release_id(self.object_id); + } +} diff --git a/l2-core/src/game_objects/player/_player.rs b/l2-core/src/game_objects/player/_player.rs index c5edc51..c7ab01c 100644 --- a/l2-core/src/game_objects/player/_player.rs +++ b/l2-core/src/game_objects/player/_player.rs @@ -23,6 +23,7 @@ use log::info; use serde_json::Value; use std::fmt::Debug; use std::sync::Arc; +use crate::id_factory::IdFactory; #[repr(u8)] #[derive(Clone, Debug, Copy, Default)] @@ -49,6 +50,7 @@ impl From for u8 { #[derive(Debug, Clone)] pub struct Player { + pub object_id: i32, pub char_model: character::Model, pub skills: Option>, //None if not initialized pub quests: Vec, @@ -77,9 +79,12 @@ impl Player { items: Vec, template: Arc, ) -> Self { - let paperdoll = PaperDoll::restore_visible_inventory(&items); + let inventory = Inventory::from_items(items); + let paperdoll = PaperDoll::restore_visible_inventory(&inventory.items); assert_eq!(char_model.class_id, template.class_id as i8); + let object_id = IdFactory::instance().get_next_id(); Self { + object_id, location: Location { x: char_model.x, y: char_model.y, @@ -106,7 +111,7 @@ impl Player { siege_state: 0, appearance: Appearance, quest_zone_id: None, - inventory: Inventory::from_items(items), + inventory, } } @@ -144,8 +149,8 @@ impl Player { } #[must_use] - pub fn get_id(&self) -> i32 { - self.char_model.id + pub fn get_object_id(&self) -> i32 { + self.object_id } #[must_use] @@ -798,6 +803,15 @@ impl Player { //todo: implement me 0 } +} + +impl Drop for Player { + fn drop(&mut self) { + IdFactory::instance().release_id(self.object_id); + } +} + +impl Player { #[must_use] pub fn get_transformation_display_id(&self) -> i32 { //todo: implement me @@ -879,7 +893,7 @@ impl Player { && another_party.eq(party) { res |= RelationChanges::HasParty; - if let Some(pi) = party.index_of(self.char_model.id) { + if let Some(pi) = party.index_of(self.get_object_id()) { res |= RelationChanges::party_index_mask(pi); } } diff --git a/l2-core/src/game_objects/player/paper_doll.rs b/l2-core/src/game_objects/player/paper_doll.rs index 876f8ef..ad937d0 100644 --- a/l2-core/src/game_objects/player/paper_doll.rs +++ b/l2-core/src/game_objects/player/paper_doll.rs @@ -94,15 +94,16 @@ impl PaperDoll { ] } #[allow(clippy::cast_sign_loss)] - pub fn restore_visible_inventory(items: &Vec) -> [[i32; 4]; 33] { + pub fn restore_visible_inventory(items: &std::collections::HashMap) -> [[i32; 4]; 33] { let mut result = [[0; 4]; 33]; - for item in items { - if item.loc == LocType::Paperdoll { - let slot = item.loc_data; - result[slot as usize][0] = item.id; - result[slot as usize][1] = item.item_id; - result[slot as usize][2] = item.enchant_level; + for item in items.values() { + if item.item_model.loc == LocType::Paperdoll { + let slot = item.item_model.loc_data; + result[slot as usize][0] = item.object_id; + result[slot as usize][1] = item.item_model.item_id; + result[slot as usize][2] = item.item_model.enchant_level; result[slot as usize][3] = item + .item_model .variables .get(ItemVariables::VisualId.as_key()) .and_then(serde_json::Value::as_i64) diff --git a/l2-core/src/game_objects/player/party.rs b/l2-core/src/game_objects/player/party.rs index 1f80458..6f05246 100644 --- a/l2-core/src/game_objects/player/party.rs +++ b/l2-core/src/game_objects/player/party.rs @@ -42,7 +42,7 @@ impl Party { } #[must_use] pub fn get_leader_id(&self) -> i32 { - self.get_leader().char_model.id + self.get_leader().get_object_id() } #[must_use] @@ -50,11 +50,11 @@ impl Party { &self.players } #[must_use] - pub fn index_of(&self, player_id: i32) -> Option { + pub fn index_of(&self, player_object_id: i32) -> Option { self.players .iter() .enumerate() - .find(|(_, p)| p.char_model.id == player_id) + .find(|(_, p)| p.get_object_id() == player_object_id) .map(|(i, _)| i) .map(u32::try_from)? .ok() diff --git a/l2-core/src/game_objects/player/quest.rs b/l2-core/src/game_objects/player/quest.rs index 7c498fe..f6478a2 100644 --- a/l2-core/src/game_objects/player/quest.rs +++ b/l2-core/src/game_objects/player/quest.rs @@ -5,6 +5,7 @@ use sea_orm::JsonValue; #[derive(Debug, Clone)] pub struct Quest { pub model: quest::Model, + } impl Quest { diff --git a/l2-core/src/id_factory.rs b/l2-core/src/id_factory.rs new file mode 100644 index 0000000..dcf9687 --- /dev/null +++ b/l2-core/src/id_factory.rs @@ -0,0 +1,61 @@ +use std::sync::atomic::{AtomicI32, Ordering}; +use std::sync::{Arc, Mutex}; + +const FIRST_OID: i32 = 0x10000000; + +#[derive(Debug)] +pub struct IdFactory { + next_id: AtomicI32, + reusable_ids: Mutex>, +} + +impl IdFactory { + pub fn instance() -> Arc { + static INSTANCE: std::sync::OnceLock> = std::sync::OnceLock::new(); + INSTANCE + .get_or_init(|| { + Arc::new(IdFactory { + next_id: AtomicI32::new(FIRST_OID), + reusable_ids: Mutex::new(Vec::new()), + }) + }) + .clone() + } + + pub fn get_next_id(&self) -> i32 { + let mut reusable = self.reusable_ids.lock().unwrap(); + if let Some(id) = reusable.pop() { + return id; + } + self.next_id.fetch_add(1, Ordering::SeqCst) + } + + pub fn release_id(&self, id: i32) { + let mut reusable = self.reusable_ids.lock().unwrap(); + reusable.push(id); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_allocation() { + let factory = IdFactory::instance(); + let id1 = factory.get_next_id(); + let id2 = factory.get_next_id(); + assert_ne!(id1, id2); + assert!(id1 >= FIRST_OID); + assert!(id2 > id1); + } + + #[test] + fn test_reuse() { + let factory = IdFactory::instance(); + let id1 = factory.get_next_id(); + factory.release_id(id1); + let id2 = factory.get_next_id(); + assert_eq!(id1, id2); + } +} diff --git a/l2-core/src/lib.rs b/l2-core/src/lib.rs index a827c20..83e7606 100644 --- a/l2-core/src/lib.rs +++ b/l2-core/src/lib.rs @@ -22,6 +22,7 @@ pub mod bitmask; pub mod game_objects; pub mod utils; pub mod data; +pub mod id_factory; #[allow(clippy::missing_errors_doc, clippy::missing_panics_doc)] pub async fn hash_password(password: &str) -> anyhow::Result { From da474249e8bfabe8f358d8516b111ee5fd71e66e Mon Sep 17 00:00:00 2001 From: Artem Fedorov Date: Tue, 25 Nov 2025 19:40:51 +0100 Subject: [PATCH 2/8] Make it Drop only once when all references go out of scope --- game/src/packets/to_client/char_selected.rs | 6 +- .../to_client/extended/equipped_items.rs | 8 +- .../to_client/extended/inventory_weight.rs | 8 +- .../to_client/extended/premium_state.rs | 7 +- .../packets/to_client/extended/rotation.rs | 5 +- game/src/packets/to_client/quest_list.rs | 3 +- game/src/packets/to_client/skill_list.rs | 1 - game/src/packets/to_client/user_info.rs | 6 +- l2-core/src/game_objects/item/_item.rs | 13 +- l2-core/src/game_objects/player/_player.rs | 16 +-- l2-core/src/game_objects/player/paper_doll.rs | 2 +- l2-core/src/id_factory.rs | 111 +++++++++++++++--- login/src/enums.rs | 1 + login/src/packet/handleable/player_logout.rs | 2 +- 14 files changed, 130 insertions(+), 59 deletions(-) diff --git a/game/src/packets/to_client/char_selected.rs b/game/src/packets/to_client/char_selected.rs index 8ea19b6..f57b67c 100644 --- a/game/src/packets/to_client/char_selected.rs +++ b/game/src/packets/to_client/char_selected.rs @@ -61,6 +61,7 @@ mod tests { use entities::entities::character; use l2_core::config::traits::ConfigDirLoader; use l2_core::data::char_template::ClassTemplates; + use l2_core::id_factory::ObjectId; #[test] fn test_char_selected() { @@ -81,17 +82,16 @@ mod tests { x: -90939, y: 248_138, z: -3563, - id: 268_476_204, ..Default::default() }; let templates = ClassTemplates::load(); let temp = templates.try_get_template(inst.class_id).unwrap().clone(); let mut char = Player::new(inst, vec![], temp); - char.object_id = char.char_model.id; + char.object_id = ObjectId::new(268_476_207); let mut packet = CharSelected::new(&char, 9998, 286).unwrap(); assert_eq!( [ - 11, 65, 0, 100, 0, 101, 0, 108, 0, 97, 0, 110, 0, 116, 0, 101, 0, 0, 0, 44, + 11, 65, 0, 100, 0, 101, 0, 108, 0, 97, 0, 110, 0, 116, 0, 101, 0, 0, 0, 47, 159, 0, 16, 0, 0, 14, 39, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 10, 0, 0, 0, 1, 0, 0, 0, 197, 156, 254, 255, 74, 201, 3, 0, 21, 242, 255, 255, 0, 0, 0, 0, 0, 128, 88, 64, 0, 0, 0, 0, 0, 128, 77, 64, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, diff --git a/game/src/packets/to_client/extended/equipped_items.rs b/game/src/packets/to_client/extended/equipped_items.rs index 8e5f5ec..cf4c3f8 100644 --- a/game/src/packets/to_client/extended/equipped_items.rs +++ b/game/src/packets/to_client/extended/equipped_items.rs @@ -62,18 +62,18 @@ mod tests { use l2_core::shared_packets::common::SendablePacket; use l2_core::traits::ServerConfig; use std::sync::Arc; + use l2_core::id_factory::ObjectId; use test_utils::utils::get_test_db; #[tokio::test] async fn test_equipped_items() { let db_pool = get_test_db().await; let user = user_factory(&db_pool, |u| u).await; - let mut char = char_factory(&db_pool, |mut m| { + let char = char_factory(&db_pool, |mut m| { m.name = "Adelante".to_string(); m.user_id = user.id; m }) .await; - char.id = 268_476_204; let cfg = Arc::new(GSServerConfig::from_string(include_str!( "../../../../config/game.yaml" ))); @@ -83,11 +83,11 @@ mod tests { .try_get_template(Class::try_from(char.class_id).unwrap()) .unwrap(); let mut player = Player::new(char, vec![], template.clone()); - player.object_id = player.char_model.id; + player.object_id = ObjectId::new(268_476_206); let p = EquippedItems::new(&player, true).unwrap(); assert_eq!( [ - 254, 86, 1, 44, 159, 0, 16, 33, 0, 255, 255, 255, 255, 128, 22, 0, 0, 0, 0, 0, 0, + 254, 86, 1, 46, 159, 0, 16, 33, 0, 255, 255, 255, 255, 128, 22, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 22, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 22, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 22, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, diff --git a/game/src/packets/to_client/extended/inventory_weight.rs b/game/src/packets/to_client/extended/inventory_weight.rs index cdc80d3..95158c8 100644 --- a/game/src/packets/to_client/extended/inventory_weight.rs +++ b/game/src/packets/to_client/extended/inventory_weight.rs @@ -33,18 +33,18 @@ mod tests { use l2_core::shared_packets::common::SendablePacket; use l2_core::traits::ServerConfig; use std::sync::Arc; + use l2_core::id_factory::ObjectId; use test_utils::utils::get_test_db; #[tokio::test] async fn test_inventory_weight() { let db_pool = get_test_db().await; let user = user_factory(&db_pool, |u| u).await; - let mut char = char_factory(&db_pool, |mut m| { + let char = char_factory(&db_pool, |mut m| { m.name = "Adelante".to_string(); m.user_id = user.id; m }) .await; - char.id = 268_476_204; let cfg = Arc::new(GSServerConfig::from_string(include_str!( "../../../../config/game.yaml" ))); @@ -54,10 +54,10 @@ mod tests { .try_get_template(Class::try_from(char.class_id).unwrap()) .unwrap(); let mut player = Player::new(char, vec![], template.clone()); - player.object_id = player.char_model.id; + player.object_id = ObjectId::new(268_476_205); let p = InventoryWeight::new(&player).unwrap(); assert_eq!( - [254, 102, 1, 44, 159, 0, 16, 0, 0, 0, 0, 108, 24, 3, 0], + [254, 102, 1, 45, 159, 0, 16, 0, 0, 0, 0, 108, 24, 3, 0], p.get_buffer().get_data_mut(false)[2..] ); } diff --git a/game/src/packets/to_client/extended/premium_state.rs b/game/src/packets/to_client/extended/premium_state.rs index efb1229..11908e5 100644 --- a/game/src/packets/to_client/extended/premium_state.rs +++ b/game/src/packets/to_client/extended/premium_state.rs @@ -30,6 +30,7 @@ mod test { use l2_core::shared_packets::common::SendablePacket; use l2_core::traits::ServerConfig; use std::sync::Arc; + use l2_core::id_factory::ObjectId; use test_utils::utils::get_test_db; use super::*; @@ -43,7 +44,6 @@ mod test { m }) .await; - char.id = 268_476_204; let cfg = Arc::new(GSServerConfig::from_string(include_str!( "../../../../config/game.yaml" ))); @@ -52,10 +52,11 @@ mod test { .class_templates .try_get_template(Class::try_from(char.class_id).unwrap()) .unwrap(); - let player = Player::new(char, vec![], template.clone()); + let mut player = Player::new(char, vec![], template.clone()); + player.object_id = ObjectId::new(268_476_209); let p = PremiumState::new(&player).unwrap(); assert_eq!( - [254, 218, 0, 44, 159, 0, 16, 0], + [254, 218, 0, 49, 159, 0, 16, 0], p.get_buffer().get_data_mut(false)[2..] ); } diff --git a/game/src/packets/to_client/extended/rotation.rs b/game/src/packets/to_client/extended/rotation.rs index 5db4af3..439dc5b 100644 --- a/game/src/packets/to_client/extended/rotation.rs +++ b/game/src/packets/to_client/extended/rotation.rs @@ -35,6 +35,7 @@ mod tests { use l2_core::shared_packets::common::SendablePacket; use l2_core::traits::ServerConfig; use std::sync::Arc; + use l2_core::id_factory::ObjectId; use test_utils::utils::get_test_db; #[tokio::test] async fn test_rotation_packet() { @@ -46,7 +47,6 @@ mod tests { m }) .await; - char.id = 268_476_204; let cfg = Arc::new(GSServerConfig::from_string(include_str!( "../../../../config/game.yaml" ))); @@ -56,10 +56,11 @@ mod tests { .try_get_template(Class::try_from(char.class_id).unwrap()) .unwrap(); let mut player = Player::new(char, vec![], template.clone()); + player.object_id = ObjectId::new(268_476_210); player.set_location_heading(33897); let p = Rotation::new(&player).unwrap(); assert_eq!( - [0, 254, 194, 0, 44, 159, 0, 16, 105, 132, 0, 0], + [0, 254, 194, 0, 50, 159, 0, 16, 105, 132, 0, 0], p.get_buffer().get_data_mut(false)[1..] ); } diff --git a/game/src/packets/to_client/quest_list.rs b/game/src/packets/to_client/quest_list.rs index 56bd618..971a3d1 100644 --- a/game/src/packets/to_client/quest_list.rs +++ b/game/src/packets/to_client/quest_list.rs @@ -48,6 +48,7 @@ mod tests { use l2_core::traits::ServerConfig; use std::sync::Arc; use sea_orm::JsonValue; + use l2_core::id_factory::ObjectId; use test_utils::utils::get_test_db; #[tokio::test] async fn test_quest_list_packet() { @@ -59,7 +60,6 @@ mod tests { m }) .await; - char.id = 268_476_204; let cfg = Arc::new(GSServerConfig::from_string(include_str!( "../../../../config/game.yaml" ))); @@ -69,6 +69,7 @@ mod tests { .try_get_template(Class::try_from(char.class_id).unwrap()) .unwrap(); let mut player = Player::new(char, vec![], template.clone()); + player.object_id = ObjectId::new(268_476_208); player.quests.push(Quest { model: quest::Model { char_id: player.char_model.id, diff --git a/game/src/packets/to_client/skill_list.rs b/game/src/packets/to_client/skill_list.rs index f0d38ef..9fbb6f6 100644 --- a/game/src/packets/to_client/skill_list.rs +++ b/game/src/packets/to_client/skill_list.rs @@ -62,7 +62,6 @@ mod test { is_female: true, delete_at: None, user_id: 1, - id: 268_476_204, ..Default::default() }; let templates = ClassTemplates::load(); diff --git a/game/src/packets/to_client/user_info.rs b/game/src/packets/to_client/user_info.rs index b470b17..ff17b7f 100644 --- a/game/src/packets/to_client/user_info.rs +++ b/game/src/packets/to_client/user_info.rs @@ -451,6 +451,7 @@ mod tests { use sea_orm::JsonValue; use std::str::FromStr; use std::sync::Arc; + use l2_core::id_factory::ObjectId; use test_utils::utils::get_test_db; #[tokio::test] @@ -483,7 +484,6 @@ mod tests { }) .await; - char.id = 268_476_204; let cfg = Arc::new(GSServerConfig::from_string(include_str!( "../../../../config/game.yaml" ))); @@ -493,7 +493,7 @@ mod tests { .try_get_template(Class::try_from(char.class_id).unwrap()) .unwrap(); let mut player = Player::new(char, vec![], template.clone()); - player.object_id = 268_476_204; + player.object_id = ObjectId::new(268_476_204); let p = UserInfo::new(&player, UserInfoType::all(), &controller) .await .unwrap(); @@ -517,7 +517,7 @@ mod tests { assert_eq!( [ 50, //packet id - 44, 159, 0, 16, //cjar model id + 44, 159, 0, 16, //char model id 137, 1, 0, 0, //block size 23, 0, //23 255, 255, 254, //flags diff --git a/l2-core/src/game_objects/item/_item.rs b/l2-core/src/game_objects/item/_item.rs index 2206166..d71837b 100644 --- a/l2-core/src/game_objects/item/_item.rs +++ b/l2-core/src/game_objects/item/_item.rs @@ -1,11 +1,12 @@ use crate::game_objects::item::attribute::Attribute; -use crate::id_factory::IdFactory; +use crate::id_factory::{IdFactory, ObjectId}; use anyhow::bail; use entities::dao::item::ItemVariables; use entities::entities::item::Model; use log::error; use serde_json::Value; use std::collections::HashMap; +use std::sync::Arc; #[repr(u8)] #[derive(Clone, Debug, PartialEq, Eq, Hash, Ord, PartialOrd)] @@ -41,7 +42,7 @@ impl TryFrom for ItemListType { #[derive(Clone, Debug)] pub struct ItemObject { - pub object_id: i32, + pub object_id: ObjectId, pub item_model: Model, } @@ -53,7 +54,7 @@ impl ItemObject { .map(|item| { let object_id = IdFactory::instance().get_next_id(); ( - object_id, + object_id.clone().into(), ItemObject { object_id, item_model: item, @@ -189,9 +190,3 @@ impl ItemObject { mask } } - -impl Drop for ItemObject { - fn drop(&mut self) { - IdFactory::instance().release_id(self.object_id); - } -} diff --git a/l2-core/src/game_objects/player/_player.rs b/l2-core/src/game_objects/player/_player.rs index c7ab01c..537fd46 100644 --- a/l2-core/src/game_objects/player/_player.rs +++ b/l2-core/src/game_objects/player/_player.rs @@ -17,13 +17,13 @@ use crate::game_objects::player::{PlayerMacro, SubclassType, TeleportBookmark}; use crate::game_objects::private_store_types::PrivateStoreType; use crate::game_objects::race::Race; use crate::game_objects::zone::{Location, ZoneId}; +use crate::id_factory::{IdFactory, ObjectId}; use chrono::Utc; use entities::entities::{character, character_mail, clan_ally, item}; use log::info; use serde_json::Value; use std::fmt::Debug; use std::sync::Arc; -use crate::id_factory::IdFactory; #[repr(u8)] #[derive(Clone, Debug, Copy, Default)] @@ -50,7 +50,7 @@ impl From for u8 { #[derive(Debug, Clone)] pub struct Player { - pub object_id: i32, + pub object_id: ObjectId, pub char_model: character::Model, pub skills: Option>, //None if not initialized pub quests: Vec, @@ -150,7 +150,7 @@ impl Player { #[must_use] pub fn get_object_id(&self) -> i32 { - self.object_id + self.object_id.value() } #[must_use] @@ -473,12 +473,12 @@ impl Player { 113 } - #[must_use] + #[must_use] pub fn get_fly_run_speed(&self) -> u16 { //todo: implement me 159 } - #[must_use] + #[must_use] pub fn get_fly_walk_speed(&self) -> u16 { //todo: implement me 113 @@ -805,12 +805,6 @@ impl Player { } } -impl Drop for Player { - fn drop(&mut self) { - IdFactory::instance().release_id(self.object_id); - } -} - impl Player { #[must_use] pub fn get_transformation_display_id(&self) -> i32 { diff --git a/l2-core/src/game_objects/player/paper_doll.rs b/l2-core/src/game_objects/player/paper_doll.rs index ad937d0..50afe33 100644 --- a/l2-core/src/game_objects/player/paper_doll.rs +++ b/l2-core/src/game_objects/player/paper_doll.rs @@ -99,7 +99,7 @@ impl PaperDoll { for item in items.values() { if item.item_model.loc == LocType::Paperdoll { let slot = item.item_model.loc_data; - result[slot as usize][0] = item.object_id; + result[slot as usize][0] = item.object_id.value(); result[slot as usize][1] = item.item_model.item_id; result[slot as usize][2] = item.item_model.enchant_level; result[slot as usize][3] = item diff --git a/l2-core/src/id_factory.rs b/l2-core/src/id_factory.rs index dcf9687..6ca38bb 100644 --- a/l2-core/src/id_factory.rs +++ b/l2-core/src/id_factory.rs @@ -1,12 +1,62 @@ +use dashmap::DashSet; +use log::warn; use std::sync::atomic::{AtomicI32, Ordering}; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; -const FIRST_OID: i32 = 0x10000000; +const FIRST_OID: i32 = 0x1000_0000; #[derive(Debug)] pub struct IdFactory { next_id: AtomicI32, - reusable_ids: Mutex>, + reusable_ids: DashSet, +} +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct ObjectId(Arc); + +impl ObjectId { + #[must_use] + pub fn new(id: i32) -> Self { + Self(Arc::new(id)) + } + #[must_use] + pub fn value(&self) -> i32 { + *self.0 + } +} + +impl From for i32 { + fn from(id: ObjectId) -> i32 { + *id.0 + } +} + +impl PartialEq for ObjectId { + fn eq(&self, other: &i32) -> bool { + *self.0 == *other + } +} +impl PartialEq for i32 { + fn eq(&self, other: &ObjectId) -> bool { + *self == *other.0 + } +} +impl PartialOrd for i32 { + fn partial_cmp(&self, other: &ObjectId) -> Option { + self.partial_cmp(&*other.0) + } +} +impl Drop for ObjectId { + fn drop(&mut self) { + // Only release if this is the last Arc + if Arc::strong_count(&self.0) == 1 { + IdFactory::instance().release_id(*self.0); + } + } +} +impl PartialOrd for ObjectId { + fn partial_cmp(&self, other: &i32) -> Option { + (*self.0).partial_cmp(other) + } } impl IdFactory { @@ -16,23 +66,32 @@ impl IdFactory { .get_or_init(|| { Arc::new(IdFactory { next_id: AtomicI32::new(FIRST_OID), - reusable_ids: Mutex::new(Vec::new()), + reusable_ids: DashSet::new(), }) }) .clone() } - pub fn get_next_id(&self) -> i32 { - let mut reusable = self.reusable_ids.lock().unwrap(); - if let Some(id) = reusable.pop() { - return id; + pub fn get_next_id(&self) -> ObjectId { + //be careful, don't hold shared reference to self.reusable_ids.iter() + // and modifying it in the same time, because it will deadlock + let id_val = { + let mut iter = self.reusable_ids.iter(); + iter.next().map(|r| *r) // copy value + }; + //at this stage self.reusable_ids shared lock is released, so we can safely modify it + if let Some(id_ref) = id_val { + self.reusable_ids.remove(&id_ref); + return ObjectId::new(id_ref); } - self.next_id.fetch_add(1, Ordering::SeqCst) + ObjectId::new(self.next_id.fetch_add(1, Ordering::SeqCst)) } - pub fn release_id(&self, id: i32) { - let mut reusable = self.reusable_ids.lock().unwrap(); - reusable.push(id); + pub fn release_id(&self, id: impl Into) { + let val = id.into(); + if !self.reusable_ids.insert(val) { + warn!("Trying to release already released id: {val}"); + } } } @@ -46,16 +105,36 @@ mod tests { let id1 = factory.get_next_id(); let id2 = factory.get_next_id(); assert_ne!(id1, id2); - assert!(id1 >= FIRST_OID); + assert!(id1 >= ObjectId::new(FIRST_OID)); assert!(id2 > id1); } #[test] fn test_reuse() { let factory = IdFactory::instance(); - let id1 = factory.get_next_id(); - factory.release_id(id1); + let id_copy: i32; + { + let id1 = factory.get_next_id(); + id_copy = id1.clone().into(); + } //drop id1 + let id2 = factory.get_next_id(); + assert_eq!(id2, id_copy); + } + + #[test] + fn test_cloned() { + let factory = IdFactory::instance(); + let id_copy: i32; + { + let id1 = factory.get_next_id(); + let id2 = id1.clone(); + assert_eq!(id1, id2); + drop(id1); + //id1 is dropped, but the clone id2 is still in the scope, so it should not be released yet + assert!(!IdFactory::instance().reusable_ids.contains(&*id2.0)); + id_copy = id2.into(); + } //drop id1 let id2 = factory.get_next_id(); - assert_eq!(id1, id2); + assert_eq!(id2, id_copy); } } diff --git a/login/src/enums.rs b/login/src/enums.rs index cb6f537..5587058 100644 --- a/login/src/enums.rs +++ b/login/src/enums.rs @@ -1,5 +1,6 @@ use l2_core::shared_packets::common::GSLoginFailReasons; use strum::Display; +use tracing::{error, info}; #[derive(Debug, Clone, Display, Eq, PartialEq, Hash, Copy, PartialOrd, Ord)] pub enum GS { diff --git a/login/src/packet/handleable/player_logout.rs b/login/src/packet/handleable/player_logout.rs index 63dafe9..24898d2 100644 --- a/login/src/packet/handleable/player_logout.rs +++ b/login/src/packet/handleable/player_logout.rs @@ -48,7 +48,7 @@ mod tests { let (_client, server) = tokio::io::duplex(1024); let cfg = LoginServerConfig::from_string(include_str!("../../../../config/login.yaml")); let lc = Arc::new(LoginController::new(Arc::new(cfg))); - lc.on_players_in_game(1, &[acc.clone()]); // hack to insert players + lc.on_players_in_game(1, std::slice::from_ref(&acc)); // hack to insert players assert!(lc.get_player("admin").is_some()); let (r, w) = split(server); let gs_actor = spawn_gs_client_actor(lc.clone(), db_pool, r, w).await; From 5ed40eec79f7928837a8b5aefb72c0f7f6814de7 Mon Sep 17 00:00:00 2001 From: Artem Fedorov Date: Wed, 26 Nov 2025 00:16:16 +0100 Subject: [PATCH 3/8] Fix some init tests, they were running in parallel and so there was an issue with accessing static global variable --- Cargo.lock | 41 ++++++++++++++++++++++++ Cargo.toml | 1 + l2-core/Cargo.toml | 3 +- l2-core/src/game_objects/player/quest.rs | 14 ++------ l2-core/src/id_factory.rs | 4 +++ 5 files changed, 51 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e774dc9..27ad01f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1591,6 +1591,7 @@ dependencies = [ "serde", "serde_json", "serde_yaml", + "serial_test", "sqlx", "strum 0.27.2", "strum_macros", @@ -2571,12 +2572,27 @@ dependencies = [ "winapi-util", ] +[[package]] +name = "scc" +version = "2.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "46e6f046b7fef48e2660c57ed794263155d713de679057f2d0c169bfc6e756cc" +dependencies = [ + "sdd", +] + [[package]] name = "scopeguard" version = "1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "94143f37725109f92c262ed2cf5e59bce7498c01bcc1502d7b9afe439a4e9f49" +[[package]] +name = "sdd" +version = "3.0.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "490dcfcbfef26be6800d11870ff2df8774fa6e86d047e3e8c8a76b25655e41ca" + [[package]] name = "sea-bae" version = "0.2.1" @@ -2802,6 +2818,31 @@ dependencies = [ "unsafe-libyaml", ] +[[package]] +name = "serial_test" +version = "3.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1b258109f244e1d6891bf1053a55d63a5cd4f8f4c30cf9a1280989f80e7a1fa9" +dependencies = [ + "futures", + "log", + "once_cell", + "parking_lot", + "scc", + "serial_test_derive", +] + +[[package]] +name = "serial_test_derive" +version = "3.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5d69265a08751de7844521fd15003ae0a888e035773ba05695c5c759a6f89eef" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.96", +] + [[package]] name = "sha1" version = "0.10.6" diff --git a/Cargo.toml b/Cargo.toml index a1d31eb..987f85c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -60,3 +60,4 @@ strum = { version = "0.27.2", features = ["derive"] } async-std = { version = "1.13.1", features = ["attributes", "tokio1"] } bytes = "1.10.1" ntest = "0.9.3" +serial_test = "3.2.0" diff --git a/l2-core/Cargo.toml b/l2-core/Cargo.toml index d6c897c..1d96f3d 100644 --- a/l2-core/Cargo.toml +++ b/l2-core/Cargo.toml @@ -43,4 +43,5 @@ kameo.workspace = true [dev-dependencies] test-utils = { path = "../test-utils" } -ntest.workspace = true \ No newline at end of file +ntest.workspace = true +serial_test.workspace = true \ No newline at end of file diff --git a/l2-core/src/game_objects/player/quest.rs b/l2-core/src/game_objects/player/quest.rs index f6478a2..3e9e87c 100644 --- a/l2-core/src/game_objects/player/quest.rs +++ b/l2-core/src/game_objects/player/quest.rs @@ -5,26 +5,18 @@ use sea_orm::JsonValue; #[derive(Debug, Clone)] pub struct Quest { pub model: quest::Model, - } impl Quest { - #[must_use] pub fn get_id(&self) -> i32 { self.model.quest_id } - pub fn has_state(&self, state: &str) -> bool { - if let Some(state) = self - .model + pub fn has_state(&self, expected_state: &str) -> bool { + self.model .variables .get(QuestVariables::State.as_key()) .and_then(JsonValue::as_str) - && state.eq(state) - { - true - } else { - false - } + .is_some_and(|state| state.eq(expected_state)) } #[must_use] diff --git a/l2-core/src/id_factory.rs b/l2-core/src/id_factory.rs index 6ca38bb..0595f76 100644 --- a/l2-core/src/id_factory.rs +++ b/l2-core/src/id_factory.rs @@ -100,6 +100,7 @@ mod tests { use super::*; #[test] + #[serial_test::serial] fn test_allocation() { let factory = IdFactory::instance(); let id1 = factory.get_next_id(); @@ -110,18 +111,21 @@ mod tests { } #[test] + #[serial_test::serial] fn test_reuse() { let factory = IdFactory::instance(); let id_copy: i32; { let id1 = factory.get_next_id(); id_copy = id1.clone().into(); + drop(id1); } //drop id1 let id2 = factory.get_next_id(); assert_eq!(id2, id_copy); } #[test] + #[serial_test::serial] fn test_cloned() { let factory = IdFactory::instance(); let id_copy: i32; From 298a1ec18dc9847f46407043910b44f694596f40 Mon Sep 17 00:00:00 2001 From: Artem Fedorov Date: Wed, 26 Nov 2025 00:37:33 +0100 Subject: [PATCH 4/8] Fix some init tests, they were running in parallel and so there was an issue with accessing static global variable --- Cargo.lock | 1 - l2-core/Cargo.toml | 1 - 2 files changed, 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 27ad01f..785038c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1577,7 +1577,6 @@ dependencies = [ "kameo", "log", "macro-common", - "ntest", "num", "num-traits", "num_enum", diff --git a/l2-core/Cargo.toml b/l2-core/Cargo.toml index 1d96f3d..9f13ee9 100644 --- a/l2-core/Cargo.toml +++ b/l2-core/Cargo.toml @@ -43,5 +43,4 @@ kameo.workspace = true [dev-dependencies] test-utils = { path = "../test-utils" } -ntest.workspace = true serial_test.workspace = true \ No newline at end of file From 3f92c2b413ece0ef7ef4a3ae2ff42a65016bdcd5 Mon Sep 17 00:00:00 2001 From: Artem Fedorov Date: Thu, 27 Nov 2025 17:05:33 +0100 Subject: [PATCH 5/8] Updating global id generation and fix unit tests --- l2-core/src/id_factory.rs | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/l2-core/src/id_factory.rs b/l2-core/src/id_factory.rs index 0595f76..a126b19 100644 --- a/l2-core/src/id_factory.rs +++ b/l2-core/src/id_factory.rs @@ -1,14 +1,15 @@ use dashmap::DashSet; use log::warn; +use std::collections::HashSet; use std::sync::atomic::{AtomicI32, Ordering}; -use std::sync::Arc; +use std::sync::{Arc, Mutex}; const FIRST_OID: i32 = 0x1000_0000; #[derive(Debug)] pub struct IdFactory { next_id: AtomicI32, - reusable_ids: DashSet, + reusable_ids: Mutex>, } #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct ObjectId(Arc); @@ -66,33 +67,32 @@ impl IdFactory { .get_or_init(|| { Arc::new(IdFactory { next_id: AtomicI32::new(FIRST_OID), - reusable_ids: DashSet::new(), + reusable_ids: Mutex::new(HashSet::new()), }) }) .clone() } pub fn get_next_id(&self) -> ObjectId { - //be careful, don't hold shared reference to self.reusable_ids.iter() - // and modifying it in the same time, because it will deadlock - let id_val = { - let mut iter = self.reusable_ids.iter(); - iter.next().map(|r| *r) // copy value + let mut set = self.get_locked_state(); + let Some(&id) = set.iter().next() else { + return ObjectId::new(self.next_id.fetch_add(1, Ordering::SeqCst)); }; - //at this stage self.reusable_ids shared lock is released, so we can safely modify it - if let Some(id_ref) = id_val { - self.reusable_ids.remove(&id_ref); - return ObjectId::new(id_ref); - } - ObjectId::new(self.next_id.fetch_add(1, Ordering::SeqCst)) + set.remove(&id); + ObjectId::new(id) } pub fn release_id(&self, id: impl Into) { let val = id.into(); - if !self.reusable_ids.insert(val) { + if !self.get_locked_state().insert(val) { warn!("Trying to release already released id: {val}"); } } + fn get_locked_state(&self) -> std::sync::MutexGuard<'_, HashSet> { + self.reusable_ids + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + } } #[cfg(test)] @@ -135,7 +135,7 @@ mod tests { assert_eq!(id1, id2); drop(id1); //id1 is dropped, but the clone id2 is still in the scope, so it should not be released yet - assert!(!IdFactory::instance().reusable_ids.contains(&*id2.0)); + assert!(!IdFactory::instance().get_locked_state().contains(&*id2.0)); id_copy = id2.into(); } //drop id1 let id2 = factory.get_next_id(); From 5691f46bd1561074c3a52048a954aaed43b379b8 Mon Sep 17 00:00:00 2001 From: Artem Fedorov Date: Thu, 27 Nov 2025 17:28:46 +0100 Subject: [PATCH 6/8] Fixes accordingly to review --- Cargo.lock | 41 --------------------------------------- Cargo.toml | 1 - l2-core/Cargo.toml | 3 +-- l2-core/src/id_factory.rs | 13 ++++++++++--- 4 files changed, 11 insertions(+), 47 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 785038c..d515501 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1590,7 +1590,6 @@ dependencies = [ "serde", "serde_json", "serde_yaml", - "serial_test", "sqlx", "strum 0.27.2", "strum_macros", @@ -2571,27 +2570,12 @@ dependencies = [ "winapi-util", ] -[[package]] -name = "scc" -version = "2.4.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "46e6f046b7fef48e2660c57ed794263155d713de679057f2d0c169bfc6e756cc" -dependencies = [ - "sdd", -] - [[package]] name = "scopeguard" version = "1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "94143f37725109f92c262ed2cf5e59bce7498c01bcc1502d7b9afe439a4e9f49" -[[package]] -name = "sdd" -version = "3.0.10" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "490dcfcbfef26be6800d11870ff2df8774fa6e86d047e3e8c8a76b25655e41ca" - [[package]] name = "sea-bae" version = "0.2.1" @@ -2817,31 +2801,6 @@ dependencies = [ "unsafe-libyaml", ] -[[package]] -name = "serial_test" -version = "3.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1b258109f244e1d6891bf1053a55d63a5cd4f8f4c30cf9a1280989f80e7a1fa9" -dependencies = [ - "futures", - "log", - "once_cell", - "parking_lot", - "scc", - "serial_test_derive", -] - -[[package]] -name = "serial_test_derive" -version = "3.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5d69265a08751de7844521fd15003ae0a888e035773ba05695c5c759a6f89eef" -dependencies = [ - "proc-macro2", - "quote", - "syn 2.0.96", -] - [[package]] name = "sha1" version = "0.10.6" diff --git a/Cargo.toml b/Cargo.toml index 987f85c..a1d31eb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -60,4 +60,3 @@ strum = { version = "0.27.2", features = ["derive"] } async-std = { version = "1.13.1", features = ["attributes", "tokio1"] } bytes = "1.10.1" ntest = "0.9.3" -serial_test = "3.2.0" diff --git a/l2-core/Cargo.toml b/l2-core/Cargo.toml index 9f13ee9..6a97e86 100644 --- a/l2-core/Cargo.toml +++ b/l2-core/Cargo.toml @@ -42,5 +42,4 @@ serde_json.workspace = true kameo.workspace = true [dev-dependencies] -test-utils = { path = "../test-utils" } -serial_test.workspace = true \ No newline at end of file +test-utils = { path = "../test-utils" } \ No newline at end of file diff --git a/l2-core/src/id_factory.rs b/l2-core/src/id_factory.rs index a126b19..a692e2b 100644 --- a/l2-core/src/id_factory.rs +++ b/l2-core/src/id_factory.rs @@ -98,11 +98,18 @@ impl IdFactory { #[cfg(test)] mod tests { use super::*; + impl IdFactory { + fn reset_for_tests(&self) { + let mut set = self.get_locked_state(); + set.clear(); + self.next_id.store(FIRST_OID, Ordering::SeqCst); + } + } #[test] - #[serial_test::serial] fn test_allocation() { let factory = IdFactory::instance(); + factory.reset_for_tests(); let id1 = factory.get_next_id(); let id2 = factory.get_next_id(); assert_ne!(id1, id2); @@ -111,9 +118,9 @@ mod tests { } #[test] - #[serial_test::serial] fn test_reuse() { let factory = IdFactory::instance(); + factory.reset_for_tests(); let id_copy: i32; { let id1 = factory.get_next_id(); @@ -125,9 +132,9 @@ mod tests { } #[test] - #[serial_test::serial] fn test_cloned() { let factory = IdFactory::instance(); + factory.reset_for_tests(); let id_copy: i32; { let id1 = factory.get_next_id(); From 3b8366ac366ad61e5e235b55fe7b8d1817ee65b3 Mon Sep 17 00:00:00 2001 From: Artem Fedorov Date: Thu, 27 Nov 2025 17:37:28 +0100 Subject: [PATCH 7/8] Relaxing tests to not consider ordering in global hashset --- l2-core/src/id_factory.rs | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/l2-core/src/id_factory.rs b/l2-core/src/id_factory.rs index a692e2b..78e20f0 100644 --- a/l2-core/src/id_factory.rs +++ b/l2-core/src/id_factory.rs @@ -114,7 +114,6 @@ mod tests { let id2 = factory.get_next_id(); assert_ne!(id1, id2); assert!(id1 >= ObjectId::new(FIRST_OID)); - assert!(id2 > id1); } #[test] @@ -127,25 +126,19 @@ mod tests { id_copy = id1.clone().into(); drop(id1); } //drop id1 - let id2 = factory.get_next_id(); - assert_eq!(id2, id_copy); + //it appears in reusable_ids + assert!(factory.get_locked_state().contains(&id_copy)); } #[test] fn test_cloned() { let factory = IdFactory::instance(); factory.reset_for_tests(); - let id_copy: i32; - { - let id1 = factory.get_next_id(); - let id2 = id1.clone(); - assert_eq!(id1, id2); - drop(id1); - //id1 is dropped, but the clone id2 is still in the scope, so it should not be released yet - assert!(!IdFactory::instance().get_locked_state().contains(&*id2.0)); - id_copy = id2.into(); - } //drop id1 - let id2 = factory.get_next_id(); - assert_eq!(id2, id_copy); + let id1 = factory.get_next_id(); + let id2 = id1.clone(); + assert_eq!(id1, id2); + drop(id1); + //id1 is dropped, but the clone id2 is still in the scope, so it should not be released yet + assert!(!IdFactory::instance().get_locked_state().contains(&*id2.0)); } } From 7e861cf99d8ace41d70b8198e3f7371bdf2333c1 Mon Sep 17 00:00:00 2001 From: Artem Fedorov Date: Thu, 27 Nov 2025 17:58:13 +0100 Subject: [PATCH 8/8] remove unused import --- l2-core/src/id_factory.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/l2-core/src/id_factory.rs b/l2-core/src/id_factory.rs index 78e20f0..558e101 100644 --- a/l2-core/src/id_factory.rs +++ b/l2-core/src/id_factory.rs @@ -1,4 +1,3 @@ -use dashmap::DashSet; use log::warn; use std::collections::HashSet; use std::sync::atomic::{AtomicI32, Ordering};