Fix: Move WS error handling (#174)

Moves all WS handling of unexpected payloads into the stream to prevent code duplication.

This also prevents non-{Hello,Resumed,Ready} messages from causing a handshake failure, as it seems Discord do not prevent such messages from appearing.

---------

Co-authored-by: Kyle Simpson <[email protected]>

src/driver/connection/error.rs

@@ -27,8 +27,6 @@ pub enum Error {
     CryptoModeUnavailable,
     /// An indicator that an endpoint URL was invalid.
     EndpointUrl,
-    /// Discord hello/ready handshake was violated.
-    ExpectedHandshake,
     /// Discord failed to correctly respond to IP discovery.
     IllegalDiscoveryResponse,
     /// Could not parse Discord's view of our IP.
@@ -103,7 +101,6 @@ impl fmt::Display for Error {
             Self::CryptoModeInvalid => write!(f, "server changed negotiated encryption mode"),
             Self::CryptoModeUnavailable => write!(f, "server did not offer chosen encryption mode"),
             Self::EndpointUrl => write!(f, "endpoint URL received from gateway was invalid"),
-            Self::ExpectedHandshake => write!(f, "voice initialisation protocol was violated"),
             Self::IllegalDiscoveryResponse =>
                 write!(f, "IP discovery/NAT punching response was invalid"),
             Self::IllegalIp => write!(f, "IP discovery/NAT punching response had bad IP value"),
@@ -124,7 +121,6 @@ impl StdError for Error {
             | Error::CryptoModeInvalid
             | Error::CryptoModeUnavailable
             | Error::EndpointUrl
-            | Error::ExpectedHandshake
             | Error::IllegalDiscoveryResponse
             | Error::IllegalIp
             | Error::InterconnectFailure(_)

src/driver/connection/mod.rs

@@ -95,8 +95,6 @@ impl Connection {
                 },
                 other => {
                     debug!("Expected ready/hello; got: {:?}", other);
-
-                    return Err(Error::ExpectedHandshake);
                 },
             }
         }
@@ -304,8 +302,6 @@ impl Connection {
                 },
                 other => {
                     debug!("Expected resumed/hello; got: {:?}", other);
-
-                    return Err(Error::ExpectedHandshake);
                 },
             }
         }

src/driver/mod.rs

@@ -30,8 +30,6 @@ pub use mix_mode::MixMode;
 #[cfg(test)]
 pub use test_config::*;
 
-#[cfg(feature = "builtin-queue")]
-use crate::tracks;
 #[cfg(feature = "builtin-queue")]
 use crate::tracks::TrackQueue;
 use crate::{

src/driver/tasks/ws.rs

@@ -95,10 +95,6 @@ impl AuxNetwork {
                 }
                 ws_msg = self.ws_client.recv_json_no_timeout(), if !self.dont_send => {
                     ws_error = match ws_msg {
-                        Err(WsError::Json(e)) => {
-                            debug!("Unexpected JSON {:?}.", e);
-                            false
-                        },
                         Err(e) => {
                             should_reconnect = ws_error_is_not_final(&e);
                             ws_reason = Some((&e).into());

src/events/context/data/disconnect.rs

@@ -90,7 +90,6 @@ impl From<&ConnectionError> for DisconnectReason {
             | ConnectionError::CryptoModeInvalid
             | ConnectionError::CryptoModeUnavailable
             | ConnectionError::EndpointUrl
-            | ConnectionError::ExpectedHandshake
             | ConnectionError::IllegalDiscoveryResponse
             | ConnectionError::IllegalIp
             | ConnectionError::Json(_) => Self::ProtocolViolation,

src/ws.rs

@@ -94,19 +94,16 @@ pub(crate) fn convert_ws_message(message: Option<Message>) -> Result<Option<Even
     Ok(match message {
         // SAFETY:
         // simd-json::serde::from_str may leave an &mut str in a non-UTF state on failure.
-        // The below is safe as we have taken ownership of the inner `String`, and don't
-        // access it as a `str`/`String` or return it if failure occurs.
+        // The below is safe as we have taken ownership of the inner `String`, and if
+        // failure occurs we forcibly re-validate its contents before logging.
         Some(Message::Text(mut payload)) =>
-            match unsafe { crate::json::from_str(payload.as_mut_str()) } {
-                Ok(event) => Some(event),
-                Err(why) => {
-                    debug!(
-                        "Could not deserialize websocket event, payload: {}, error: {}",
-                        payload, why
-                    );
-                    None
-                },
-            },
+            (unsafe { crate::json::from_str(payload.as_mut_str()) })
+                .map_err(|e| {
+                    let safe_payload = String::from_utf8_lossy(payload.as_bytes());
+                    debug!("Unexpected JSON: {e}. Payload: {safe_payload}");
+                    e
+                })
+                .ok(),
         Some(Message::Binary(bytes)) => {
             return Err(Error::UnexpectedBinaryMessage(bytes));
         },