From cbfd39803501c65724b2fdd104a0c75bf64aa3a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcel=20M=C3=A4rtens?= Date: Sat, 4 Jul 2020 11:52:21 +0200 Subject: [PATCH 1/3] remove Mutex in server as Stream is now 'Sync' --- network/examples/chat/Cargo.lock | 1 + network/examples/fileshare/Cargo.lock | 1 + network/examples/network-speed/Cargo.lock | 1 + network/src/api.rs | 4 ---- server/src/client.rs | 10 ++-------- server/src/lib.rs | 2 +- server/src/sys/message.rs | 2 +- 7 files changed, 7 insertions(+), 14 deletions(-) diff --git a/network/examples/chat/Cargo.lock b/network/examples/chat/Cargo.lock index 8839dcce09..9c26fd8f98 100644 --- a/network/examples/chat/Cargo.lock +++ b/network/examples/chat/Cargo.lock @@ -795,6 +795,7 @@ version = "0.1.0" dependencies = [ "async-std", "bincode", + "crossbeam-channel", "futures", "lazy_static", "prometheus", diff --git a/network/examples/fileshare/Cargo.lock b/network/examples/fileshare/Cargo.lock index de5da54e7e..0144ec3ac0 100644 --- a/network/examples/fileshare/Cargo.lock +++ b/network/examples/fileshare/Cargo.lock @@ -886,6 +886,7 @@ version = "0.1.0" dependencies = [ "async-std", "bincode", + "crossbeam-channel", "futures", "lazy_static", "prometheus", diff --git a/network/examples/network-speed/Cargo.lock b/network/examples/network-speed/Cargo.lock index 58b125e281..7cd1b8e982 100644 --- a/network/examples/network-speed/Cargo.lock +++ b/network/examples/network-speed/Cargo.lock @@ -877,6 +877,7 @@ version = "0.1.0" dependencies = [ "async-std", "bincode", + "crossbeam-channel", "futures", "lazy_static", "prometheus", diff --git a/network/src/api.rs b/network/src/api.rs index 47cee1f713..80bf3f1588 100644 --- a/network/src/api.rs +++ b/network/src/api.rs @@ -60,14 +60,10 @@ pub struct Participant { /// /// Unlike [`Network`] and [`Participant`], `Streams` don't implement interior /// mutability, as multiple threads don't need access to the same `Stream`. -/// [`Sync`] is not supported! In that case multiple `Streams` should be used -/// instead. However it's still possible to [`Send`] `Streams`. /// /// [`Networks`]: crate::api::Network /// [`open`]: Participant::open /// [`opened`]: Participant::opened -/// [`Send`]: std::marker::Send -/// [`Sync`]: std::marker::Sync #[derive(Debug)] pub struct Stream { pid: Pid, diff --git a/server/src/client.rs b/server/src/client.rs index 43803a42ae..6de7d502b3 100644 --- a/server/src/client.rs +++ b/server/src/client.rs @@ -7,7 +7,7 @@ use vek::*; pub struct Client { pub client_state: ClientState, - pub singleton_stream: std::sync::Mutex, + pub singleton_stream: Stream, pub last_ping: f64, pub login_msg_sent: bool, } @@ -17,9 +17,7 @@ impl Component for Client { } impl Client { - pub fn notify(&mut self, msg: ServerMsg) { - let _ = self.singleton_stream.lock().unwrap().send(msg); - } + pub fn notify(&mut self, msg: ServerMsg) { let _ = self.singleton_stream.send(msg); } pub fn is_registered(&self) -> bool { match self.client_state { @@ -39,16 +37,12 @@ impl Client { self.client_state = new_state; let _ = self .singleton_stream - .lock() - .unwrap() .send(ServerMsg::StateAnswer(Ok(new_state))); } pub fn error_state(&mut self, error: RequestStateError) { let _ = self .singleton_stream - .lock() - .unwrap() .send(ServerMsg::StateAnswer(Err((error, self.client_state)))); } } diff --git a/server/src/lib.rs b/server/src/lib.rs index 753d2b1fb4..1817330397 100644 --- a/server/src/lib.rs +++ b/server/src/lib.rs @@ -601,7 +601,7 @@ impl Server { let mut client = Client { client_state: ClientState::Connected, - singleton_stream: std::sync::Mutex::new(singleton_stream), + singleton_stream, last_ping: self.state.get_time(), login_msg_sent: false, }; diff --git a/server/src/sys/message.rs b/server/src/sys/message.rs index b13fe3753e..1cc98d53ad 100644 --- a/server/src/sys/message.rs +++ b/server/src/sys/message.rs @@ -57,7 +57,7 @@ impl Sys { settings: &Read<'_, ServerSettings>, ) -> Result<(), crate::error::Error> { loop { - let msg = client.singleton_stream.lock().unwrap().recv().await?; + let msg = client.singleton_stream.recv().await?; *cnt += 1; match msg { // Go back to registered state (char selection screen) From e7195b57adca5a117f8712622fdf52fe81481e19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcel=20M=C3=A4rtens?= Date: Sat, 4 Jul 2020 12:17:33 +0200 Subject: [PATCH 2/3] extend network with better Error codes for Network --- network/src/api.rs | 43 +++++++++++++++++++++------- network/src/protocols.rs | 10 +++++-- voxygen/src/menu/main/client_init.rs | 5 ++-- 3 files changed, 43 insertions(+), 15 deletions(-) diff --git a/network/src/api.rs b/network/src/api.rs index 80bf3f1588..110bae0fa6 100644 --- a/network/src/api.rs +++ b/network/src/api.rs @@ -82,6 +82,8 @@ pub struct Stream { pub enum NetworkError { NetworkClosed, ListenFailed(std::io::Error), + ConnectFailed(std::io::Error), + GracefulDisconnectFailed(std::io::Error), } /// Error type thrown by [`Participants`](Participant) methods @@ -313,7 +315,10 @@ impl Network { .await .send((address, pid_sender)) .await?; - let participant = pid_receiver.await??; + let participant = match pid_receiver.await? { + Ok(p) => p, + Err(e) => return Err(NetworkError::ConnectFailed(e)), + }; let pid = participant.remote_pid; debug!( ?pid, @@ -433,6 +438,7 @@ impl Network { this is a bad idea. Participant will only be dropped when you drop your last \ reference" ); + Ok(()) }, Ok(mut participant) => { trace!("waiting now for participant to close"); @@ -447,13 +453,30 @@ impl Network { .send((pid, finished_sender)) .await .expect("something is wrong in internal scheduler coding"); - let res = finished_receiver.await.unwrap(); - trace!("participant is now closed"); - res?; + match finished_receiver.await { + Ok(Ok(())) => { + trace!(?pid, "Participant is now closed"); + Ok(()) + }, + Ok(Err(e)) => { + trace!( + ?e, + "Error occured during shutdown of participant and is propagated to \ + User" + ); + Err(NetworkError::GracefulDisconnectFailed(e)) + }, + Err(e) => { + error!( + ?pid, + ?e, + "Failed to get a message back from the scheduler, closing the network" + ); + Err(NetworkError::NetworkClosed) + }, + } }, - }; - - Ok(()) + } } /// returns a copy of all current connected [`Participants`], @@ -942,10 +965,6 @@ impl From> for NetworkError { fn from(_err: crossbeam_channel::SendError) -> Self { NetworkError::NetworkClosed } } -impl From for NetworkError { - fn from(err: async_std::io::Error) -> Self { NetworkError::ListenFailed(err) } -} - impl From for StreamError { fn from(_err: std::option::NoneError) -> Self { StreamError::StreamClosed } } @@ -1002,6 +1021,8 @@ impl core::fmt::Display for NetworkError { match self { NetworkError::NetworkClosed => write!(f, "network closed"), NetworkError::ListenFailed(_) => write!(f, "listening failed"), + NetworkError::ConnectFailed(_) => write!(f, "connecting failed"), + NetworkError::GracefulDisconnectFailed(_) => write!(f, "graceful disconnect failed"), } } } diff --git a/network/src/protocols.rs b/network/src/protocols.rs index 1fe32dd947..3f20c78cfa 100644 --- a/network/src/protocols.rs +++ b/network/src/protocols.rs @@ -71,7 +71,10 @@ impl TcpProtocol { "closing tcp protocol due to read error, sending close frame to gracefully \ shutdown" ); - w2c_cid_frame_s.send((cid, Frame::Shutdown)).await.unwrap(); + w2c_cid_frame_s + .send((cid, Frame::Shutdown)) + .await + .expect("Channel or Participant seems no longer to exist to be Shutdown"); } } @@ -201,7 +204,10 @@ impl TcpProtocol { }, }; metrics_cache.with_label_values(&frame).inc(); - w2c_cid_frame_s.send((cid, frame)).await.unwrap(); + w2c_cid_frame_s + .send((cid, frame)) + .await + .expect("Channel or Participant seems no longer to exist"); } trace!("shutting down tcp read()"); } diff --git a/voxygen/src/menu/main/client_init.rs b/voxygen/src/menu/main/client_init.rs index 0bc8fc29f3..86fe5dcde6 100644 --- a/voxygen/src/menu/main/client_init.rs +++ b/voxygen/src/menu/main/client_init.rs @@ -100,8 +100,9 @@ impl ClientInit { }, Err(err) => { match err { - ClientError::NetworkErr(NetworkError::ListenFailed(..)) => { - }, + ClientError::NetworkErr(NetworkError::ConnectFailed( + .., + )) => {}, // Non-connection error, stop attempts err => { last_err = Some(Error::ClientError(err)); From fe47b113454cc2fa521bb745c7c045b32b42ef1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcel=20M=C3=A4rtens?= Date: Sat, 4 Jul 2020 12:30:37 +0200 Subject: [PATCH 3/3] instead of trying to connect for 80 minutes, just try 4 mins, added Changelog --- CHANGELOG.md | 2 ++ voxygen/src/menu/main/client_init.rs | 12 +++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 621684c177..e154859904 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fixed unable to use ability; Secondary and ability3 (fire rod) will now automatically wield - Gliding is now a toggle that can be triggered from the ground - Replaced `log` with `tracing` in all crates +- Switch to a new network backend that will allow several improvements in the future +- Connection screen fails after 4 minutes if it can't connect to the server instead of 80 minutes ### Removed diff --git a/voxygen/src/menu/main/client_init.rs b/voxygen/src/menu/main/client_init.rs index 86fe5dcde6..c11d848c83 100644 --- a/voxygen/src/menu/main/client_init.rs +++ b/voxygen/src/menu/main/client_init.rs @@ -12,6 +12,7 @@ use std::{ thread, time::Duration, }; +use tracing::debug; #[derive(Debug)] pub enum Error { @@ -70,8 +71,8 @@ impl ClientInit { let mut last_err = None; - 'tries: for _ in 0..960 + 1 { - // 300 Seconds + const FOUR_MINUTES_RETRIES: u64 = 48; + 'tries: for _ in 0..FOUR_MINUTES_RETRIES { if cancel2.load(Ordering::Relaxed) { break; } @@ -102,7 +103,12 @@ impl ClientInit { match err { ClientError::NetworkErr(NetworkError::ConnectFailed( .., - )) => {}, + )) => { + debug!( + "can't reach the server, going to retry in a few \ + seconds" + ); + }, // Non-connection error, stop attempts err => { last_err = Some(Error::ClientError(err));