Merge pull request #87 from caspark/large-packets-only-crash-in-debug…

…-mode

Only panic when sending large packets if in debug mode

CHANGELOG.md

@@ -7,6 +7,8 @@ In this document, all remarkable changes are listed. Not mentioned are smaller c
 - allow non-`Clone` types to be stored in `GameStateCell`.
 - added `SyncTestSession::current_frame()` and `SpectatorSession::current_frame()` to match the existing `P2PSession::current_frame()`.
 - added `P2PSession::desync_detection()` to read the session's desync detection mode.
+- ggrs no longer panics when trying to send an overly large UDP packet, unless debug assertions are on.
+- fixed: ggrs would panic when trying to send a message over a custom socket implementation if that message exceeded the maximum safe UDP packet size, even though the underlying socket might have totally different applicable thresholds for what messages can be safely delivered.
 
 ## 0.10.2
 

src/network/protocol.rs

@@ -25,7 +25,6 @@ const SYNC_RETRY_INTERVAL: Duration = Duration::from_millis(200);
 const RUNNING_RETRY_INTERVAL: Duration = Duration::from_millis(200);
 const KEEP_ALIVE_INTERVAL: Duration = Duration::from_millis(200);
 const QUALITY_REPORT_INTERVAL: Duration = Duration::from_millis(200);
-const MAX_PAYLOAD: usize = 467; // 512 is max safe UDP payload, minus 45 bytes for the rest of the packet
 /// Number of old checksums to keep in memory
 pub const MAX_CHECKSUM_HISTORY_SIZE: usize = 32;
 
@@ -485,9 +484,6 @@ impl<T: Config> UdpProtocol<T> {
                 self.pending_output.iter().map(|gi| &gi.bytes),
             );
 
-            // the byte buffer should not exceed a certain size to guarantee a maximum UDP packet size
-            assert!(body.bytes.len() <= MAX_PAYLOAD);
-
             body.ack_frame = self.last_recv_frame();
             body.disconnect_requested = self.state == ProtocolState::Disconnected;
             connect_status.clone_into(&mut body.peer_connect_status);

src/network/udp_socket.rs

@@ -6,6 +6,10 @@ use std::{
 use crate::{network::messages::Message, NonBlockingSocket};
 
 const RECV_BUFFER_SIZE: usize = 4096;
+/// A packet larger than this may be fragmented, so ideally we wouldn't send packets larger than
+/// this.
+/// Source: https://stackoverflow.com/a/35697810/775982
+const IDEAL_MAX_UDP_PACKET_SIZE: usize = 508;
 
 /// A simple non-blocking UDP socket tu use with GGRS Sessions. Listens to 0.0.0.0 on a given port.
 #[derive(Debug)]
@@ -30,6 +34,31 @@ impl UdpNonBlockingSocket {
 impl NonBlockingSocket<SocketAddr> for UdpNonBlockingSocket {
     fn send_to(&mut self, msg: &Message, addr: &SocketAddr) {
         let buf = bincode::serialize(&msg).unwrap();
+
+        // Overly large packets risk being fragmented, which can increase packet loss (any fragment
+        // of a packet getting lost will cause the whole fragment to be lost), or increase latency
+        // to be delayed (have to wait for all fragments to arrive).
+        //
+        // And if there's a large packet that's being sent, it's basically guaranteed that it's
+        // because consuming code has submitted an input struct that is too large (and/or too large
+        // a prediction window on too poor a connection, and/or the input struct did not delta
+        // encode well). So we should let the user of ggrs know about that, so they can fix it by
+        // reducing the size of their input struct.
+        //
+        // On the other hand, the occaisional large packet is kind of harmless - whether it gets
+        // fragmented or not, the odds are that it will get through unless the connection is truly
+        // horrible.
+        //
+        // So ideally we'd inform the user by logging a warning, but we don't have any logging set
+        // up - so as a compromise, we ignore this in release mode but panic in debug mode.
+        if buf.len() > IDEAL_MAX_UDP_PACKET_SIZE && cfg!(debug_assertions) {
+            panic!(
+                "Sending UDP packet of size {} bytes, which is \
+                larger than ideal ({IDEAL_MAX_UDP_PACKET_SIZE})",
+                buf.len()
+            );
+        }
+
         self.socket.send_to(&buf, addr).unwrap();
     }