From 2ac84848161aea2ba126b3d389375f0acda300a0 Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 11 Aug 2025 19:19:44 +0100 Subject: [PATCH 1/4] Rename listener binding API and assert bound ports --- src/server/config/binding.rs | 12 ++++----- src/server/config/mod.rs | 6 ++--- src/server/config/tests.rs | 35 +++++++++++---------------- src/server/connection.rs | 2 +- src/server/mod.rs | 4 +-- src/server/runtime.rs | 2 +- src/server/test_util.rs | 47 +++++++++++++++++++++++++++++++++++- tests/preamble.rs | 2 +- tests/server.rs | 2 +- tests/world.rs | 2 +- 10 files changed, 76 insertions(+), 38 deletions(-) diff --git a/src/server/config/binding.rs b/src/server/config/binding.rs index fdadc041..f9c42be2 100644 --- a/src/server/config/binding.rs +++ b/src/server/config/binding.rs @@ -56,7 +56,7 @@ where /// Returns a [`ServerError`] if binding or configuring the listener fails. pub fn bind(self, addr: SocketAddr) -> Result, ServerError> { let std = StdTcpListener::bind(addr).map_err(ServerError::Bind)?; - self.bind_listener(std) + self.bind_existing_listener(std) } /// Bind to an existing `StdTcpListener`. @@ -70,14 +70,14 @@ where /// /// let std = StdTcpListener::bind(SocketAddr::from((Ipv4Addr::LOCALHOST, 0))).unwrap(); /// let server = WireframeServer::new(|| WireframeApp::default()) - /// .bind_listener(std) + /// .bind_existing_listener(std) /// .expect("bind failed"); /// assert!(server.local_addr().is_some()); /// ``` /// /// # Errors /// Returns a [`ServerError`] if configuring the listener fails. - pub fn bind_listener( + pub fn bind_existing_listener( self, std: StdTcpListener, ) -> Result, ServerError> { @@ -142,7 +142,7 @@ where /// Returns a [`ServerError`] if binding or configuring the listener fails. pub fn bind(self, addr: SocketAddr) -> Result { let std = StdTcpListener::bind(addr).map_err(ServerError::Bind)?; - self.bind_listener(std) + self.bind_existing_listener(std) } /// Rebind using an existing `StdTcpListener`. @@ -159,13 +159,13 @@ where /// .bind(addr) /// .expect("bind failed"); /// let std = StdTcpListener::bind(SocketAddr::from((Ipv4Addr::LOCALHOST, 0))).unwrap(); - /// let server = server.bind_listener(std).expect("rebind failed"); + /// let server = server.bind_existing_listener(std).expect("rebind failed"); /// assert!(server.local_addr().is_some()); /// ``` /// /// # Errors /// Returns a [`ServerError`] if configuring the listener fails. - pub fn bind_listener(self, std: StdTcpListener) -> Result { + pub fn bind_existing_listener(self, std: StdTcpListener) -> Result { std.set_nonblocking(true).map_err(ServerError::Bind)?; let tokio = TcpListener::from_std(std).map_err(ServerError::Bind)?; Ok(WireframeServer { diff --git a/src/server/config/mod.rs b/src/server/config/mod.rs index fa5ce2da..cd742850 100644 --- a/src/server/config/mod.rs +++ b/src/server/config/mod.rs @@ -5,8 +5,8 @@ //! TCP binding is provided via the [`binding`](self::binding) module; preamble //! behaviour is customized via the [`preamble`](self::preamble) module. The //! server may be constructed unbound and later bound using -//! [`bind`](WireframeServer::bind) or [`bind_listener`](WireframeServer::bind_listener) -//! on [`Unbound`] servers. +//! [`bind`](WireframeServer::bind) or +//! [`bind_existing_listener`](WireframeServer::bind_existing_listener) on [`Unbound`] servers. use core::marker::PhantomData; @@ -52,7 +52,7 @@ where /// The worker count defaults to the number of available CPU cores (or 1 if /// this cannot be determined). The server is initially [`Unbound`]; call /// [`bind`](WireframeServer::bind) or - /// [`bind_listener`](WireframeServer::bind_listener) + /// [`bind_existing_listener`](WireframeServer::bind_existing_listener) /// (methods provided by the [`binding`](self::binding) module) before running the server. /// /// # Examples diff --git a/src/server/config/tests.rs b/src/server/config/tests.rs index ff47860a..e17d1009 100644 --- a/src/server/config/tests.rs +++ b/src/server/config/tests.rs @@ -6,7 +6,6 @@ //! cases via `rstest`. use std::{ - net::SocketAddr, sync::{ Arc, atomic::{AtomicUsize, Ordering}, @@ -21,7 +20,9 @@ use crate::server::test_util::{ TestPreamble, bind_server, factory, + free_addr, free_listener, + listener_addr, server_with_preamble, }; @@ -68,15 +69,13 @@ async fn test_bind_success( factory: impl Fn() -> WireframeApp + Send + Sync + Clone + 'static, free_listener: std::net::TcpListener, ) { - let expected = free_listener - .local_addr() - .expect("failed to get listener address"); + let expected = listener_addr(&free_listener); let local_addr = WireframeServer::new(factory) - .bind_listener(free_listener) + .bind_existing_listener(free_listener) .expect("Failed to bind") .local_addr() .expect("local address missing"); - assert_eq!(local_addr.ip(), expected.ip()); + assert_eq!(local_addr, expected); } #[rstest] @@ -90,11 +89,9 @@ async fn test_local_addr_after_bind( factory: impl Fn() -> WireframeApp + Send + Sync + Clone + 'static, free_listener: std::net::TcpListener, ) { - let expected = free_listener - .local_addr() - .expect("failed to get listener address"); + let expected = listener_addr(&free_listener); let local_addr = bind_server(factory, free_listener).local_addr().unwrap(); - assert_eq!(local_addr.ip(), expected.ip()); + assert_eq!(local_addr, expected); } #[rstest] @@ -150,7 +147,7 @@ async fn test_method_chaining( }) }) .on_preamble_decode_failure(|_: &DecodeError| {}) - .bind_listener(free_listener) + .bind_existing_listener(free_listener) .expect("Failed to bind"); assert_eq!(server.worker_count(), 2); assert!(server.local_addr().is_some()); @@ -165,7 +162,7 @@ async fn test_server_configuration_persistence( ) { let server = WireframeServer::new(factory) .workers(5) - .bind_listener(free_listener) + .bind_existing_listener(free_listener) .expect("Failed to bind"); assert_eq!(server.worker_count(), 5); assert!(server.local_addr().is_some()); @@ -185,23 +182,19 @@ async fn test_bind_to_multiple_addresses( factory: impl Fn() -> WireframeApp + Send + Sync + Clone + 'static, free_listener: std::net::TcpListener, ) { - let listener2 = - std::net::TcpListener::bind(SocketAddr::new(std::net::Ipv4Addr::LOCALHOST.into(), 0)) - .expect("failed to bind second listener"); - let addr2 = listener2 - .local_addr() - .expect("failed to get second listener address"); - drop(listener2); + let addr1 = listener_addr(&free_listener); + let addr2 = free_addr(); let server = WireframeServer::new(factory); let server = server - .bind_listener(free_listener) + .bind_existing_listener(free_listener) .expect("Failed to bind first address"); let first = server.local_addr().expect("first bound address missing"); + assert_eq!(first, addr1); let server = server.bind(addr2).expect("Failed to bind second address"); let second = server.local_addr().expect("second bound address missing"); + assert_eq!(second, addr2); assert_ne!(first.port(), second.port()); - assert_eq!(second.ip(), addr2.ip()); } #[rstest] diff --git a/src/server/connection.rs b/src/server/connection.rs index 5de438f4..9d71a783 100644 --- a/src/server/connection.rs +++ b/src/server/connection.rs @@ -167,7 +167,7 @@ mod tests { }; let server = WireframeServer::new(app_factory) .workers(1) - .bind_listener(free_listener) + .bind_existing_listener(free_listener) .expect("bind"); let addr = server .local_addr() diff --git a/src/server/mod.rs b/src/server/mod.rs index 1177e77d..88faa82b 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -67,8 +67,8 @@ pub type PreambleErrorHandler = Arc StdTcpListener { StdTcpListener::bind(addr).expect("Failed to bind free port listener") } +/// Extract the bound address from a listener. +/// +/// # Examples +/// +/// ``` +/// use std::net::TcpListener; +/// +/// use wireframe::server::test_util::{free_listener, listener_addr}; +/// +/// let listener = free_listener(); +/// let addr = listener_addr(&listener); +/// assert_eq!(listener.local_addr().unwrap(), addr); +/// ``` +#[cfg_attr(test, allow(dead_code, reason = "Used via path in tests"))] +#[cfg_attr(not(test), expect(dead_code, reason = "Only used in tests"))] +#[must_use] +pub fn listener_addr(listener: &StdTcpListener) -> SocketAddr { + listener + .local_addr() + .expect("failed to get listener address") +} + +/// Reserve a free local port and return its address. +/// +/// A temporary listener is created and immediately dropped so the port may be +/// rebound. +/// +/// # Examples +/// +/// ``` +/// use wireframe::server::test_util::free_addr; +/// +/// let addr = free_addr(); +/// assert_eq!(addr.ip(), std::net::Ipv4Addr::LOCALHOST.into()); +/// ``` +#[cfg_attr(test, allow(dead_code, reason = "Used via path in tests"))] +#[cfg_attr(not(test), expect(dead_code, reason = "Only used in tests"))] +#[must_use] +pub fn free_addr() -> SocketAddr { + let listener = free_listener(); + let addr = listener_addr(&listener); + drop(listener); + addr +} + pub fn bind_server(factory: F, listener: StdTcpListener) -> WireframeServer where F: Fn() -> WireframeApp + Send + Sync + Clone + 'static, { WireframeServer::new(factory) - .bind_listener(listener) + .bind_existing_listener(listener) .expect("Failed to bind") } diff --git a/tests/preamble.rs b/tests/preamble.rs index 1671da06..fb54e7c1 100644 --- a/tests/preamble.rs +++ b/tests/preamble.rs @@ -69,7 +69,7 @@ where B: FnOnce(std::net::SocketAddr) -> Fut, { let listener = unused_listener(); - let server = server.bind_listener(listener).expect("bind"); + let server = server.bind_existing_listener(listener).expect("bind"); let addr = server.local_addr().expect("addr"); let (shutdown_tx, shutdown_rx) = oneshot::channel::<()>(); let handle = tokio::spawn(async move { diff --git a/tests/server.rs b/tests/server.rs index d9b97e99..f04503ca 100644 --- a/tests/server.rs +++ b/tests/server.rs @@ -42,7 +42,7 @@ async fn readiness_receiver_dropped() { let listener = unused_listener(); let server = WireframeServer::new(factory()) .workers(1) - .bind_listener(listener) + .bind_existing_listener(listener) .unwrap(); let addr = server.local_addr().expect("local addr missing"); diff --git a/tests/world.rs b/tests/world.rs index cb32dd4a..a6e19abe 100644 --- a/tests/world.rs +++ b/tests/world.rs @@ -39,7 +39,7 @@ impl PanicServer { let listener = unused_listener(); let server = WireframeServer::new(factory) .workers(1) - .bind_listener(listener) + .bind_existing_listener(listener) .expect("bind"); let addr = server.local_addr().expect("Failed to get server address"); let (tx_shutdown, rx_shutdown) = oneshot::channel(); From ec89399947ca54c7e4cc138f058cc57e20b3f885 Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 11 Aug 2025 19:38:53 +0100 Subject: [PATCH 2/4] Reserve test ports until binding --- src/server/config/tests.rs | 5 +++-- src/server/test_util.rs | 23 ----------------------- 2 files changed, 3 insertions(+), 25 deletions(-) diff --git a/src/server/config/tests.rs b/src/server/config/tests.rs index e17d1009..3aece8c8 100644 --- a/src/server/config/tests.rs +++ b/src/server/config/tests.rs @@ -20,7 +20,6 @@ use crate::server::test_util::{ TestPreamble, bind_server, factory, - free_addr, free_listener, listener_addr, server_with_preamble, @@ -183,7 +182,8 @@ async fn test_bind_to_multiple_addresses( free_listener: std::net::TcpListener, ) { let addr1 = listener_addr(&free_listener); - let addr2 = free_addr(); + let listener2 = crate::server::test_util::free_listener(); + let addr2 = listener_addr(&listener2); let server = WireframeServer::new(factory); let server = server @@ -191,6 +191,7 @@ async fn test_bind_to_multiple_addresses( .expect("Failed to bind first address"); let first = server.local_addr().expect("first bound address missing"); assert_eq!(first, addr1); + drop(listener2); let server = server.bind(addr2).expect("Failed to bind second address"); let second = server.local_addr().expect("second bound address missing"); assert_eq!(second, addr2); diff --git a/src/server/test_util.rs b/src/server/test_util.rs index 37051d99..3289a5b0 100644 --- a/src/server/test_util.rs +++ b/src/server/test_util.rs @@ -56,29 +56,6 @@ pub fn listener_addr(listener: &StdTcpListener) -> SocketAddr { .expect("failed to get listener address") } -/// Reserve a free local port and return its address. -/// -/// A temporary listener is created and immediately dropped so the port may be -/// rebound. -/// -/// # Examples -/// -/// ``` -/// use wireframe::server::test_util::free_addr; -/// -/// let addr = free_addr(); -/// assert_eq!(addr.ip(), std::net::Ipv4Addr::LOCALHOST.into()); -/// ``` -#[cfg_attr(test, allow(dead_code, reason = "Used via path in tests"))] -#[cfg_attr(not(test), expect(dead_code, reason = "Only used in tests"))] -#[must_use] -pub fn free_addr() -> SocketAddr { - let listener = free_listener(); - let addr = listener_addr(&listener); - drop(listener); - addr -} - pub fn bind_server(factory: F, listener: StdTcpListener) -> WireframeServer where F: Fn() -> WireframeApp + Send + Sync + Clone + 'static, From ff0b404b8c161e7bedd3a463db0381a0e1466dc9 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 02:19:32 +0100 Subject: [PATCH 3/4] Clarify free_addr races and tighten bind tests --- src/server/config/tests.rs | 14 ++++++++------ src/server/test_util.rs | 25 ++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/src/server/config/tests.rs b/src/server/config/tests.rs index 3aece8c8..485b7ac4 100644 --- a/src/server/config/tests.rs +++ b/src/server/config/tests.rs @@ -89,7 +89,9 @@ async fn test_local_addr_after_bind( free_listener: std::net::TcpListener, ) { let expected = listener_addr(&free_listener); - let local_addr = bind_server(factory, free_listener).local_addr().unwrap(); + let local_addr = bind_server(factory, free_listener) + .local_addr() + .expect("local address missing"); assert_eq!(local_addr, expected); } @@ -182,8 +184,6 @@ async fn test_bind_to_multiple_addresses( free_listener: std::net::TcpListener, ) { let addr1 = listener_addr(&free_listener); - let listener2 = crate::server::test_util::free_listener(); - let addr2 = listener_addr(&listener2); let server = WireframeServer::new(factory); let server = server @@ -191,10 +191,12 @@ async fn test_bind_to_multiple_addresses( .expect("Failed to bind first address"); let first = server.local_addr().expect("first bound address missing"); assert_eq!(first, addr1); - drop(listener2); - let server = server.bind(addr2).expect("Failed to bind second address"); + + let server = server + .bind(std::net::SocketAddr::new(addr1.ip(), 0)) + .expect("Failed to bind second address"); let second = server.local_addr().expect("second bound address missing"); - assert_eq!(second, addr2); + assert_eq!(second.ip(), addr1.ip()); assert_ne!(first.port(), second.port()); } diff --git a/src/server/test_util.rs b/src/server/test_util.rs index 3289a5b0..a91c39f2 100644 --- a/src/server/test_util.rs +++ b/src/server/test_util.rs @@ -34,6 +34,24 @@ pub fn free_listener() -> StdTcpListener { StdTcpListener::bind(addr).expect("Failed to bind free port listener") } +/// Reserve a free local port and return its address. +/// +/// Creates a temporary listener to obtain an ephemeral port, then immediately +/// drops it so the port may be rebound. This is inherently subject to a +/// time-of-check/time-of-use race; only use in tests. +/// +/// # Examples +/// +/// ```no_run +/// use wireframe::server::test_util::free_addr; +/// let addr = free_addr(); +/// assert_eq!(addr.ip(), std::net::Ipv4Addr::LOCALHOST.into()); +/// ``` +#[cfg(test)] +#[allow(dead_code, reason = "Used only in doctests")] +#[must_use] +pub fn free_addr() -> SocketAddr { listener_addr(&free_listener()) } + /// Extract the bound address from a listener. /// /// # Examples @@ -45,7 +63,12 @@ pub fn free_listener() -> StdTcpListener { /// /// let listener = free_listener(); /// let addr = listener_addr(&listener); -/// assert_eq!(listener.local_addr().unwrap(), addr); +/// assert_eq!( +/// listener +/// .local_addr() +/// .expect("failed to get listener address"), +/// addr +/// ); /// ``` #[cfg_attr(test, allow(dead_code, reason = "Used via path in tests"))] #[cfg_attr(not(test), expect(dead_code, reason = "Only used in tests"))] From 6c86c542b5686399c884786b029a7f3fcfc333c9 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 12 Aug 2025 09:43:40 +0100 Subject: [PATCH 4/4] Exercise test utilities to avoid dead code --- src/server/test_util.rs | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/src/server/test_util.rs b/src/server/test_util.rs index a91c39f2..98d1ea60 100644 --- a/src/server/test_util.rs +++ b/src/server/test_util.rs @@ -48,7 +48,6 @@ pub fn free_listener() -> StdTcpListener { /// assert_eq!(addr.ip(), std::net::Ipv4Addr::LOCALHOST.into()); /// ``` #[cfg(test)] -#[allow(dead_code, reason = "Used only in doctests")] #[must_use] pub fn free_addr() -> SocketAddr { listener_addr(&free_listener()) } @@ -70,8 +69,7 @@ pub fn free_addr() -> SocketAddr { listener_addr(&free_listener()) } /// addr /// ); /// ``` -#[cfg_attr(test, allow(dead_code, reason = "Used via path in tests"))] -#[cfg_attr(not(test), expect(dead_code, reason = "Only used in tests"))] +#[cfg(test)] #[must_use] pub fn listener_addr(listener: &StdTcpListener) -> SocketAddr { listener @@ -88,14 +86,36 @@ where .expect("Failed to bind") } -#[cfg_attr( - not(test), - expect(dead_code, reason = "Only used in configuration tests") -)] -#[cfg_attr(test, allow(dead_code, reason = "Only used in configuration tests"))] +#[cfg(test)] pub fn server_with_preamble(factory: F) -> WireframeServer where F: Fn() -> WireframeApp + Send + Sync + Clone + 'static, { WireframeServer::new(factory).with_preamble::() } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn free_addr_uses_localhost() { + let addr = free_addr(); + assert_eq!(addr.ip(), std::net::IpAddr::from(Ipv4Addr::LOCALHOST)); + } + + #[test] + fn listener_addr_matches_local_addr() { + let listener = free_listener(); + assert_eq!( + listener_addr(&listener), + listener.local_addr().expect("failed to get address") + ); + } + + #[test] + fn server_with_preamble_is_unbound() { + let server = server_with_preamble(factory()); + assert!(server.local_addr().is_none()); + } +}