From 9123c4842b81672c25ff4a7802fe32cbbd70fced Mon Sep 17 00:00:00 2001 From: jake Date: Tue, 31 Jan 2023 19:22:11 -0700 Subject: [PATCH] start of fixing item_id parity issues --- src/ship/items/actions.rs | 34 ++++++++++- src/ship/items/apply_item.rs | 4 +- src/ship/items/bank.rs | 27 ++++++--- src/ship/items/inventory.rs | 93 +++++++++++++++++++++++------- src/ship/items/state.rs | 63 ++++++++++++++++---- src/ship/items/tasks.rs | 2 +- src/ship/ship.rs | 1 - tests/test_bank.rs | 2 +- tests/test_item_pickup.rs | 2 +- tests/test_item_use.rs | 1 + tests/test_rooms.rs | 109 +---------------------------------- 11 files changed, 182 insertions(+), 156 deletions(-) diff --git a/src/ship/items/actions.rs b/src/ship/items/actions.rs index 984fdbd..53a59db 100644 --- a/src/ship/items/actions.rs +++ b/src/ship/items/actions.rs @@ -97,6 +97,30 @@ where } +pub(super) fn remove_item_from_inventory( + character_id: CharacterEntityId, + item_id: ClientItemId, + amount: u32, +) -> impl Fn((ItemStateProxy, TR), ()) + -> BoxFuture> +where + EG: EntityGateway, + TR: EntityGatewayTransaction + 'static, +{ + move |(mut item_state, mut transaction), _| { + Box::pin(async move { + let mut inventory = item_state.inventory(&character_id).await?; + let item = inventory.remove_item(&item_id, amount) + .await + .ok_or_else(|| ItemStateError::NoInventoryItem(item_id))?; + + transaction.gateway().set_character_inventory(&character_id, &inventory.as_inventory_entity(&character_id)).await?; + item_state.set_inventory(inventory).await; + + Ok(((item_state, transaction), item)) + }) + } +} pub(super) fn take_item_from_inventory( character_id: CharacterEntityId, @@ -111,7 +135,9 @@ where move |(mut item_state, mut transaction), _| { Box::pin(async move { let mut inventory = item_state.inventory(&character_id).await?; - let item = inventory.take_item(&item_id, amount).ok_or_else(|| ItemStateError::NoInventoryItem(item_id))?; + let item = inventory.take_item(&item_id, amount) + .await + .ok_or_else(|| ItemStateError::NoInventoryItem(item_id))?; transaction.gateway().set_character_inventory(&character_id, &inventory.as_inventory_entity(&character_id)).await?; item_state.set_inventory(inventory).await; @@ -306,7 +332,9 @@ where move |(mut item_state, mut transaction), _| { Box::pin(async move { let mut bank = item_state.bank(&character_id).await?; - let item = bank.take_item(&item_id, amount).ok_or_else(|| ItemStateError::NoBankItem(item_id))?; + let item = bank.take_item(&item_id, amount) + .await + .ok_or_else(|| ItemStateError::NoBankItem(item_id))?; transaction.gateway().set_character_bank(&character_id, &bank.as_bank_entity(), &bank.name).await?; item_state.set_bank(bank).await; @@ -471,7 +499,7 @@ where pub(super) fn use_consumed_item( character: &CharacterEntity, -) -> impl Fn((ItemStateProxy, TR), InventoryItem) +) -> impl Fn((ItemStateProxy, TR), InventoryItemDetail) -> BoxFuture), anyhow::Error>> where EG: EntityGateway + Clone + 'static, diff --git a/src/ship/items/apply_item.rs b/src/ship/items/apply_item.rs index ad2f27d..d73941e 100644 --- a/src/ship/items/apply_item.rs +++ b/src/ship/items/apply_item.rs @@ -356,12 +356,12 @@ where pub async fn apply_item<'a, EG>(item_state: &mut ItemStateProxy, entity_gateway: &mut EG, character: &mut CharacterEntity, - item: InventoryItem + item: InventoryItemDetail ) -> Result, anyhow::Error> where EG: EntityGateway + ?Sized + Clone + 'static { - match item.item { + match item { InventoryItemDetail::Individual(individual_item) => { match individual_item.item { ItemDetail::Tool(tool) => apply_tool(item_state, entity_gateway, character, individual_item.entity_id, tool.tool).await, diff --git a/src/ship/items/bank.rs b/src/ship/items/bank.rs index 5494102..4f922af 100644 --- a/src/ship/items/bank.rs +++ b/src/ship/items/bank.rs @@ -3,6 +3,7 @@ use libpso::character::character; use crate::ship::items::ClientItemId; use crate::entity::item::{Meseta, ItemEntityId, ItemDetail, ItemEntity, BankEntity, BankItemEntity, BankName}; use std::future::Future; +use async_std::sync::{Arc, Mutex}; use crate::entity::character::CharacterEntityId; use crate::ship::items::state::ItemStateError; @@ -96,18 +97,26 @@ impl Bank { #[derive(Clone, Debug)] pub struct BankState { pub character_id: CharacterEntityId, - pub item_id_counter: u32, + pub item_id_counter: Arc>, pub name: BankName, pub bank: Bank, pub meseta: Meseta, } +async fn new_item_id(item_id_counter: &Arc>) -> ClientItemId { + let mut item_id_counter = item_id_counter.lock().await; + let item_id = *item_id_counter; + *item_id_counter += 1; + + ClientItemId(item_id) +} + impl BankState { pub fn new(character_id: CharacterEntityId, name: BankName, mut bank: Bank, meseta: Meseta) -> BankState { bank.0.sort(); BankState { character_id, - item_id_counter: 0, + item_id_counter: Arc::new(Mutex::new(0)), name, bank, meseta, @@ -118,11 +127,14 @@ impl BankState { self.bank.0.len() } - pub fn initialize_item_ids(&mut self, base_item_id: u32) { + pub async fn initialize_item_ids(&mut self, base_item_id: Arc>) { + self.item_id_counter = base_item_id; + let mut bitem_id = self.item_id_counter.lock().await; for (i, item) in self.bank.0.iter_mut().enumerate() { - item.item_id = ClientItemId(base_item_id + i as u32); + item.item_id = ClientItemId(*bitem_id + i as u32); } - self.item_id_counter = base_item_id + self.bank.0.len() as u32; + + *bitem_id = *bitem_id + self.bank.0.len() as u32; } pub fn add_meseta(&mut self, amount: u32) -> Result<(), anyhow::Error> { @@ -191,7 +203,7 @@ impl BankState { } } - pub fn take_item(&mut self, item_id: &ClientItemId, amount: u32) -> Option { + pub async fn take_item(&mut self, item_id: &ClientItemId, amount: u32) -> Option { let idx = self.bank.0 .iter() .position(|i| i.item_id == *item_id)?; @@ -211,9 +223,8 @@ impl BankState { } else { let entity_ids = stacked_item.entity_ids.drain(..(amount as usize)).collect(); - self.item_id_counter += 1; Some(BankItem { - item_id: ClientItemId(self.item_id_counter), + item_id: new_item_id(&self.item_id_counter).await, item: BankItemDetail::Stacked(StackedItemDetail { entity_ids, tool: stacked_item.tool, diff --git a/src/ship/items/inventory.rs b/src/ship/items/inventory.rs index d7e74fd..19fa372 100644 --- a/src/ship/items/inventory.rs +++ b/src/ship/items/inventory.rs @@ -3,6 +3,7 @@ use libpso::character::character; use crate::ship::items::ClientItemId; use crate::entity::item::{Meseta, ItemEntityId, ItemDetail, ItemEntity, InventoryEntity, InventoryItemEntity, EquippedEntity}; use std::future::Future; +use async_std::sync::{Arc, Mutex}; use crate::entity::character::CharacterEntityId; use crate::entity::item::tool::ToolType; @@ -117,22 +118,12 @@ impl InventoryItemDetail { } } -} - - -#[derive(Clone, Debug)] -pub struct InventoryItem { - pub item_id: ClientItemId, - pub item: InventoryItemDetail, -} - -impl InventoryItem { pub async fn with_entity_id(&self, mut param: T, mut func: F) -> Result where F: FnMut(T, ItemEntityId) -> Fut, Fut: Future>, { - match &self.item { + match &self { InventoryItemDetail::Individual(individual_item) => { param = func(param, individual_item.entity_id).await?; }, @@ -145,6 +136,23 @@ impl InventoryItem { Ok(param) } +} + + +#[derive(Clone, Debug)] +pub struct InventoryItem { + pub item_id: ClientItemId, + pub item: InventoryItemDetail, +} + +impl InventoryItem { + pub async fn with_entity_id(&self, param: T, func: F) -> Result + where + F: FnMut(T, ItemEntityId) -> Fut, + Fut: Future>, + { + self.item.with_entity_id(param, func).await + } pub async fn with_mag(&self, mut param: T, mut func: F) -> Result where @@ -183,23 +191,38 @@ pub enum InventoryError { #[derive(Clone, Debug)] pub struct InventoryState { pub character_id: CharacterEntityId, - pub item_id_counter: u32, + pub item_id_counter: Arc>, pub inventory: Inventory, pub equipped: EquippedEntity, pub meseta: Meseta, } +async fn new_item_id(item_id_counter: &Arc>) -> ClientItemId { + let mut item_id_counter = item_id_counter.lock().await; + let item_id = *item_id_counter; + *item_id_counter += 1; + + ClientItemId(item_id) +} + impl InventoryState { - pub fn initialize_item_ids(&mut self, base_item_id: u32) { + pub async fn initialize_item_ids(&mut self, base_item_id: Arc>) { + self.item_id_counter = base_item_id; + let mut bitem_id = self.item_id_counter.lock().await; + for (i, item) in self.inventory.0.iter_mut().enumerate() { - item.item_id = ClientItemId(base_item_id + i as u32); + item.item_id = ClientItemId(*bitem_id + i as u32); } - self.item_id_counter = base_item_id + self.inventory.0.len() as u32 + 1; + + *bitem_id = *bitem_id + self.inventory.0.len() as u32; } - pub fn new_item_id(&mut self) -> ClientItemId { - self.item_id_counter += 1; - ClientItemId(self.item_id_counter) + pub async fn new_item_id(&mut self) -> ClientItemId { + let mut item_id_counter = self.item_id_counter.lock().await; + let item_id = *item_id_counter; + *item_id_counter += 1; + + ClientItemId(item_id) } pub fn count(&self) -> usize { @@ -326,7 +349,36 @@ impl InventoryState { } } - pub fn take_item(&mut self, item_id: &ClientItemId, amount: u32) -> Option { + pub async fn remove_item(&mut self, item_id: &ClientItemId, amount: u32) -> Option { + let idx = self.inventory.0 + .iter() + .position(|i| i.item_id == *item_id)?; + match &mut self.inventory.0[idx].item { + InventoryItemDetail::Individual(_individual_item) => { + Some(self.inventory.0.remove(idx).item) + }, + InventoryItemDetail::Stacked(stacked_item) => { + let remove_all = (amount == 0) || match stacked_item.entity_ids.len().cmp(&(amount as usize)) { + Ordering::Equal => true, + Ordering::Greater => false, + Ordering::Less => return None, + }; + + if remove_all { + Some(self.inventory.0.remove(idx).item) + } + else { + let entity_ids = stacked_item.entity_ids.drain(..(amount as usize)).collect(); + Some(InventoryItemDetail::Stacked(StackedItemDetail { + entity_ids, + tool: stacked_item.tool, + })) + } + } + } + } + + pub async fn take_item(&mut self, item_id: &ClientItemId, amount: u32) -> Option { let idx = self.inventory.0 .iter() .position(|i| i.item_id == *item_id)?; @@ -346,9 +398,8 @@ impl InventoryState { } else { let entity_ids = stacked_item.entity_ids.drain(..(amount as usize)).collect(); - self.item_id_counter += 1; Some(InventoryItem { - item_id: ClientItemId(self.item_id_counter), + item_id: new_item_id(&self.item_id_counter).await, item: InventoryItemDetail::Stacked(StackedItemDetail { entity_ids, tool: stacked_item.tool, diff --git a/src/ship/items/state.rs b/src/ship/items/state.rs index 79f24eb..b22a25c 100644 --- a/src/ship/items/state.rs +++ b/src/ship/items/state.rs @@ -134,15 +134,40 @@ pub enum AddItemResult { Meseta, } +#[derive(Clone, Debug)] +struct RoomGemItemIdCounter { + inventory: [Arc>; 4], + bank: [Arc>; 4], +} + +impl Default for RoomGemItemIdCounter { + fn default() -> RoomGemItemIdCounter { + RoomGemItemIdCounter { + inventory: core::array::from_fn(|gem| Arc::new(Mutex::new(((gem as u32) << 21) | 0x10000))), + bank: core::array::from_fn(|gem| Arc::new(Mutex::new(((gem as u32) << 21) | 0x20000))), + } + } +} + +impl RoomGemItemIdCounter { + fn inventory(&self, area_client: &AreaClient) -> Arc> { + self.inventory[area_client.local_client.id() as usize].clone() + } + + fn bank(&self, area_client: &AreaClient) -> Arc> { + self.bank[area_client.local_client.id() as usize].clone() + } +} + #[derive(Clone, Debug)] pub struct ItemState { character_inventory: Arc>>>, character_bank: Arc>>>, - character_room: Arc>>, character_floor: Arc>>>, room_floor: Arc>>>, + room_gem_item_ids: Arc>>, room_item_id_counter: Arc>, } @@ -155,6 +180,7 @@ impl Default for ItemState { character_room: Arc::new(RwLock::new(HashMap::new())), character_floor: Arc::new(RwLock::new(HashMap::new())), room_floor: Arc::new(RwLock::new(HashMap::new())), + room_gem_item_ids: Arc::new(RwLock::new(HashMap::new())), room_item_id_counter: Arc::new(RwLock::new(0x00810000)), } } @@ -230,12 +256,12 @@ impl ItemState { let character_meseta = entity_gateway.get_character_meseta(&character.id).await?; let inventory_state = InventoryState { character_id: character.id, - item_id_counter: 0, + item_id_counter: Arc::new(Mutex::new(0)), inventory: Inventory::new(inventory_items), equipped, meseta: character_meseta, }; - + let bank_items = join_all( bank.items.into_iter() .map(|item| { @@ -271,7 +297,7 @@ impl ItemState { .await .into_iter() .collect::, anyhow::Error>>()?; - + let bank_meseta = entity_gateway.get_bank_meseta(&character.id, &BankName("".into())).await?; let bank_state = BankState::new(character.id, BankName("".into()), Bank::new(bank_items), bank_meseta); @@ -287,7 +313,13 @@ impl ItemState { } pub async fn add_character_to_room(&mut self, room_id: RoomId, character: &CharacterEntity, area_client: AreaClient) { - let base_inventory_id = ((area_client.local_client.id() as u32) << 21) | 0x10000; + let mut base_item_ids = self.room_gem_item_ids + .write() + .await; + let base_item_ids = base_item_ids + .entry(room_id) + .or_insert_with(RoomGemItemIdCounter::default); + self.character_inventory .read() .await @@ -295,8 +327,8 @@ impl ItemState { .unwrap() .write() .await - .initialize_item_ids(base_inventory_id); - let base_bank_id = ((area_client.local_client.id() as u32) << 21) | 0x20000; + .initialize_item_ids(base_item_ids.inventory(&area_client).clone()) + .await; self.character_bank .read() .await @@ -304,7 +336,8 @@ impl ItemState { .unwrap() .write() .await - .initialize_item_ids(base_bank_id); + .initialize_item_ids(base_item_ids.bank(&area_client)) + .await; self.character_room .write() .await @@ -486,16 +519,22 @@ impl ItemStateProxy { } pub async fn floor(&mut self, character_id: &CharacterEntityId) -> Result { - let room_id = *self.item_state.character_room.read().await.get(character_id).unwrap(); + let room_id = *self.item_state.character_room.read().await.get(character_id) + .ok_or_else(|| anyhow::Error::from(ItemStateError::NoCharacter(*character_id))) + .with_context(|| format!("character {character_id}\nrooms: {:#?}", self.item_state.character_room))?; Ok(FloorState { character_id: *character_id, - local: get_or_clone(&self.item_state.character_floor, &self.proxied_state.character_floor, *character_id, ItemStateError::NoCharacter).await?, - shared: get_or_clone(&self.item_state.room_floor, &self.proxied_state.room_floor, room_id, ItemStateError::NoRoom).await?, + local: get_or_clone(&self.item_state.character_floor, &self.proxied_state.character_floor, *character_id, ItemStateError::NoCharacter).await + .with_context(|| format!("no local_floor state: {character_id:?} {:#?}\nproxy: {:#?}", self.item_state.character_floor, self.proxied_state.character_floor))?, + shared: get_or_clone(&self.item_state.room_floor, &self.proxied_state.room_floor, room_id, ItemStateError::NoRoom).await + .with_context(|| format!("no share_floor state: {character_id:?} {:#?}\nproxy: {:#?}", self.item_state.room_floor, self.proxied_state.room_floor))?, }) } pub async fn set_floor(&mut self, floor: FloorState) { - let room_id = *self.item_state.character_room.read().await.get(&floor.character_id).unwrap(); + let room_id = *self.item_state.character_room.read().await.get(&floor.character_id) + .ok_or_else(|| anyhow::Error::from(ItemStateError::NoCharacter(floor.character_id))) + .with_context(|| format!("character {}\nrooms: {:#?}", floor.character_id, self.item_state.character_room)).unwrap(); self.proxied_state.character_floor.lock().await.insert(floor.character_id, floor.local); self.proxied_state.room_floor.lock().await.insert(room_id, floor.shared); } diff --git a/src/ship/items/tasks.rs b/src/ship/items/tasks.rs index ad3e585..c0d71ed 100644 --- a/src/ship/items/tasks.rs +++ b/src/ship/items/tasks.rs @@ -281,7 +281,7 @@ where entity_gateway.with_transaction(|transaction| async move { let item_state_proxy = ItemStateProxy::new(item_state.clone()); let ((item_state_proxy, transaction), (pkts, new_character)) = ItemStateAction::default() - .act(actions::take_item_from_inventory(character.id, *item_id, amount)) + .act(actions::remove_item_from_inventory(character.id, *item_id, amount)) .act(actions::use_consumed_item(character)) .act(actions::fork( actions::foreach(actions::apply_item_action_packets(character.id, area_client)), diff --git a/src/ship/ship.rs b/src/ship/ship.rs index 1d1bb47..6f53fdf 100644 --- a/src/ship/ship.rs +++ b/src/ship/ship.rs @@ -489,7 +489,6 @@ impl Blocks { .get_mut(block) .ok_or_else(|| ShipError::InvalidBlock(block).into()) } - } #[derive(Clone)] diff --git a/tests/test_bank.rs b/tests/test_bank.rs index e809371..2034070 100644 --- a/tests/test_bank.rs +++ b/tests/test_bank.rs @@ -1072,7 +1072,7 @@ async fn test_withdraw_partial_stacked_item() { assert!(packets.len() == 2); assert!(matches!(&packets[1], (ClientId(2), SendShipPacket::Message(Message {msg: GameMessage::CreateItem(create_item)})) - if create_item.item_id == 0x20002 + if create_item.item_id == 0x20001 )); let bank_items = entity_gateway.get_character_bank(&char1.id, &item::BankName("".into())).await.unwrap(); diff --git a/tests/test_item_pickup.rs b/tests/test_item_pickup.rs index b216c13..820e167 100644 --- a/tests/test_item_pickup.rs +++ b/tests/test_item_pickup.rs @@ -734,7 +734,7 @@ async fn test_player_drops_partial_stack_and_other_player_picks_it_up() { ship.handle(ClientId(2), RecvShipPacket::DirectMessage(DirectMessage::new(0, GameMessage::PickupItem(PickupItem { client: 0, target: 0, - item_id: 0x10003, + item_id: 0x10001, map_area: 0, unknown: [0; 3] })))).await.unwrap(); diff --git a/tests/test_item_use.rs b/tests/test_item_use.rs index 6474e6f..b6b8502 100644 --- a/tests/test_item_use.rs +++ b/tests/test_item_use.rs @@ -445,6 +445,7 @@ async fn test_use_monogrinder() { + // TODO: tests for ALL ITEMS WOW /* diff --git a/tests/test_rooms.rs b/tests/test_rooms.rs index 05a3852..00b512d 100644 --- a/tests/test_rooms.rs +++ b/tests/test_rooms.rs @@ -12,112 +12,6 @@ mod common; use common::*; -#[async_std::test] -async fn test_item_ids_reset_when_rejoining_rooms() { - let mut entity_gateway = InMemoryGateway::default(); - - let (_user1, char1) = new_user_character(&mut entity_gateway, "a1", "a", 1).await; - let (_user2, char2) = new_user_character(&mut entity_gateway, "a2", "a", 1).await; - - let mut p1_inv = Vec::new(); - for _ in 0..3usize { - p1_inv.push(entity_gateway.create_item( - item::NewItemEntity { - item: item::ItemDetail::Weapon( - item::weapon::Weapon { - weapon: item::weapon::WeaponType::Saber, - grind: 0, - special: None, - attrs: [None, None, None], - tekked: true, - } - ), - }).await.unwrap()); - } - - let mut p2_inv = Vec::new(); - for _ in 0..10usize { - p2_inv.push(entity_gateway.create_item( - item::NewItemEntity { - item: item::ItemDetail::Weapon( - item::weapon::Weapon { - weapon: item::weapon::WeaponType::Saber, - grind: 0, - special: None, - attrs: [None, None, None], - tekked: true, - } - ), - }).await.unwrap()); - } - - entity_gateway.set_character_inventory(&char1.id, &item::InventoryEntity::new(p1_inv)).await.unwrap(); - entity_gateway.set_character_inventory(&char2.id, &item::InventoryEntity::new(p2_inv)).await.unwrap(); - - let mut ship = Box::new(ShipServerState::builder() - .gateway(entity_gateway.clone()) - .build()); - log_in_char(&mut ship, ClientId(1), "a1", "a").await; - log_in_char(&mut ship, ClientId(2), "a2", "a").await; - - join_lobby(&mut ship, ClientId(1)).await; - join_lobby(&mut ship, ClientId(2)).await; - - create_room(&mut ship, ClientId(1), "room", "").await; - let p = ship.handle(ClientId(2), RecvShipPacket::MenuSelect(MenuSelect { - menu: ROOM_MENU_ID, - item: 0, - })).await.unwrap(); - ship.handle(ClientId(2), RecvShipPacket::DoneBursting(DoneBursting {})).await.unwrap(); - - match &p[1].1 { - SendShipPacket::AddToRoom(add_to) => { - assert_eq!(add_to.playerinfo.inventory.items.iter().map(|k| k.item_id).collect::>(), - vec![0x210000,0x210001,0x210002,0x210003,0x210004,0x210005,0x210006,0x210007,0x210008,0x210009, - 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]); - }, - _ => panic!(), - } - - leave_room(&mut ship, ClientId(2)).await; - - let p = ship.handle(ClientId(2), RecvShipPacket::MenuSelect(MenuSelect { - menu: ROOM_MENU_ID, - item: 0, - })).await.unwrap(); - - match &p[1].1 { - SendShipPacket::AddToRoom(add_to) => { - assert_eq!(add_to.playerinfo.inventory.items.iter().map(|k| k.item_id).collect::>(), - vec![0x210000,0x210001,0x210002,0x210003,0x210004,0x210005,0x210006,0x210007,0x210008,0x210009, - 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]); - }, - _ => panic!(), - } -} - -/* -#[async_std::test] -async fn test_load_rare_monster_default_appear_rates() { - let mut entity_gateway = InMemoryGateway::default(); - let (_user1, _char1) = new_user_character(&mut entity_gateway, "a1", "a", 1).await; - let mut ship = Box::new(ShipServerState::builder() - .gateway(entity_gateway.clone()) - .build()); - log_in_char(&mut ship, ClientId(1), "a1", "a").await; - join_lobby(&mut ship, ClientId(1)).await; - create_room(&mut ship, ClientId(1), "room", "").await; - - // assume episode 1 - ship.blocks.0[0].rooms.with(RoomId(0), |room| Box::pin(async move { - let rates = &*room.rare_monster_table; - for (_monster, rate) in rates.clone().appear_rate { - assert_eq!(rate, 0.001953125f32); // 1/512 = 0.001953125 - } - })).await.unwrap(); -} -*/ - #[async_std::test] async fn test_set_valid_quest_group() { let mut entity_gateway = InMemoryGateway::default(); @@ -226,3 +120,6 @@ async fn test_cannot_join_room_after_its_closed() { msg: _expectedmsg, // wow yes cool rust is so great literally the best i can't put a String::from() directly in here. })))); } + + +// TODO: test joining twice errors not hangs forever