From 974b50c5848788162ab47bb1250dec28f6111d12 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 19 Sep 2026 15:28:37 +0200 Subject: [PATCH 1/3] Serve raw HTTP bytes from the shared test fixture (#743) `malformed_status_line_failure` stood up its own `TcpListener`, wrote a status line no parser accepts, and dropped the stream without reading the request. On Windows a close with unread peer data can become a connection abort, so `ureq` returned `Error::Io(ConnectionAborted)` before it ever parsed the status line, and the test asserted the platform's close semantics instead of the parser's verdict. The fixture now serves caller-supplied bytes. `RawHttpResponse` carries them; `DriveStrategy` lets the structured and raw shapes share one accept loop, one bounded request read, one accounting path, and one shutdown, so no second listener exists to drift. The raw path drains the request before writing and then shuts down the write half, which is what keeps a transport abort from masking the parse failure under test. Production redirect classification is untouched: `Error::Io(_)` with `ErrorKind::ConnectionAborted` still classifies as `"connection"`, and the test still requires `ureq::Error::Protocol(_)` and category `"protocol"`. --- src/stdlib/network/redirect_error_tests.rs | 34 ++--- test_support/src/http/mod.rs | 91 ++++++++++- test_support/src/http/response.rs | 47 ++++++ test_support/src/http/server.rs | 168 +++++++++++++++++++-- test_support/src/http/tests.rs | 74 ++++++++- 5 files changed, 370 insertions(+), 44 deletions(-) diff --git a/src/stdlib/network/redirect_error_tests.rs b/src/stdlib/network/redirect_error_tests.rs index b90d81566..413355a11 100644 --- a/src/stdlib/network/redirect_error_tests.rs +++ b/src/stdlib/network/redirect_error_tests.rs @@ -7,11 +7,11 @@ //! renames or adds a variant is caught here rather than by a silent fallback to //! `other`. -use std::io::Write as _; use std::time::Duration; use anyhow::{Context, Result, bail, ensure}; use rstest::rstest; +use test_support::http::{self, RawHttpResponse}; use super::*; @@ -92,33 +92,29 @@ fn protocol_failures_are_classified_from_a_live_response() -> Result<()> { /// Drive a hop against a server whose status line cannot be parsed. /// -/// The fixture writes well-formed responses, so this stands a bare listener in -/// its place and answers with a status line no HTTP client accepts. +/// The structured fixture renders only responses a real server could send, so +/// the raw-response fixture supplies the bytes instead. Its URL carries the +/// credentials this hop redacts, and its fixture drains the request before +/// writing and then half-closes, so the malformed bytes are received whole +/// rather than the read failing first. /// /// # Errors /// -/// Returns an error when the listener cannot be bound or read, the malformed -/// response cannot be delivered, or the hop unexpectedly succeeds. +/// Returns an error when the fixture cannot be started or joined, the test URL +/// cannot be parsed, or the hop unexpectedly succeeds. fn malformed_status_line_failure() -> Result { - let listener = std::net::TcpListener::bind(("127.0.0.1", 0)) - .context("bind a malformed-response server")?; - let port = listener - .local_addr() - .context("read the malformed-response server address")? - .port(); - let server = std::thread::spawn(move || -> std::io::Result<()> { - let (mut stream, _addr) = listener.accept()?; - stream.write_all(b"HTTP/1.1 banana OK\r\nContent-Length: 0\r\n\r\n") - }); + let (raw, _log, server) = http::spawn_http_server_raw_response(RawHttpResponse::new( + b"HTTP/1.1 banana OK\r\nContent-Length: 0\r\n\r\n".as_slice(), + )) + .context("spawn a malformed-response server")?; - let raw = format!("http://redirect-user:redirect-secret@127.0.0.1:{port}/start"); - let url = Url::parse(&raw).with_context(|| format!("test URL should parse: {raw}"))?; + let url = Url::parse(&format!("{raw}/start")) + .with_context(|| format!("test URL should parse: {raw}"))?; let agent = build_redirect_agent(); let outcome = request_hop(&agent, &url, Duration::from_secs(5)); server .join() - .map_err(|_panic| anyhow::anyhow!("malformed-response server panicked"))? - .context("write the malformed status line")?; + .map_err(|_panic| anyhow::anyhow!("malformed-response server panicked"))?; let Err(err) = outcome else { bail!("a malformed status line must fail the hop"); }; diff --git a/test_support/src/http/mod.rs b/test_support/src/http/mod.rs index ab57d6467..42d57e9e3 100644 --- a/test_support/src/http/mod.rs +++ b/test_support/src/http/mod.rs @@ -23,8 +23,8 @@ mod server; use self::accept::{AcceptWait, accept_connection}; pub use self::request::RequestLog; -pub use self::response::HttpResponse; -use self::server::{FixtureLedger, run_http_server}; +pub use self::response::{HttpResponse, RawHttpResponse}; +use self::server::{DriveStrategy, FixtureLedger, RawResponses, StructuredResponses}; /// Override for the timeout in milliseconds within which a client must connect. pub(crate) const ENV_HTTP_ACCEPT_TIMEOUT_MS: &str = "NETSUKE_TEST_HTTP_ACCEPT_TIMEOUT_MS"; @@ -279,6 +279,57 @@ pub fn spawn_http_server_expecting_no_requests( Ok((url, log, server)) } +/// Spawn an HTTP server that emits each raw response in sequence. +/// +/// # Raw responses +/// +/// The bytes in `responses` are written to the client verbatim, so a caller +/// can emit a status line no HTTP client should accept. This exists because +/// the structured fixtures cannot: [`HttpResponse::new`] takes a status code, +/// so a response whose status line is malformed is not expressible at all, +/// and a test about a client's failure to parse one has to own the bytes. +/// +/// The returned URL carries `redirect-user:redirect-secret` credentials, so a +/// test can drive a credentialed fetch — whose URL redaction is asserted +/// elsewhere — against malformed bytes rather than against a live host. +/// +/// Every raw response is sent over the same fixture machinery as a structured +/// one: the same bounded accept, the same bounded request read, the same +/// accounting and shutdown. The request is drained before the bytes go out, +/// and the write half is then shut down rather than the socket closed, so the +/// response is delivered whole instead of racing a platform-dependent +/// transport abort. See `server::serve_raw_response` for the full reasoning. +/// +/// # Errors +/// +/// Returns an [`io::Error`] if the listener cannot be bound, switched to +/// non-blocking mode, queried for its local address, or if the fixture thread +/// fails to spawn. As with the structured fixtures, later I/O failures inside +/// the fixture thread panic it rather than returning. +pub fn spawn_http_server_raw_responses( + responses: impl IntoIterator, +) -> io::Result<(String, RequestLog, HttpServer)> { + let (url, _requests, log, server) = spawn_raw_fixture_server( + responses, + HttpServerConfig::from_env().accepting_until_shutdown(), + )?; + Ok((url, log, server)) +} + +/// Spawn an HTTP server that emits one raw response. +/// +/// The singular form of [`spawn_http_server_raw_responses`], for the common +/// case of a test that drives exactly one hop against malformed bytes. +/// +/// # Errors +/// +/// Propagates failures while starting the fixture server. +pub fn spawn_http_server_raw_response( + response: RawHttpResponse, +) -> io::Result<(String, RequestLog, HttpServer)> { + spawn_http_server_raw_responses([response]) +} + /// Spawn an HTTP server using `config`, emitting responses in sequence. /// /// Returns the bound URL, the shared request count, the request log, and the @@ -289,10 +340,41 @@ fn spawn_fixture_server( config: HttpServerConfig, ) -> io::Result<(String, Arc, RequestLog, HttpServer)> { let response_sequence = responses.into_iter().collect::>(); + spawn_fixture_thread( + Box::new(StructuredResponses::new(response_sequence)), + config, + ) +} + +/// Spawn an HTTP server using `config`, emitting raw responses in sequence. +/// +/// The raw counterpart of [`spawn_fixture_server`], and the only place the two +/// shapes diverge: both hand a [`DriveStrategy`] to +/// [`spawn_fixture_thread`], which owns binding, accounting, and the thread. +fn spawn_raw_fixture_server( + responses: impl IntoIterator, + config: HttpServerConfig, +) -> io::Result<(String, Arc, RequestLog, HttpServer)> { + let response_sequence = responses.into_iter().collect::>(); + spawn_fixture_thread(Box::new(RawResponses::new(response_sequence)), config) +} + +/// Serve `strategy` on a fresh listener, returning its URL and shared state. +/// +/// This is the one implementation behind every fixture the module exposes. A +/// [`DriveStrategy`] supplies what to emit and how to advertise it; everything +/// else — binding a loopback port, the request counter and log shared with the +/// caller, the shutdown flag, the named thread, and the handle that joins or +/// signals them — is common, so no fixture shape can drift from another's +/// acceptance, accounting, or shutdown behaviour. +fn spawn_fixture_thread( + strategy: Box, + config: HttpServerConfig, +) -> io::Result<(String, Arc, RequestLog, HttpServer)> { let listener = TcpListener::bind(("127.0.0.1", 0))?; listener.set_nonblocking(true)?; let addr = listener.local_addr()?; - let url = format!("http://{addr}"); + let url = strategy.advertise(addr); let requests = Arc::new(AtomicUsize::new(0)); let server_requests = Arc::clone(&requests); let log = RequestLog::default(); @@ -302,9 +384,8 @@ fn spawn_fixture_server( let handle = thread::Builder::new() .name("netsuke-http-fixture".into()) .spawn(move || { - run_http_server( + strategy.drive( &listener, - &response_sequence, &config, &FixtureLedger::new(&server_requests, &server_log, &server_shutdown), ); diff --git a/test_support/src/http/response.rs b/test_support/src/http/response.rs index 11dc4d1db..d4b67f0d6 100644 --- a/test_support/src/http/response.rs +++ b/test_support/src/http/response.rs @@ -1,4 +1,9 @@ //! HTTP response shapes emitted by the local test fixture. +//! +//! Two shapes are emitted. [`HttpResponse`] describes a valid structured +//! response, which is what most fixtures want; [`RawHttpResponse`] carries the +//! bytes themselves, for tests about what a client does when the bytes on the +//! wire are not a response any server should send. use std::{io, io::Write, net::TcpStream}; @@ -32,6 +37,36 @@ impl HttpResponse { } } +/// Describe one response emitted by the local HTTP fixture as raw bytes. +/// +/// [`HttpResponse`] is the fixture's normal shape: it renders a valid +/// response, so every field it accepts is one a real server could send. A test +/// about what a client does when the bytes on the wire are *not* valid — a +/// status line no client can parse, a truncated header block — cannot be +/// expressed that way, because the constructor refuses the very input the test +/// needs. This type carries the bytes instead, and imposes nothing on them. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RawHttpResponse { + /// Bytes written to the client verbatim, in this order. + bytes: Vec, +} + +impl RawHttpResponse { + /// Create a response that writes `bytes` to the client verbatim. + #[must_use] + pub fn new(bytes: impl Into>) -> Self { + Self { + bytes: bytes.into(), + } + } + + /// Return the bytes this response writes to the client. + #[must_use] + pub fn bytes(&self) -> &[u8] { + &self.bytes + } +} + /// Write `response` to `stream` as a complete HTTP/1.1 response. /// /// # Errors @@ -59,6 +94,18 @@ pub(super) fn render_response(response: &HttpResponse) -> String { ) } +/// Write `response`'s bytes to `stream` verbatim. +/// +/// # Errors +/// +/// Returns an error when the bytes cannot be written to the stream. +pub(super) fn write_raw_response( + stream: &mut TcpStream, + response: &RawHttpResponse, +) -> io::Result<()> { + stream.write_all(&response.bytes) +} + /// Return the standard reason phrase for fixture status codes. const fn reason_phrase(status: u16) -> &'static str { match status { diff --git a/test_support/src/http/server.rs b/test_support/src/http/server.rs index 0206dca33..57d03d1ac 100644 --- a/test_support/src/http/server.rs +++ b/test_support/src/http/server.rs @@ -1,13 +1,13 @@ //! Request-serving implementation for the local HTTP fixture. use std::{ - net::{TcpListener, TcpStream}, + net::{Shutdown, SocketAddr, TcpListener, TcpStream}, sync::atomic::{AtomicBool, AtomicUsize, Ordering}, }; use super::{ - HttpResponse, HttpServerConfig, RequestLog, accept_connection, request::read_request_line, - response, + HttpResponse, HttpServerConfig, RawHttpResponse, RequestLog, accept_connection, + request::read_request_line, response, }; /// What one fixture run records about the requests it answers, and the state it @@ -52,28 +52,98 @@ enum FixtureProgress { Shutdown, } -/// Serve the configured responses in request order until one is not requested. -pub(super) fn run_http_server( - listener: &TcpListener, - responses: &[HttpResponse], - config: &HttpServerConfig, - ledger: &FixtureLedger<'_>, -) { - for response in responses { - if serve_fixture_response(listener, response, config, ledger) == FixtureProgress::Shutdown { - return; +/// What a fixture emits, and how it marks the end of what it emits. +/// +/// This is the whole of the difference between the two public shapes the +/// fixture serves: a valid structured response, and bytes the caller supplies. +/// Both are driven by the one loop below, so each shape inherits the same +/// bounded accept, the same bounded request read, the same accounting, and the +/// same shutdown behaviour, rather than reimplementing them. +/// +/// Implementors belong only to the local HTTP fixture, and are moved into the +/// thread that drives them; no production call site may depend on the +/// panic-oriented test failure contract. +pub(super) trait DriveStrategy { + /// Serve the sequence in request order until one response is not requested. + fn drive(&self, listener: &TcpListener, config: &HttpServerConfig, ledger: &FixtureLedger<'_>); + + /// Form the fixture URL for a bound loopback address. + fn advertise(&self, addr: SocketAddr) -> String; +} + +/// Serve `responses` in request order, as valid structured HTTP responses. +#[derive(Debug)] +pub(super) struct StructuredResponses { + /// The responses to emit, in arrival order. + responses: Vec, +} + +impl StructuredResponses { + /// Serve `responses` in request order. + #[must_use] + pub(super) const fn new(responses: Vec) -> Self { + Self { responses } + } +} + +impl DriveStrategy for StructuredResponses { + fn drive(&self, listener: &TcpListener, config: &HttpServerConfig, ledger: &FixtureLedger<'_>) { + for response in &self.responses { + if serve_fixture_response(listener, response, config, ledger) + == FixtureProgress::Shutdown + { + return; + } + } + } + + fn advertise(&self, addr: SocketAddr) -> String { + format!("http://{addr}") + } +} + +/// Serve `responses` in request order, as raw bytes. +/// +/// The advertised URL carries the credentials the structured shape omits. A +/// test of a credentialed fetch needs userinfo in the URL it drives, and this +/// is the shape such a test reaches for, so the fixture supplies it rather +/// than making every caller paste the same userinfo into its own request. +#[derive(Debug)] +pub(super) struct RawResponses { + /// The responses to emit, in arrival order. + responses: Vec, +} + +impl RawResponses { + /// Serve `responses` in request order. + #[must_use] + pub(super) const fn new(responses: Vec) -> Self { + Self { responses } + } +} + +impl DriveStrategy for RawResponses { + fn drive(&self, listener: &TcpListener, config: &HttpServerConfig, ledger: &FixtureLedger<'_>) { + for response in &self.responses { + if serve_raw_response(listener, response, config, ledger) == FixtureProgress::Shutdown { + return; + } } } + + fn advertise(&self, addr: SocketAddr) -> String { + format!("http://redirect-user:redirect-secret@{addr}") + } } -/// Serve one fixture response after a client sends a non-empty request. +/// Serve one structured fixture response after a client sends a request. /// /// Returns [`FixtureProgress::Shutdown`] when the client disconnects before /// sending a request, so an abandoned chain cannot leave later responses /// waiting on a connection that will never arrive, and likewise when the test /// shuts the fixture down before any client connects. /// -/// This helper belongs only to the local HTTP fixture: `run_http_server` +/// This helper belongs only to the local HTTP fixture: the drive loop /// composes it once for every configured response, and no production call site /// may depend on its panic-oriented test failure contract. #[must_use] @@ -97,6 +167,46 @@ fn serve_fixture_response( FixtureProgress::Continue } +/// Serve one raw fixture response after a client sends a request. +/// +/// Returns [`FixtureProgress::Shutdown`] on the same two conditions as +/// [`serve_fixture_response`], reached by the same two helpers, so the two +/// shapes differ only in what they write. +/// +/// The request is read to a non-empty line *before* the bytes go out, and the +/// write half is then shut down. Both matter, and both exist to keep a +/// transport abort from being mistaken for a protocol verdict: a fixture that +/// closes a socket without draining the request can have its close turned into +/// a connection abort by the peer's stack — Windows does exactly this — which +/// the client reports as an I/O failure before it has parsed a status line no +/// parser accepts. The test asserting a parse failure would then be asserting +/// whatever the platform's close semantics happened to do. Draining first +/// removes the unread data, and the half-close tells the client the response +/// ended while leaving the read path intact, so the bytes are received whole +/// and parsed. A well-formed status line sent this way still arrives; only a +/// malformed one fails, which is the failure under test. +#[must_use] +fn serve_raw_response( + listener: &TcpListener, + response: &RawHttpResponse, + config: &HttpServerConfig, + ledger: &FixtureLedger<'_>, +) -> FixtureProgress { + let Some(mut stream) = accept_fixture_connection(listener, config, ledger.shutdown) else { + return FixtureProgress::Shutdown; + }; + configure_fixture_stream(&stream); + let Some(line) = read_request_line(&mut stream, config.read_deadline(), config.poll_interval) + else { + return FixtureProgress::Shutdown; + }; + ledger.log.record(line); + ledger.requests.fetch_add(1, Ordering::Relaxed); + write_raw_response(&mut stream, response); + finish_raw_response(&stream); + FixtureProgress::Continue +} + /// Accept one client connection using the fixture configuration. /// /// Returns `None` once `shutdown` is set, which ends the run on the signal @@ -135,3 +245,31 @@ fn write_fixture_response(stream: &mut TcpStream, response: &HttpResponse) { panic!("failed to write fixture response: {err}"); } } + +/// Write one raw response to a fixture client stream. +#[expect( + clippy::panic, + reason = "test HTTP helper should fail fast when response writing fails" +)] +fn write_raw_response(stream: &mut TcpStream, response: &RawHttpResponse) { + if let Err(err) = response::write_raw_response(stream, response) { + panic!("failed to write raw fixture response: {err}"); + } +} + +/// Mark the end of a raw response by shutting down the stream's write half. +/// +/// A shutdown, not a close: the read half stays open, so the client still has +/// a usable socket to parse from. See [`serve_raw_response`] for why the +/// fixture needs this at all, and why a failure here is fatal rather than +/// ignored — a fixture that did not half-close is the only other reason the +/// client could see a short response, and it should fail loudly. +#[expect( + clippy::panic, + reason = "test HTTP helper should fail fast when the response cannot be framed" +)] +fn finish_raw_response(stream: &TcpStream) { + if let Err(err) = stream.shutdown(Shutdown::Write) { + panic!("failed to shut down the raw fixture response: {err}"); + } +} diff --git a/test_support/src/http/tests.rs b/test_support/src/http/tests.rs index 75cf57a81..75fad38e8 100644 --- a/test_support/src/http/tests.rs +++ b/test_support/src/http/tests.rs @@ -6,7 +6,8 @@ //! module. use super::{ - AcceptWait, HttpResponse, HttpServerConfig, accept_connection, response::render_response, + AcceptWait, HttpResponse, HttpServerConfig, RawHttpResponse, accept_connection, + response::render_response, }; use std::{ @@ -56,10 +57,7 @@ fn response_server_counts_each_client_request() -> anyhow::Result<()> { /// Send one minimal HTTP request to the fixture at `url`. fn send_request(url: &str) -> anyhow::Result<()> { - let address = url - .strip_prefix("http://") - .ok_or_else(|| anyhow::anyhow!("fixture URL must use HTTP: {url}"))?; - let mut stream = TcpStream::connect(address)?; + let mut stream = connect_fixture(url)?; stream.write_all(b"GET / HTTP/1.1\r\nHost: fixture\r\nConnection: close\r\n\r\n")?; let mut response = String::new(); stream.read_to_string(&mut response)?; @@ -70,6 +68,21 @@ fn send_request(url: &str) -> anyhow::Result<()> { Ok(()) } +/// Connect to the loopback address in a fixture `url`. +/// +/// The raw fixture advertises a URL carrying credentials, which a client must +/// not send as part of the authority, so they are stripped here. The +/// structured fixture advertises none, and takes the same path. +fn connect_fixture(url: &str) -> anyhow::Result { + let authority = url + .strip_prefix("http://") + .ok_or_else(|| anyhow::anyhow!("fixture URL must use HTTP: {url}"))?; + let address = authority + .rsplit_once('@') + .map_or(authority, |(_userinfo, host)| host); + Ok(TcpStream::connect(address)?) +} + #[test] fn accept_connection_respects_accept_timeout() -> anyhow::Result<()> { let listener = TcpListener::bind(("127.0.0.1", 0))?; @@ -209,6 +222,57 @@ fn expect_no_requests_fixture_waits_past_the_accept_timeout() -> anyhow::Result< Ok(()) } +/// A raw response reaches the client byte for byte, framed by a half-close. +/// +/// This is the fixture contract the redirect error tests rest on, and it is +/// asserted here rather than only through those tests because the failure it +/// guards against is silent: a fixture that wrote the bytes and then closed +/// the socket outright could still pass a client-side test on Linux, while +/// aborting the connection on Windows. Reading to EOF is what proves the +/// response was framed — the read terminates on the shutdown, not on a reset — +/// and the exact-byte comparison is what proves nothing was rewritten in +/// flight. The raw bytes are deliberately something no real server should +/// send, because that is the only input the fixture exists to serve. +#[test] +fn raw_response_fixture_delivers_the_exact_bytes_it_was_given() -> anyhow::Result<()> { + let malformed = b"HTTP/1.1 banana OK\r\nContent-Length: 0\r\n\r\n"; + let (url, log, server) = + super::spawn_http_server_raw_response(RawHttpResponse::new(malformed))?; + anyhow::ensure!( + url.contains("redirect-user:redirect-secret@"), + "the raw fixture URL should carry credentials: {url}", + ); + + let received = send_request_and_read_to_eof(&url)?; + server + .join() + .map_err(|err| anyhow::anyhow!("fixture server panicked: {err:?}"))?; + + anyhow::ensure!( + received == malformed, + "the fixture should deliver the bytes it was given, got {:?}", + String::from_utf8_lossy(&received), + ); + anyhow::ensure!( + log.lines() == vec!["GET / HTTP/1.1".to_owned()], + "the raw fixture should record the request it answered: {:?}", + log.lines(), + ); + Ok(()) +} + +/// Send one minimal HTTP request and read the response back to EOF. +/// +/// Unlike [`send_request`], this returns the bytes rather than asserting a +/// status line, because the caller is testing responses that have none. +fn send_request_and_read_to_eof(url: &str) -> anyhow::Result> { + let mut stream = connect_fixture(url)?; + stream.write_all(b"GET / HTTP/1.1\r\nHost: fixture\r\nConnection: close\r\n\r\n")?; + let mut received = Vec::new(); + stream.read_to_end(&mut received)?; + Ok(received) +} + /// The no-request fixture still records a request it receives. /// /// Without this, an empty log would also hold for a fixture that never records From ceb7ce7393a210b6f44103772b5e6927af269751 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 19 Sep 2026 15:53:04 +0200 Subject: [PATCH 2/3] Split fixture env and spawn helper out of the http module Whitaker's `module_max_lines` caps a module at 400 lines, and adding the raw-response entry points took `test_support/src/http/mod.rs` to 459. Rather than widen the cap, the two seams the module already had are now their own files: `env.rs` owns the timeout overrides and the redaction rule that keeps a caller-supplied value out of the log, and `spawn.rs` owns binding, accounting, and thread ownership for every fixture shape. `mod.rs` keeps the public API and the configuration type, and is now 331 lines. The split is by responsibility, not by line count: each new module is named for the single job it does, and its `//!` header says which. --- test_support/src/http/config_tests.rs | 3 +- test_support/src/http/env.rs | 77 ++++++++++++++ test_support/src/http/mod.rs | 140 ++------------------------ test_support/src/http/spawn.rs | 94 +++++++++++++++++ 4 files changed, 179 insertions(+), 135 deletions(-) create mode 100644 test_support/src/http/env.rs create mode 100644 test_support/src/http/spawn.rs diff --git a/test_support/src/http/config_tests.rs b/test_support/src/http/config_tests.rs index 59d3a9389..ef8fc9230 100644 --- a/test_support/src/http/config_tests.rs +++ b/test_support/src/http/config_tests.rs @@ -6,7 +6,8 @@ use super::{ ENV_HTTP_ACCEPT_TIMEOUT_MS, ENV_HTTP_POLL_INTERVAL_MS, ENV_HTTP_READ_TIMEOUT_MS, - HttpServerConfig, duration_from_env, take_duration_warnings, + HttpServerConfig, + env::{duration_from_env, take_duration_warnings}, }; use mockable::MockEnv; diff --git a/test_support/src/http/env.rs b/test_support/src/http/env.rs new file mode 100644 index 000000000..03199ca2a --- /dev/null +++ b/test_support/src/http/env.rs @@ -0,0 +1,77 @@ +//! Environment overrides for the fixture's timeouts. +//! +//! The fixture's deadlines are tunable so a slow machine can be given longer +//! than a default that suits a fast one. An override arrives as a string, so +//! this module owns the parsing, the fallback when it does not parse, and the +//! redaction that keeps a caller-supplied value out of the log. + +use std::{fmt, time::Duration}; + +use mockable::Env; + +#[cfg(test)] +use std::{cell::RefCell, thread_local}; + +#[cfg(test)] +thread_local! { + /// Warnings recorded for assertions by the fixture's own tests. + /// + /// A `tracing` subscriber would be the production path, but a unit test + /// cannot observe one without a global subscriber, so the test build + /// captures into a thread-local instead. + pub(super) static DURATION_WARNINGS: RefCell> = + const { RefCell::new(Vec::new()) }; +} + +/// Read `var` as whole milliseconds, falling back to `default` when unset or +/// unparsable. +pub(super) fn duration_from_env(env: &impl Env, var: &str, default: Duration) -> Duration { + env.raw(var).map_or(default, |value| { + let trimmed = value.trim(); + match trimmed.parse::() { + Ok(ms) => Duration::from_millis(ms), + Err(err) => { + log_duration_parse_error(var, trimmed.len(), &err); + default + } + } + }) +} + +/// Report an unparsable duration override without echoing its value. +/// +/// The value is redacted: an environment variable's contents are outside this +/// crate's control, and logging them verbatim would put whatever the caller +/// exported into the log. `err` already names the bounded parse failure, and +/// `value_len` distinguishes an empty override from a malformed one, which is +/// all the diagnosis this fixture needs. +fn log_duration_parse_error(var: &str, value_len: usize, err: &dyn fmt::Display) { + #[cfg(test)] + { + record_duration_warning(format!( + "ignoring invalid {var}: {err} (value redacted, {value_len} bytes)" + )); + } + + #[cfg(not(test))] + { + tracing::warn!( + variable = var, + value_len, + error = %err, + "ignoring invalid fixture duration" + ); + } +} + +/// Record a warning for the fixture's own tests to assert on. +#[cfg(test)] +fn record_duration_warning(message: String) { + DURATION_WARNINGS.with(|warnings| warnings.borrow_mut().push(message)); +} + +/// Take the warnings recorded so far on this thread, leaving the buffer empty. +#[cfg(test)] +pub(super) fn take_duration_warnings() -> Vec { + DURATION_WARNINGS.with(|warnings| warnings.borrow_mut().drain(..).collect()) +} diff --git a/test_support/src/http/mod.rs b/test_support/src/http/mod.rs index 42d57e9e3..e6585c235 100644 --- a/test_support/src/http/mod.rs +++ b/test_support/src/http/mod.rs @@ -6,8 +6,8 @@ use mockable::{DefaultEnv, Env}; use std::{ - fmt, io, - net::{SocketAddr, TcpListener, TcpStream}, + io, + net::{SocketAddr, TcpStream}, sync::{ Arc, atomic::{AtomicBool, AtomicUsize, Ordering}, @@ -17,14 +17,17 @@ use std::{ }; mod accept; +mod env; mod request; mod response; mod server; +mod spawn; use self::accept::{AcceptWait, accept_connection}; +use self::env::duration_from_env; pub use self::request::RequestLog; pub use self::response::{HttpResponse, RawHttpResponse}; -use self::server::{DriveStrategy, FixtureLedger, RawResponses, StructuredResponses}; +use self::spawn::{spawn_fixture_server, spawn_raw_fixture_server}; /// Override for the timeout in milliseconds within which a client must connect. pub(crate) const ENV_HTTP_ACCEPT_TIMEOUT_MS: &str = "NETSUKE_TEST_HTTP_ACCEPT_TIMEOUT_MS"; @@ -33,14 +36,6 @@ pub(crate) const ENV_HTTP_READ_TIMEOUT_MS: &str = "NETSUKE_TEST_HTTP_READ_TIMEOU /// Override for the polling interval in milliseconds used while waiting. pub(crate) const ENV_HTTP_POLL_INTERVAL_MS: &str = "NETSUKE_TEST_HTTP_POLL_INTERVAL_MS"; -#[cfg(test)] -use std::{cell::RefCell, thread_local}; - -#[cfg(test)] -thread_local! { - static DURATION_WARNINGS: RefCell> = const { RefCell::new(Vec::new()) }; -} - /// Configuration for HTTP fixtures, including timeouts used during polling. #[derive(Debug, Clone)] pub struct HttpServerConfig { @@ -330,129 +325,6 @@ pub fn spawn_http_server_raw_response( spawn_http_server_raw_responses([response]) } -/// Spawn an HTTP server using `config`, emitting responses in sequence. -/// -/// Returns the bound URL, the shared request count, the request log, and the -/// server handle. The public wrappers above reshape this tuple for their -/// callers, so every fixture shares one server implementation. -fn spawn_fixture_server( - responses: impl IntoIterator, - config: HttpServerConfig, -) -> io::Result<(String, Arc, RequestLog, HttpServer)> { - let response_sequence = responses.into_iter().collect::>(); - spawn_fixture_thread( - Box::new(StructuredResponses::new(response_sequence)), - config, - ) -} - -/// Spawn an HTTP server using `config`, emitting raw responses in sequence. -/// -/// The raw counterpart of [`spawn_fixture_server`], and the only place the two -/// shapes diverge: both hand a [`DriveStrategy`] to -/// [`spawn_fixture_thread`], which owns binding, accounting, and the thread. -fn spawn_raw_fixture_server( - responses: impl IntoIterator, - config: HttpServerConfig, -) -> io::Result<(String, Arc, RequestLog, HttpServer)> { - let response_sequence = responses.into_iter().collect::>(); - spawn_fixture_thread(Box::new(RawResponses::new(response_sequence)), config) -} - -/// Serve `strategy` on a fresh listener, returning its URL and shared state. -/// -/// This is the one implementation behind every fixture the module exposes. A -/// [`DriveStrategy`] supplies what to emit and how to advertise it; everything -/// else — binding a loopback port, the request counter and log shared with the -/// caller, the shutdown flag, the named thread, and the handle that joins or -/// signals them — is common, so no fixture shape can drift from another's -/// acceptance, accounting, or shutdown behaviour. -fn spawn_fixture_thread( - strategy: Box, - config: HttpServerConfig, -) -> io::Result<(String, Arc, RequestLog, HttpServer)> { - let listener = TcpListener::bind(("127.0.0.1", 0))?; - listener.set_nonblocking(true)?; - let addr = listener.local_addr()?; - let url = strategy.advertise(addr); - let requests = Arc::new(AtomicUsize::new(0)); - let server_requests = Arc::clone(&requests); - let log = RequestLog::default(); - let server_log = log.clone(); - let shutdown = Arc::new(AtomicBool::new(false)); - let server_shutdown = Arc::clone(&shutdown); - let handle = thread::Builder::new() - .name("netsuke-http-fixture".into()) - .spawn(move || { - strategy.drive( - &listener, - &config, - &FixtureLedger::new(&server_requests, &server_log, &server_shutdown), - ); - })?; - Ok(( - url, - requests, - log, - HttpServer { - handle: Some(handle), - addr, - shutdown, - }, - )) -} - -/// Read `var` as whole milliseconds, falling back to `default` when unset or -/// unparsable. -fn duration_from_env(env: &impl Env, var: &str, default: Duration) -> Duration { - env.raw(var).map_or(default, |value| { - let trimmed = value.trim(); - match trimmed.parse::() { - Ok(ms) => Duration::from_millis(ms), - Err(err) => { - log_duration_parse_error(var, trimmed.len(), &err); - default - } - } - }) -} - -/// Report an unparsable duration override without echoing its value. -/// -/// The value is redacted: an environment variable's contents are outside this -/// crate's control, and logging them verbatim would put whatever the caller -/// exported into the log. `err` already names the bounded parse failure, and -/// `value_len` distinguishes an empty override from a malformed one, which is -/// all the diagnosis this fixture needs. -fn log_duration_parse_error(var: &str, value_len: usize, err: &dyn fmt::Display) { - #[cfg(test)] - { - record_duration_warning(format!( - "ignoring invalid {var}: {err} (value redacted, {value_len} bytes)" - )); - } - - #[cfg(not(test))] - { - tracing::warn!( - variable = var, - value_len, - error = %err, - "ignoring invalid fixture duration" - ); - } -} - -#[cfg(test)] -fn record_duration_warning(message: String) { - DURATION_WARNINGS.with(|warnings| warnings.borrow_mut().push(message)); -} - -#[cfg(test)] -fn take_duration_warnings() -> Vec { - DURATION_WARNINGS.with(|warnings| warnings.borrow_mut().drain(..).collect()) -} - #[cfg(test)] mod config_tests; #[cfg(test)] diff --git a/test_support/src/http/spawn.rs b/test_support/src/http/spawn.rs new file mode 100644 index 000000000..db1ab51cb --- /dev/null +++ b/test_support/src/http/spawn.rs @@ -0,0 +1,94 @@ +//! Listener setup and thread ownership for the local HTTP fixture. +//! +//! Every fixture the module exposes is built here, and every one of them is +//! built the same way: bind a loopback port, share a request counter, a request +//! log, and a shutdown flag with the caller, then move a +//! [`DriveStrategy`] into a named thread that serves the responses. What the +//! fixture emits is the only thing a caller varies. + +use std::{ + io, + net::TcpListener, + sync::{ + Arc, + atomic::{AtomicBool, AtomicUsize}, + }, + thread, +}; + +use super::{ + HttpResponse, HttpServer, HttpServerConfig, RawHttpResponse, RequestLog, + server::{DriveStrategy, FixtureLedger, RawResponses, StructuredResponses}, +}; + +/// Spawn an HTTP server using `config`, emitting responses in sequence. +/// +/// Returns the bound URL, the shared request count, the request log, and the +/// server handle. The public wrappers in the parent module reshape this tuple +/// for their callers, so every fixture shares one server implementation. +pub(super) fn spawn_fixture_server( + responses: impl IntoIterator, + config: HttpServerConfig, +) -> io::Result<(String, Arc, RequestLog, HttpServer)> { + let response_sequence = responses.into_iter().collect::>(); + spawn_fixture_thread( + Box::new(StructuredResponses::new(response_sequence)), + config, + ) +} + +/// Spawn an HTTP server using `config`, emitting raw responses in sequence. +/// +/// The raw counterpart of [`spawn_fixture_server`], and the only place the two +/// shapes diverge: both hand a [`DriveStrategy`] to [`spawn_fixture_thread`], +/// which owns binding, accounting, and the thread. +pub(super) fn spawn_raw_fixture_server( + responses: impl IntoIterator, + config: HttpServerConfig, +) -> io::Result<(String, Arc, RequestLog, HttpServer)> { + let response_sequence = responses.into_iter().collect::>(); + spawn_fixture_thread(Box::new(RawResponses::new(response_sequence)), config) +} + +/// Serve `strategy` on a fresh listener, returning its URL and shared state. +/// +/// This is the one implementation behind every fixture the module exposes. A +/// [`DriveStrategy`] supplies what to emit and how to advertise it; everything +/// else — binding a loopback port, the request counter and log shared with the +/// caller, the shutdown flag, the named thread, and the handle that joins or +/// signals them — is common, so no fixture shape can drift from another's +/// acceptance, accounting, or shutdown behaviour. +fn spawn_fixture_thread( + strategy: Box, + config: HttpServerConfig, +) -> io::Result<(String, Arc, RequestLog, HttpServer)> { + let listener = TcpListener::bind(("127.0.0.1", 0))?; + listener.set_nonblocking(true)?; + let addr = listener.local_addr()?; + let url = strategy.advertise(addr); + let requests = Arc::new(AtomicUsize::new(0)); + let server_requests = Arc::clone(&requests); + let log = RequestLog::default(); + let server_log = log.clone(); + let shutdown = Arc::new(AtomicBool::new(false)); + let server_shutdown = Arc::clone(&shutdown); + let handle = thread::Builder::new() + .name("netsuke-http-fixture".into()) + .spawn(move || { + strategy.drive( + &listener, + &config, + &FixtureLedger::new(&server_requests, &server_log, &server_shutdown), + ); + })?; + Ok(( + url, + requests, + log, + HttpServer { + handle: Some(handle), + addr, + shutdown, + }, + )) +} From 2caef4a3bf04adbe065ca07e1dc6575e43376e91 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 19 Sep 2026 18:18:56 +0200 Subject: [PATCH 3/3] Share one drive loop between fixture response shapes CodeScene flagged the two `DriveStrategy::drive` implementations as duplication: both ran the same accept, request-read, accounting, and shutdown sequence, differing only in the bytes written. The trait's own doc comment claimed "the one loop below" while there were two. Replace `DriveStrategy`, `StructuredResponses`, and `RawResponses` with a `FixtureServe` trait whose `drive` is a provided method, and one `FixtureResponses` enum that dispatches `serve_one` between the shapes. The loop now exists once. `accept_request` returns the accepted stream together with the sequence index, taken from the request log's length, so the per-shape methods no longer index an array themselves. `finish_raw_response` now tolerates a departed peer. A probe of the mechanism shows `shutdown(SHUT_WR)` succeeds after a peer FIN but fails ENOTCONN after a peer RST, and a client that reset the connection leaves nothing to half-close. Panicking there turned the client's own departure into a fixture failure -- the same conflation of transport outcome with protocol verdict this fixture exists to remove. NotConnected, ConnectionReset, and ConnectionAborted are ignored; anything else still fails, and BrokenPipe is deliberately excluded because a peer that closed only its read half is still there to be answered. Pin that decision with a test over the predicate, rather than a sleep-timed live reset that would reintroduce the race the fixture removes. --- test_support/src/http/server.rs | 278 ++++++++++++++++++-------------- test_support/src/http/spawn.rs | 14 +- test_support/src/http/tests.rs | 36 +++++ 3 files changed, 196 insertions(+), 132 deletions(-) diff --git a/test_support/src/http/server.rs b/test_support/src/http/server.rs index 57d03d1ac..19245883e 100644 --- a/test_support/src/http/server.rs +++ b/test_support/src/http/server.rs @@ -10,6 +10,13 @@ use super::{ request::read_request_line, response, }; +/// Credentials the raw fixture advertises, and the structured one omits. +/// +/// A test of a credentialed fetch needs userinfo in the URL it drives, and the +/// raw shape is the one such a test reaches for, so the fixture supplies it +/// rather than making every caller paste the same userinfo into its own request. +const RAW_FIXTURE_USERINFO: &str = "redirect-user:redirect-secret"; + /// What one fixture run records about the requests it answers, and the state it /// shares with the test that owns it. /// @@ -44,7 +51,7 @@ impl<'run> FixtureLedger<'run> { /// Report whether the fixture should keep serving configured responses. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum FixtureProgress { +pub(super) enum FixtureProgress { /// The client sent a request and the next response may be served. Continue, /// No further connection is accepted, because either the client @@ -52,159 +59,157 @@ enum FixtureProgress { Shutdown, } -/// What a fixture emits, and how it marks the end of what it emits. +/// One accepted client connection, and the response it is owed. +/// +/// The stream and the index travel together because the index is only known +/// once the request has been read: a client that connects and sends nothing +/// must not advance the sequence, so the two cannot be carried separately +/// without inviting a response for a request that never arrived. +#[derive(Debug)] +struct FixtureRequest { + /// The accepted client connection. + stream: TcpStream, + /// Zero-based index, within the configured sequence, of the response owed. + request: usize, +} + +/// What a fixture emits, and how it is driven. /// -/// This is the whole of the difference between the two public shapes the -/// fixture serves: a valid structured response, and bytes the caller supplies. -/// Both are driven by the one loop below, so each shape inherits the same -/// bounded accept, the same bounded request read, the same accounting, and the -/// same shutdown behaviour, rather than reimplementing them. +/// This is the whole of the difference between the two shapes the fixture +/// serves: a valid structured response, and bytes the caller supplies. Both are +/// driven by the one loop in [`FixtureServe::drive`], so each shape inherits +/// the same bounded accept, the same bounded request read, the same accounting, +/// and the same shutdown behaviour, rather than reimplementing them. /// /// Implementors belong only to the local HTTP fixture, and are moved into the /// thread that drives them; no production call site may depend on the /// panic-oriented test failure contract. -pub(super) trait DriveStrategy { - /// Serve the sequence in request order until one response is not requested. - fn drive(&self, listener: &TcpListener, config: &HttpServerConfig, ledger: &FixtureLedger<'_>); +pub(super) trait FixtureServe { + /// Serve one response after a client sends a request. + #[must_use] + fn serve_one( + &self, + listener: &TcpListener, + config: &HttpServerConfig, + ledger: &FixtureLedger<'_>, + ) -> FixtureProgress; /// Form the fixture URL for a bound loopback address. - fn advertise(&self, addr: SocketAddr) -> String; -} - -/// Serve `responses` in request order, as valid structured HTTP responses. -#[derive(Debug)] -pub(super) struct StructuredResponses { - /// The responses to emit, in arrival order. - responses: Vec, -} - -impl StructuredResponses { - /// Serve `responses` in request order. #[must_use] - pub(super) const fn new(responses: Vec) -> Self { - Self { responses } - } -} + fn advertise(&self, addr: SocketAddr) -> String; -impl DriveStrategy for StructuredResponses { + /// Serve responses in request order until one response is not requested. + /// + /// The single drive loop behind every fixture shape the module exposes. + /// It is defined here, once, rather than in each implementor, so a new + /// shape cannot arrive with its own copy of the run's control flow. fn drive(&self, listener: &TcpListener, config: &HttpServerConfig, ledger: &FixtureLedger<'_>) { - for response in &self.responses { - if serve_fixture_response(listener, response, config, ledger) - == FixtureProgress::Shutdown - { + loop { + if self.serve_one(listener, config, ledger) == FixtureProgress::Shutdown { return; } } } - - fn advertise(&self, addr: SocketAddr) -> String { - format!("http://{addr}") - } } -/// Serve `responses` in request order, as raw bytes. +/// The response shape a fixture emits, and the URL it advertises. /// -/// The advertised URL carries the credentials the structured shape omits. A -/// test of a credentialed fetch needs userinfo in the URL it drives, and this -/// is the shape such a test reaches for, so the fixture supplies it rather -/// than making every caller paste the same userinfo into its own request. +/// The shapes differ in exactly two ways — the bytes written, and whether the +/// advertised URL carries credentials — so both are one enum, and the one +/// [`FixtureServe`] implementation below dispatches between them. #[derive(Debug)] -pub(super) struct RawResponses { - /// The responses to emit, in arrival order. - responses: Vec, +pub(super) enum FixtureResponses { + /// Valid structured responses, advertised without credentials. + Structured(Vec), + /// Bytes written to the client verbatim, advertised with credentials. + Raw(Vec), } -impl RawResponses { - /// Serve `responses` in request order. +impl FixtureResponses { + /// Emit `responses` in request order as valid structured HTTP responses. #[must_use] - pub(super) const fn new(responses: Vec) -> Self { - Self { responses } + pub(super) const fn structured(responses: Vec) -> Self { + Self::Structured(responses) + } + + /// Emit `responses` in request order as raw bytes. + #[must_use] + pub(super) const fn raw(responses: Vec) -> Self { + Self::Raw(responses) } } -impl DriveStrategy for RawResponses { - fn drive(&self, listener: &TcpListener, config: &HttpServerConfig, ledger: &FixtureLedger<'_>) { - for response in &self.responses { - if serve_raw_response(listener, response, config, ledger) == FixtureProgress::Shutdown { - return; - } +impl FixtureServe for FixtureResponses { + fn serve_one( + &self, + listener: &TcpListener, + config: &HttpServerConfig, + ledger: &FixtureLedger<'_>, + ) -> FixtureProgress { + let Some(mut request) = accept_request(listener, config, ledger) else { + return FixtureProgress::Shutdown; + }; + match self { + Self::Structured(responses) => match responses.get(request.request) { + Some(response) => write_fixture_response(&mut request.stream, response), + // The sequence holds fewer responses than the requests made + // against it, so there is nothing left to send. Ending the run + // keeps the caller's log honest about how far the exchange got. + None => return FixtureProgress::Shutdown, + }, + Self::Raw(responses) => match responses.get(request.request) { + Some(response) => { + write_raw_response(&mut request.stream, response); + finish_raw_response(&request.stream); + } + None => return FixtureProgress::Shutdown, + }, } + FixtureProgress::Continue } fn advertise(&self, addr: SocketAddr) -> String { - format!("http://redirect-user:redirect-secret@{addr}") + match self { + Self::Structured(_) => format!("http://{addr}"), + Self::Raw(_) => format!("http://{RAW_FIXTURE_USERINFO}@{addr}"), + } } } -/// Serve one structured fixture response after a client sends a request. -/// -/// Returns [`FixtureProgress::Shutdown`] when the client disconnects before -/// sending a request, so an abandoned chain cannot leave later responses -/// waiting on a connection that will never arrive, and likewise when the test -/// shuts the fixture down before any client connects. -/// -/// This helper belongs only to the local HTTP fixture: the drive loop -/// composes it once for every configured response, and no production call site -/// may depend on its panic-oriented test failure contract. -#[must_use] -fn serve_fixture_response( - listener: &TcpListener, - response: &HttpResponse, - config: &HttpServerConfig, - ledger: &FixtureLedger<'_>, -) -> FixtureProgress { - let Some(mut stream) = accept_fixture_connection(listener, config, ledger.shutdown) else { - return FixtureProgress::Shutdown; - }; - configure_fixture_stream(&stream); - let Some(line) = read_request_line(&mut stream, config.read_deadline(), config.poll_interval) - else { - return FixtureProgress::Shutdown; - }; - ledger.log.record(line); - ledger.requests.fetch_add(1, Ordering::Relaxed); - write_fixture_response(&mut stream, response); - FixtureProgress::Continue -} - -/// Serve one raw fixture response after a client sends a request. +/// Accept one client connection and read the request it sent. /// -/// Returns [`FixtureProgress::Shutdown`] on the same two conditions as -/// [`serve_fixture_response`], reached by the same two helpers, so the two -/// shapes differ only in what they write. +/// Returns `None` on the same two conditions as the helpers it composes: no +/// client connected before the test shut the fixture down, or the client +/// disconnected before sending a request. The latter is how an abandoned chain +/// is detected, so a later response cannot wait on a connection that will never +/// arrive. /// -/// The request is read to a non-empty line *before* the bytes go out, and the -/// write half is then shut down. Both matter, and both exist to keep a -/// transport abort from being mistaken for a protocol verdict: a fixture that -/// closes a socket without draining the request can have its close turned into -/// a connection abort by the peer's stack — Windows does exactly this — which -/// the client reports as an I/O failure before it has parsed a status line no -/// parser accepts. The test asserting a parse failure would then be asserting -/// whatever the platform's close semantics happened to do. Draining first -/// removes the unread data, and the half-close tells the client the response -/// ended while leaving the read path intact, so the bytes are received whole -/// and parsed. A well-formed status line sent this way still arrives; only a -/// malformed one fails, which is the failure under test. -#[must_use] -fn serve_raw_response( +/// The request is read to a non-empty line *before* any response goes out. That +/// ordering matters, and exists to keep a transport abort from being mistaken +/// for a protocol verdict: a fixture that closes a socket without draining the +/// request can have its close turned into a connection abort by the peer's +/// stack — Windows does exactly this — which the client reports as an I/O +/// failure before it has parsed a status line no parser accepts. The test +/// asserting a parse failure would then be asserting whatever the platform's +/// close semantics happened to do. Draining first removes the unread data, so +/// the bytes are received whole and parsed. A well-formed status line sent this +/// way still arrives; only a malformed one fails, which is the failure under +/// test. +fn accept_request( listener: &TcpListener, - response: &RawHttpResponse, config: &HttpServerConfig, ledger: &FixtureLedger<'_>, -) -> FixtureProgress { - let Some(mut stream) = accept_fixture_connection(listener, config, ledger.shutdown) else { - return FixtureProgress::Shutdown; - }; +) -> Option { + let mut stream = accept_fixture_connection(listener, config, ledger.shutdown)?; configure_fixture_stream(&stream); - let Some(line) = read_request_line(&mut stream, config.read_deadline(), config.poll_interval) - else { - return FixtureProgress::Shutdown; - }; + let line = read_request_line(&mut stream, config.read_deadline(), config.poll_interval)?; + // Exactly one request line is recorded per accepted connection, and the + // fixture serves one connection at a time, so the log's length before this + // line is recorded is the index of the response this request is owed. + let request = ledger.log.len(); ledger.log.record(line); ledger.requests.fetch_add(1, Ordering::Relaxed); - write_raw_response(&mut stream, response); - finish_raw_response(&stream); - FixtureProgress::Continue + Some(FixtureRequest { stream, request }) } /// Accept one client connection using the fixture configuration. @@ -259,17 +264,40 @@ fn write_raw_response(stream: &mut TcpStream, response: &RawHttpResponse) { /// Mark the end of a raw response by shutting down the stream's write half. /// -/// A shutdown, not a close: the read half stays open, so the client still has -/// a usable socket to parse from. See [`serve_raw_response`] for why the -/// fixture needs this at all, and why a failure here is fatal rather than -/// ignored — a fixture that did not half-close is the only other reason the -/// client could see a short response, and it should fail loudly. -#[expect( - clippy::panic, - reason = "test HTTP helper should fail fast when the response cannot be framed" -)] +/// A shutdown, not a close: the read half stays open, so the client still has a +/// usable socket to parse from, and the bytes already written are delivered +/// rather than abandoned to whatever a close does to a socket still holding +/// unread peer data. +/// +/// A failure is fatal for every cause except the peer having gone, which is not +/// the fixture's doing and not a verdict on the bytes it sent. A client that +/// has already reset the connection — an abandoned redirect hop, a test that +/// stopped reading — leaves nothing to half-close, and the platform reports +/// that as a transport error (`WSAECONNABORTED` or `ENOTCONN` on Windows, a +/// reset or a disconnection on Unix). Panicking there would turn the client's +/// own departure into a fixture failure, which is the same conflation of +/// transport outcome with protocol verdict this fixture exists to remove. Any +/// other failure means the fixture could not frame what it wrote — the one +/// other reason a client could see a short response — so it still panics. fn finish_raw_response(stream: &TcpStream) { if let Err(err) = stream.shutdown(Shutdown::Write) { - panic!("failed to shut down the raw fixture response: {err}"); + assert!( + peer_is_gone(&err), + "failed to shut down the raw fixture response: {err}" + ); } } + +/// Report whether `err` is the peer having already left the connection. +/// +/// Deliberately narrow. `BrokenPipe` is excluded: a peer that closed its read +/// half is still there to be answered, so a broken write is the fixture failing +/// to deliver, not the client departing, and must stay fatal. +pub(super) fn peer_is_gone(err: &std::io::Error) -> bool { + use std::io::ErrorKind; + + matches!( + err.kind(), + ErrorKind::NotConnected | ErrorKind::ConnectionReset | ErrorKind::ConnectionAborted + ) +} diff --git a/test_support/src/http/spawn.rs b/test_support/src/http/spawn.rs index db1ab51cb..2e3708425 100644 --- a/test_support/src/http/spawn.rs +++ b/test_support/src/http/spawn.rs @@ -3,7 +3,7 @@ //! Every fixture the module exposes is built here, and every one of them is //! built the same way: bind a loopback port, share a request counter, a request //! log, and a shutdown flag with the caller, then move a -//! [`DriveStrategy`] into a named thread that serves the responses. What the +//! [`FixtureResponses`] into a named thread that serves the responses. What the //! fixture emits is the only thing a caller varies. use std::{ @@ -18,7 +18,7 @@ use std::{ use super::{ HttpResponse, HttpServer, HttpServerConfig, RawHttpResponse, RequestLog, - server::{DriveStrategy, FixtureLedger, RawResponses, StructuredResponses}, + server::{FixtureLedger, FixtureResponses, FixtureServe}, }; /// Spawn an HTTP server using `config`, emitting responses in sequence. @@ -32,7 +32,7 @@ pub(super) fn spawn_fixture_server( ) -> io::Result<(String, Arc, RequestLog, HttpServer)> { let response_sequence = responses.into_iter().collect::>(); spawn_fixture_thread( - Box::new(StructuredResponses::new(response_sequence)), + Box::new(FixtureResponses::structured(response_sequence)), config, ) } @@ -40,26 +40,26 @@ pub(super) fn spawn_fixture_server( /// Spawn an HTTP server using `config`, emitting raw responses in sequence. /// /// The raw counterpart of [`spawn_fixture_server`], and the only place the two -/// shapes diverge: both hand a [`DriveStrategy`] to [`spawn_fixture_thread`], +/// shapes diverge: both hand a [`FixtureResponses`] to [`spawn_fixture_thread`], /// which owns binding, accounting, and the thread. pub(super) fn spawn_raw_fixture_server( responses: impl IntoIterator, config: HttpServerConfig, ) -> io::Result<(String, Arc, RequestLog, HttpServer)> { let response_sequence = responses.into_iter().collect::>(); - spawn_fixture_thread(Box::new(RawResponses::new(response_sequence)), config) + spawn_fixture_thread(Box::new(FixtureResponses::raw(response_sequence)), config) } /// Serve `strategy` on a fresh listener, returning its URL and shared state. /// /// This is the one implementation behind every fixture the module exposes. A -/// [`DriveStrategy`] supplies what to emit and how to advertise it; everything +/// [`FixtureResponses`] supplies what to emit and how to advertise it; everything /// else — binding a loopback port, the request counter and log shared with the /// caller, the shutdown flag, the named thread, and the handle that joins or /// signals them — is common, so no fixture shape can drift from another's /// acceptance, accounting, or shutdown behaviour. fn spawn_fixture_thread( - strategy: Box, + strategy: Box, config: HttpServerConfig, ) -> io::Result<(String, Arc, RequestLog, HttpServer)> { let listener = TcpListener::bind(("127.0.0.1", 0))?; diff --git a/test_support/src/http/tests.rs b/test_support/src/http/tests.rs index 75fad38e8..f9ab84cf8 100644 --- a/test_support/src/http/tests.rs +++ b/test_support/src/http/tests.rs @@ -261,6 +261,42 @@ fn raw_response_fixture_delivers_the_exact_bytes_it_was_given() -> anyhow::Resul Ok(()) } +/// The raw fixture tolerates exactly one shutdown failure: the peer's absence. +/// +/// Pinned as a predicate rather than through a live reset, because the window +/// between the fixture writing its response and shutting down its write half +/// cannot be lost on purpose from the test side without a sleep, and a +/// sleep-based assertion here would reintroduce the very platform-dependent +/// race this fixture exists to remove. The predicate is the whole of the +/// decision — which transport outcomes the fixture may absorb silently — so +/// asserting it directly is asserting the behaviour. +/// +/// The negative cases matter as much as the positive ones: widening this set +/// would let a fixture that genuinely failed to frame a response report success, +/// which is the silent failure the half-close contract guards against. +#[test] +fn only_a_departed_peer_is_tolerated_when_framing_a_raw_response() { + use std::io::{Error, ErrorKind}; + + for kind in [ + ErrorKind::NotConnected, + ErrorKind::ConnectionReset, + ErrorKind::ConnectionAborted, + ] { + assert!( + super::server::peer_is_gone(&Error::from(kind)), + "{kind:?} is the peer having left, and must not fail the fixture", + ); + } + + for kind in [ErrorKind::BrokenPipe, ErrorKind::TimedOut, ErrorKind::Other] { + assert!( + !super::server::peer_is_gone(&Error::from(kind)), + "{kind:?} is not the peer having left, and must still fail the fixture", + ); + } +} + /// Send one minimal HTTP request and read the response back to EOF. /// /// Unlike [`send_request`], this returns the bytes rather than asserting a