From 12ab35304a683d72fc34a1f81e8d00a84691a745 Mon Sep 17 00:00:00 2001 From: Nathaniel Ford Date: Wed, 6 May 2026 23:17:37 +0000 Subject: [PATCH 1/4] name_resolution: include structural attributes in Address PartialEq --- grpc/src/client/name_resolution/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/grpc/src/client/name_resolution/mod.rs b/grpc/src/client/name_resolution/mod.rs index 9354d0514..61ebcedee 100644 --- a/grpc/src/client/name_resolution/mod.rs +++ b/grpc/src/client/name_resolution/mod.rs @@ -317,10 +317,13 @@ impl Eq for Address {} impl PartialEq for Address { fn eq(&self, other: &Self) -> bool { - self.network_type == other.network_type && self.address == other.address + self.network_type == other.network_type + && self.address == other.address + && self.attributes == other.attributes } } + impl Hash for Address { fn hash(&self, state: &mut H) { self.network_type.hash(state); From 1018fdaf3086be79553a6f2502fdd67348a16590 Mon Sep 17 00:00:00 2001 From: Nathaniel Ford Date: Wed, 6 May 2026 23:30:36 +0000 Subject: [PATCH 2/4] name_resolution: include structural attributes in Address PartialEq --- grpc/src/client/name_resolution/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/grpc/src/client/name_resolution/mod.rs b/grpc/src/client/name_resolution/mod.rs index 61ebcedee..99d9dae44 100644 --- a/grpc/src/client/name_resolution/mod.rs +++ b/grpc/src/client/name_resolution/mod.rs @@ -323,7 +323,6 @@ impl PartialEq for Address { } } - impl Hash for Address { fn hash(&self, state: &mut H) { self.network_type.hash(state); From acd6b2e29eb1a647fb00b3d974be1bb22f168f0f Mon Sep 17 00:00:00 2001 From: Nathaniel Ford Date: Wed, 6 May 2026 23:55:59 +0000 Subject: [PATCH 3/4] name_resolution: derive structural equality for Address and add map collision unit test --- grpc/src/client/name_resolution/mod.rs | 64 ++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 10 deletions(-) diff --git a/grpc/src/client/name_resolution/mod.rs b/grpc/src/client/name_resolution/mod.rs index 99d9dae44..3f56cbca8 100644 --- a/grpc/src/client/name_resolution/mod.rs +++ b/grpc/src/client/name_resolution/mod.rs @@ -298,7 +298,7 @@ impl Hash for Endpoint { /// An Address is an identifier that indicates how to connect to a server. #[non_exhaustive] -#[derive(Debug, Clone, Default, Ord, PartialOrd)] +#[derive(Debug, Clone, Default, PartialEq, Eq, Ord, PartialOrd)] pub(crate) struct Address { /// The network type is used to identify what kind of transport to create /// when connecting to this address. Typically TCP_IP_ADDRESS_TYPE. @@ -313,15 +313,6 @@ pub(crate) struct Address { pub attributes: Attributes, } -impl Eq for Address {} - -impl PartialEq for Address { - fn eq(&self, other: &Self) -> bool { - self.network_type == other.network_type - && self.address == other.address - && self.attributes == other.attributes - } -} impl Hash for Address { fn hash(&self, state: &mut H) { @@ -452,4 +443,57 @@ mod test { assert_eq!(&target.to_string(), tc.want_str); } } + + // This test ensures that the Address struct correctly maintains its asymmetric PartialEq and Hash contracts. + // Specifically, two addresses with the same physical coordinates but different metadata attributes must + // hash to the same HashMap bucket (intentional collision) but fail strict equality, forcing collection layers + // (like load balancers and connection pools) to safely treat them as distinct endpoints without corrupting the map. + #[test] + fn test_address_hashmap_asymmetric_collision() { + use std::collections::HashMap; + use std::hash::{Hash, Hasher}; + use std::collections::hash_map::DefaultHasher; + use crate::client::name_resolution::Address; + use crate::attributes::Attributes; + use crate::byte_str::ByteStr; + + let addr_base = "127.0.0.1:8080"; + + // Address A: No metadata attributes + let addr_a = Address { + network_type: "tcp", + address: ByteStr::from(addr_base.to_string()), + attributes: Attributes::new(), + }; + + // Address B: Identical text/type coordinates, but WITH metadata attributes + let attrs = Attributes::new().add("metadata_payload".to_string()); + let addr_b = Address { + network_type: "tcp", + address: ByteStr::from(addr_base.to_string()), + attributes: attrs, + }; + + // Invariant Verification: Stricter equality must say they differ + assert_ne!(addr_a, addr_b, "Address equality must be distinct when attributes mutate!"); + + // Invariant Verification: Hashing must ignore attributes (intentional collision) + let mut hasher_a = DefaultHasher::new(); + let mut hasher_b = DefaultHasher::new(); + addr_a.hash(&mut hasher_a); + addr_b.hash(&mut hasher_b); + assert_eq!(hasher_a.finish(), hasher_b.finish(), "Address hashes MUST remain identical to ensure same HashMap memory bucket routing!"); + + // Map Functional Verification + let mut map = HashMap::new(); + map.insert(addr_a.clone(), "subchannel_a"); + + // Querying or removing using B (mutated attributes) must FAIL due to asymmetric == check + assert!(map.remove(&addr_b).is_none(), "Flaw: HashMap incorrectly matched key despite mismatched attributes!"); + + // Querying or removing using A (exact match) must SUCCEED + assert_eq!(map.remove(&addr_a), Some("subchannel_a")); + assert!(map.is_empty()); + } } + From cc467f555bf8f1f0ded42f6274b0d3cfbd9c5810 Mon Sep 17 00:00:00 2001 From: Nathaniel Ford Date: Thu, 7 May 2026 16:27:18 +0000 Subject: [PATCH 4/4] Update test to remove coverage of derived traits, add coverage for manually written hash impl --- grpc/src/client/name_resolution/mod.rs | 82 ++++++++++++++++++-------- 1 file changed, 58 insertions(+), 24 deletions(-) diff --git a/grpc/src/client/name_resolution/mod.rs b/grpc/src/client/name_resolution/mod.rs index 3f56cbca8..9cecab56c 100644 --- a/grpc/src/client/name_resolution/mod.rs +++ b/grpc/src/client/name_resolution/mod.rs @@ -313,7 +313,6 @@ pub(crate) struct Address { pub attributes: Attributes, } - impl Hash for Address { fn hash(&self, state: &mut H) { self.network_type.hash(state); @@ -381,6 +380,12 @@ impl NopResolver { #[cfg(test)] mod test { use super::Target; + use crate::attributes::Attributes; + use crate::byte_str::ByteStr; + use crate::client::name_resolution::Address; + use std::collections::HashMap; + use std::collections::hash_map::DefaultHasher; + use std::hash::{Hash, Hasher}; #[test] pub fn parse_target() { @@ -444,29 +449,25 @@ mod test { } } - // This test ensures that the Address struct correctly maintains its asymmetric PartialEq and Hash contracts. - // Specifically, two addresses with the same physical coordinates but different metadata attributes must - // hash to the same HashMap bucket (intentional collision) but fail strict equality, forcing collection layers - // (like load balancers and connection pools) to safely treat them as distinct endpoints without corrupting the map. + // This test ensures that the Address struct correctly maintains its + // asymmetric PartialEq and Hash contracts. + // Specifically, two addresses with the same physical coordinates but + // different metadata attributes must hash to the same HashMap bucket + // (intentional collision) but fail strict equality, forcing collection + // layers (e.g., load balancers and connection pools) to safely treat them + // as distinct endpoints without corrupting the map. #[test] fn test_address_hashmap_asymmetric_collision() { - use std::collections::HashMap; - use std::hash::{Hash, Hasher}; - use std::collections::hash_map::DefaultHasher; - use crate::client::name_resolution::Address; - use crate::attributes::Attributes; - use crate::byte_str::ByteStr; - let addr_base = "127.0.0.1:8080"; - // Address A: No metadata attributes + // Address A: without metadata attributes let addr_a = Address { network_type: "tcp", address: ByteStr::from(addr_base.to_string()), attributes: Attributes::new(), }; - // Address B: Identical text/type coordinates, but WITH metadata attributes + // Address B: with metadata attributes let attrs = Attributes::new().add("metadata_payload".to_string()); let addr_b = Address { network_type: "tcp", @@ -474,26 +475,59 @@ mod test { attributes: attrs, }; - // Invariant Verification: Stricter equality must say they differ - assert_ne!(addr_a, addr_b, "Address equality must be distinct when attributes mutate!"); - - // Invariant Verification: Hashing must ignore attributes (intentional collision) + // Hashing must ignore attributes (intentional collision) let mut hasher_a = DefaultHasher::new(); let mut hasher_b = DefaultHasher::new(); addr_a.hash(&mut hasher_a); addr_b.hash(&mut hasher_b); - assert_eq!(hasher_a.finish(), hasher_b.finish(), "Address hashes MUST remain identical to ensure same HashMap memory bucket routing!"); + assert_eq!( + hasher_a.finish(), + hasher_b.finish(), + "Identical Address hashes must route to the same HashMap memory bucket!" + ); + + let hash_a = hasher_a.finish(); + + // Verify that changing network_type changes the hash + let addr_diff_net = Address { + network_type: "uds", + address: ByteStr::from(addr_base.to_string()), + attributes: Attributes::new(), + }; + let mut hasher_diff_net = DefaultHasher::new(); + addr_diff_net.hash(&mut hasher_diff_net); + assert_ne!( + hash_a, + hasher_diff_net.finish(), + "Changing network_type must change the hash!" + ); + + // Verify that changing address changes the hash + let addr_diff_addr = Address { + network_type: "tcp", + address: ByteStr::from("127.0.0.1:8081".to_string()), + attributes: Attributes::new(), + }; + let mut hasher_diff_addr = DefaultHasher::new(); + addr_diff_addr.hash(&mut hasher_diff_addr); + assert_ne!( + hash_a, + hasher_diff_addr.finish(), + "Changing address must change the hash!" + ); // Map Functional Verification let mut map = HashMap::new(); map.insert(addr_a.clone(), "subchannel_a"); - // Querying or removing using B (mutated attributes) must FAIL due to asymmetric == check - assert!(map.remove(&addr_b).is_none(), "Flaw: HashMap incorrectly matched key despite mismatched attributes!"); - - // Querying or removing using A (exact match) must SUCCEED + // Removing using B (different attributes) should fail. + assert!( + map.remove(&addr_b).is_none(), + "HashMap incorrectly matched key despite mismatched attributes!" + ); + + // Removing using A (same attributes) should succeed. assert_eq!(map.remove(&addr_a), Some("subchannel_a")); assert!(map.is_empty()); } } -