diff --git a/rust/clippy.toml b/rust/clippy.toml index fb7af89c0..30d4edfea 100644 --- a/rust/clippy.toml +++ b/rust/clippy.toml @@ -2,6 +2,8 @@ avoid-breaking-exported-api = false # We don't publish anything to crates.io, he disallowed-methods = [ { path = "std::collections::HashMap::iter", reason = "HashMap has non-deterministic iteration order, use BTreeMap instead" }, { path = "std::collections::HashSet::iter", reason = "HashSet has non-deterministic iteration order, use BTreeSet instead" }, + { path = "std::collections::HashMap::iter_mut", reason = "HashMap has non-deterministic iteration order, use BTreeMap instead" }, + { path = "std::collections::HashSet::iter_mut", reason = "HashSet has non-deterministic iteration order, use BTreeSet instead" }, { path = "std::collections::HashMap::into_iter", reason = "HashMap has non-deterministic iteration order, use BTreeMap instead" }, { path = "std::collections::HashSet::into_iter", reason = "HashSet has non-deterministic iteration order, use BTreeSet instead" }, { path = "tracing::subscriber::set_global_default", reason = "Does not init `LogTracer`, use `firezone_logging::init` instead." }, diff --git a/rust/connlib/tunnel/src/io/gso_queue.rs b/rust/connlib/tunnel/src/io/gso_queue.rs index a051a4417..1bf20c20e 100644 --- a/rust/connlib/tunnel/src/io/gso_queue.rs +++ b/rust/connlib/tunnel/src/io/gso_queue.rs @@ -1,5 +1,5 @@ use std::{ - collections::HashMap, + collections::BTreeMap, net::SocketAddr, sync::Arc, time::{Duration, Instant}, @@ -19,7 +19,7 @@ const MAX_SEGMENT_SIZE: usize = /// Calling [`Io::send_network`](super::Io::send_network) will copy the provided payload into this buffer. /// The buffer is then flushed using GSO in a single syscall. pub struct GsoQueue { - inner: HashMap, + inner: BTreeMap, buffer_pool: Arc>, } @@ -117,9 +117,9 @@ impl GsoQueue { #[derive(Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Clone, Copy)] struct Key { + segment_size: usize, // `segment_size` comes first to ensure that the datagrams are flushed to the socket in ascending order. src: Option, dst: SocketAddr, - segment_size: usize, } struct DatagramBuffer { @@ -139,7 +139,7 @@ mod tests { let now = Instant::now(); let mut send_queue = GsoQueue::new(); - send_queue.enqueue(None, DST, b"foobar", Ecn::NonEct, now); + send_queue.enqueue(None, DST_1, b"foobar", Ecn::NonEct, now); for _entry in send_queue.datagrams() {} send_queue.handle_timeout(now + Duration::from_secs(60)); @@ -152,7 +152,7 @@ mod tests { let now = Instant::now(); let mut send_queue = GsoQueue::new(); - send_queue.enqueue(None, DST, b"foobar", Ecn::NonEct, now); + send_queue.enqueue(None, DST_1, b"foobar", Ecn::NonEct, now); send_queue.handle_timeout(now + Duration::from_secs(60)); @@ -164,7 +164,7 @@ mod tests { let now = Instant::now(); let mut send_queue = GsoQueue::new(); - send_queue.enqueue(None, DST, b"foobar", Ecn::NonEct, now); + send_queue.enqueue(None, DST_1, b"foobar", Ecn::NonEct, now); let datagrams = send_queue.datagrams(); drop(datagrams); @@ -172,7 +172,7 @@ mod tests { let datagrams = send_queue.datagrams().collect::>(); assert_eq!(datagrams.len(), 1); - assert_eq!(datagrams[0].dst, DST); + assert_eq!(datagrams[0].dst, DST_1); assert_eq!(datagrams[0].packet.as_ref(), b"foobar"); } @@ -181,7 +181,7 @@ mod tests { let now = Instant::now(); let mut send_queue = GsoQueue::new(); - send_queue.enqueue(None, DST, b"foobar", Ecn::NonEct, now); + send_queue.enqueue(None, DST_1, b"foobar", Ecn::NonEct, now); send_queue.enqueue(None, DST_2, b"bar", Ecn::NonEct, now); // Taking it from the iterator is "sending" ... @@ -192,6 +192,35 @@ mod tests { } } - const DST: SocketAddr = SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::LOCALHOST, 1234)); - const DST_2: SocketAddr = SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::LOCALHOST, 5678)); + #[test] + fn prioritises_small_packets() { + let now = Instant::now(); + let mut send_queue = GsoQueue::new(); + + send_queue.enqueue( + None, + DST_1, + b"foobarfoobarfoobarfoobarfoobarfoobarfoobarfoobar", + Ecn::NonEct, + now, + ); + send_queue.enqueue(None, DST_2, b"barbaz", Ecn::NonEct, now); + send_queue.enqueue(None, DST_3, b"barbaz1234", Ecn::NonEct, now); + send_queue.enqueue(None, DST_4, b"b", Ecn::NonEct, now); + send_queue.enqueue(None, DST_5, b"barbazfoobafoobarfoobar", Ecn::NonEct, now); + send_queue.enqueue(None, DST_2, b"baz", Ecn::NonEct, now); + + let datagrams = send_queue.datagrams().collect::>(); + + let is_sorted = datagrams.is_sorted_by_key(|datagram| datagram.segment_size); + + assert!(is_sorted); + assert_eq!(datagrams[0].segment_size, Some(1)); + } + + const DST_1: SocketAddr = SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::LOCALHOST, 1111)); + const DST_2: SocketAddr = SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::LOCALHOST, 2222)); + const DST_3: SocketAddr = SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::LOCALHOST, 3333)); + const DST_4: SocketAddr = SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::LOCALHOST, 4444)); + const DST_5: SocketAddr = SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::LOCALHOST, 5555)); } diff --git a/rust/connlib/tunnel/src/tests/sim_gateway.rs b/rust/connlib/tunnel/src/tests/sim_gateway.rs index 8b30efdf2..b45ba2f9a 100644 --- a/rust/connlib/tunnel/src/tests/sim_gateway.rs +++ b/rust/connlib/tunnel/src/tests/sim_gateway.rs @@ -15,7 +15,7 @@ use ip_packet::{IcmpEchoHeader, Icmpv4Type, Icmpv6Type, IpPacket}; use proptest::prelude::*; use snownet::Transmit; use std::{ - collections::{BTreeMap, HashMap}, + collections::BTreeMap, iter, net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}, time::Instant, @@ -36,8 +36,8 @@ pub(crate) struct SimGateway { pub(crate) received_tcp_requests: BTreeMap, site_specific_dns_records: DnsRecords, - udp_dns_server_resources: HashMap, - tcp_dns_server_resources: HashMap, + udp_dns_server_resources: BTreeMap, + tcp_dns_server_resources: BTreeMap, } impl SimGateway {