From 293112777a25cb290efa9a3e185164b56cf389dd Mon Sep 17 00:00:00 2001 From: Joscha Date: Fri, 17 Feb 2023 17:29:12 +0100 Subject: [PATCH] Fix bugged room state from lingering connection When disconnecting from a room whose instance is "waiting" and then reconnecting, the old instance would not be stopped immediately. Instead, it would continue to run until it managed to reconnect, sending status updates to the main event bus in the process. These events led to the euph::Room entering a state where it was connected but no last_msg_id was set. This meant that no new messages could be entered into the vault, including messages sent by the user. The result was UI weirdness when sending a message. As a fix, euphoxide instances are now identified via an u32 id. This id is unique across all rooms. Packets by unknown ids are rejected and have no effect on room states. --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/euph/room.rs | 22 +++++++++------- src/ui/euph/room.rs | 44 +++++++++++++++++++++----------- src/ui/rooms.rs | 61 ++++++++++++++++++++++++++++++++------------- 5 files changed, 87 insertions(+), 44 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 20b9f53..42a6604 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -320,7 +320,7 @@ dependencies = [ [[package]] name = "euphoxide" version = "0.3.0" -source = "git+https://github.com/Garmelon/euphoxide.git?tag=v0.3.0#c5be90cd60ff1fac78c2063c812d88dd36858481" +source = "git+https://github.com/Garmelon/euphoxide.git?rev=069e4e02c77e5bc649cd770af8183d8be8cd52f5#069e4e02c77e5bc649cd770af8183d8be8cd52f5" dependencies = [ "async-trait", "caseless", diff --git a/Cargo.toml b/Cargo.toml index 4fd5a56..bc38f83 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -35,7 +35,7 @@ features = ["rustls-tls-native-roots"] [dependencies.euphoxide] git = "https://github.com/Garmelon/euphoxide.git" -tag = "v0.3.0" +rev = "069e4e02c77e5bc649cd770af8183d8be8cd52f5" features = ["bot"] # [patch."https://github.com/Garmelon/euphoxide.git"] diff --git a/src/euph/room.rs b/src/euph/room.rs index fa577d0..e38c2ed 100644 --- a/src/euph/room.rs +++ b/src/euph/room.rs @@ -85,6 +85,10 @@ impl Room { self.instance.stopped() } + pub fn instance(&self) -> &Instance { + &self.instance + } + pub fn state(&self) -> &State { &self.state } @@ -108,7 +112,7 @@ impl Room { self.log_request_canary = Some(tx); let vault_clone = self.vault.clone(); let conn_tx_clone = conn_tx.clone(); - debug!("{}: spawning log request task", self.instance.config().name); + debug!("{}: spawning log request task", self.instance.config().room); tokio::task::spawn(async move { select! { _ = rx => {}, @@ -204,34 +208,34 @@ impl Room { } async fn on_packet(&mut self, packet: ParsedPacket) { - let instance_name = &self.instance.config().name; + let room_name = &self.instance.config().room; let data = ok_or_return!(&packet.content); match data { Data::BounceEvent(_) => {} Data::DisconnectEvent(_) => {} Data::HelloEvent(_) => {} Data::JoinEvent(d) => { - debug!("{instance_name}: {:?} joined", d.0.name); + debug!("{room_name}: {:?} joined", d.0.name); } Data::LoginEvent(_) => {} Data::LogoutEvent(_) => {} Data::NetworkEvent(d) => { - warn!("{instance_name}: network event ({})", d.r#type); + warn!("{room_name}: network event ({})", d.r#type); } Data::NickEvent(d) => { - debug!("{instance_name}: {:?} renamed to {:?}", d.from, d.to); + debug!("{room_name}: {:?} renamed to {:?}", d.from, d.to); } Data::EditMessageEvent(_) => { - info!("{instance_name}: a message was edited"); + info!("{room_name}: a message was edited"); } Data::PartEvent(d) => { - debug!("{instance_name}: {:?} left", d.0.name); + debug!("{room_name}: {:?} left", d.0.name); } Data::PingEvent(_) => {} Data::PmInitiateEvent(d) => { // TODO Show info popup and automatically join PM room info!( - "{instance_name}: {:?} initiated a pm from &{}", + "{room_name}: {:?} initiated a pm from &{}", d.from_nick, d.from_room ); } @@ -247,7 +251,7 @@ impl Room { } } Data::SnapshotEvent(d) => { - info!("{instance_name}: successfully joined"); + info!("{room_name}: successfully joined"); logging_unwrap!(self.vault.join(Time::now()).await); self.last_msg_id = Some(d.log.last().map(|m| m.id)); logging_unwrap!( diff --git a/src/ui/euph/room.rs b/src/ui/euph/room.rs index 59a113a..0d48d57 100644 --- a/src/ui/euph/room.rs +++ b/src/ui/euph/room.rs @@ -95,15 +95,16 @@ impl EuphRoom { self.vault().room() } - pub fn connect(&mut self) { + pub fn connect(&mut self, next_instance_id: &mut u32) { if self.room.is_none() { let instance_config = self .server_config .clone() - .room(self.vault().room().to_string()) + .room(*next_instance_id, self.vault().room().to_string()) .username(self.config.username.clone()) .force_username(self.config.force_username) .password(self.config.password.clone()); + *next_instance_id = next_instance_id.wrapping_add(1); let tx = self.ui_event_tx.clone(); self.room = Some(euph::Room::new( @@ -674,23 +675,36 @@ impl EuphRoom { } pub async fn handle_event(&mut self, event: Event) -> bool { - let handled = if self.room.is_some() { - if let Event::Packet(_, packet, _) = &event { - match &packet.content { - Ok(data) => self.handle_euph_data(data), - Err(reason) => self.handle_euph_error(packet.r#type, reason), - } - } else { - true - } - } else { - false + let room = match &self.room { + None => return false, + Some(room) => room, }; - if let Some(room) = &mut self.room { - room.handle_event(event).await; + if event.config().id != room.instance().config().id { + // If we allowed ids other than the current one, old instances that + // haven't yet shut down properly could mess up our state. + return false; } + // We handle the packet internally first because the room event handling + // will consume it while we only need a reference. + let handled = if let Event::Packet(_, packet, _) = &event { + match &packet.content { + Ok(data) => self.handle_euph_data(data), + Err(reason) => self.handle_euph_error(packet.r#type, reason), + } + } else { + // The room state changes, which always means a redraw. + true + }; + + self.room + .as_mut() + // See check at the beginning of the function. + .expect("no room even though we checked earlier") + .handle_event(event) + .await; + handled } diff --git a/src/ui/rooms.rs b/src/ui/rooms.rs index 56dac95..2718eb0 100644 --- a/src/ui/rooms.rs +++ b/src/ui/rooms.rs @@ -61,6 +61,7 @@ pub struct Rooms { order: Order, euph_server_config: ServerConfig, + euph_next_instance_id: u32, euph_rooms: HashMap, } @@ -81,13 +82,14 @@ impl Rooms { list: ListState::new(), order: Order::from_rooms_sort_order(config.rooms_sort_order), euph_server_config, + euph_next_instance_id: 0, euph_rooms: HashMap::new(), }; if !config.offline { for (name, config) in &config.euph.rooms { if config.autojoin { - result.get_or_insert_room(name.clone()).connect(); + result.connect_to_room(name.clone()); } } } @@ -106,6 +108,36 @@ impl Rooms { }) } + fn connect_to_room(&mut self, name: String) { + let room = self.euph_rooms.entry(name.clone()).or_insert_with(|| { + EuphRoom::new( + self.euph_server_config.clone(), + self.config.euph_room(&name), + self.vault.euph().room(name), + self.ui_event_tx.clone(), + ) + }); + room.connect(&mut self.euph_next_instance_id); + } + + fn connect_to_all_rooms(&mut self) { + for room in self.euph_rooms.values_mut() { + room.connect(&mut self.euph_next_instance_id); + } + } + + fn disconnect_from_room(&mut self, name: &str) { + if let Some(room) = self.euph_rooms.get_mut(name) { + room.disconnect(); + } + } + + fn disconnect_from_all_rooms(&mut self) { + for room in self.euph_rooms.values_mut() { + room.disconnect(); + } + } + /// Remove rooms that are not running any more and can't be found in the db. /// Insert rooms that are in the db but not yet in in the hash map. /// @@ -370,36 +402,28 @@ impl Rooms { } key!('c') => { if let Some(name) = self.list.cursor() { - if let Some(room) = self.euph_rooms.get_mut(&name) { - room.connect(); - } + self.connect_to_room(name); } return true; } key!('C') => { - for room in self.euph_rooms.values_mut() { - room.connect(); - } + self.connect_to_all_rooms(); return true; } key!('d') => { if let Some(name) = self.list.cursor() { - if let Some(room) = self.euph_rooms.get_mut(&name) { - room.disconnect(); - } + self.disconnect_from_room(&name); } return true; } key!('D') => { - for room in self.euph_rooms.values_mut() { - room.disconnect(); - } + self.disconnect_from_all_rooms(); return true; } key!('a') => { for (name, options) in &self.config.euph.rooms { if options.autojoin { - self.get_or_insert_room(name.clone()).connect(); + self.connect_to_room(name.clone()); } } return true; @@ -511,7 +535,7 @@ impl Rooms { key!(Enter) => { let name = ed.text(); if !name.is_empty() { - self.get_or_insert_room(name.clone()).connect(); + self.connect_to_room(name.clone()); self.state = State::ShowRoom(name); } return true; @@ -545,12 +569,13 @@ impl Rooms { } pub async fn handle_euph_event(&mut self, event: Event) -> bool { - let instance_name = event.config().name.clone(); - let room = self.get_or_insert_room(instance_name.clone()); + let room_name = event.config().room.clone(); + let Some(room) = self.euph_rooms.get_mut(&room_name) else { return false; }; + let handled = room.handle_event(event).await; let room_visible = match &self.state { - State::ShowRoom(name) => *name == instance_name, + State::ShowRoom(name) => *name == room_name, _ => true, }; handled && room_visible