From 8ee4b5e3b38d82f0222f0d3c9a8900ecc6afe590 Mon Sep 17 00:00:00 2001 From: Caspar Krieger Date: Thu, 12 Dec 2024 16:36:35 +0800 Subject: [PATCH] Only panic when sending large packets if in debug mode And move the packet size checking to the UDP socket layer, since technically the thresholds will be different for different network transports. --- CHANGELOG.md | 2 ++ src/network/protocol.rs | 4 ---- src/network/udp_socket.rs | 29 +++++++++++++++++++++++++++++ 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d0fc15b..e1d3c15 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/network/protocol.rs b/src/network/protocol.rs index 6fef863..04bebce 100644 --- a/src/network/protocol.rs +++ b/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 UdpProtocol { 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); diff --git a/src/network/udp_socket.rs b/src/network/udp_socket.rs index 83fc43f..7dc4a2c 100644 --- a/src/network/udp_socket.rs +++ b/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 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(); }