diff --git a/ROADMAP.md b/ROADMAP.md index 2ec174f..177ba4e 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -229,21 +229,19 @@ PubSub underneath. ## Known bugs -- **HTTP/1.0 keep-alive: server honors but never advertises** (found - 2026-06-12 via `ab -k`). A 1.0 request with `Connection: Keep-Alive` - IS kept alive (second pipelined request on the same socket gets - answered), but the response carries no `Connection: Keep-Alive` - header. Per 1.0 convention the client may only reuse when the server - explicitly opts in; absent the header, clients assume the server will - close and wait for EOF — which never comes. `ab -k` therefore - deadlocks until its poll timeout (`apr_pollset_poll: timeout 70007`) - on the FIRST response; no keep-alive bench numbers obtainable. - Fix direction: when the conn stays alive on a 1.0 exchange, echo - `Connection: Keep-Alive` in serialise_response — or don't keep 1.0 - alive at all. Decide scope (error responses, HEAD) with user first. - Baseline for reference: GET /users (crud example, release, loopback, - stdout logging silenced), no keep-alive, c=50: ~18.9k req/s, 0 - failed, p50 3ms / p99 5ms. HTTP/1.1 path responds normally. +- ~~**HTTP/1.0 keep-alive: server honors but never advertises**~~ FIXED + (2026-06-12, same session it was found). As-landed decisions: + serialise_response now echoes `connection: keep-alive` on a kept-alive + 1.0 response (1.1 keep-alive stays implicit); error statuses (>=400) + on 1.0 force close — no keep-alive advertised on a 4xx/5xx where reuse + is opt-in (conn_actor, alongside the existing 1.0-stream force-close); + HEAD needs nothing special (header echo only, framing untouched); + parse-error responses already carried `connection: close`. Validated + against the original repro: `ab -k -n 5000 -c 50` on GET /users now + reports 5000/5000 keep-alive requests, 0 failed, ~63k req/s + (vs ~18.9k no-keep-alive baseline, same box). Watch out benching: + ab counts a response toward "Non-2xx" silently — a 404'd route still + shows "Failed requests: 0" with great rps; check the route first. ## Later / icebox diff --git a/src/conn_actor.rs b/src/conn_actor.rs index daa2451..52adb7e 100644 --- a/src/conn_actor.rs +++ b/src/conn_actor.rs @@ -248,10 +248,13 @@ pub fn run_connection( // ----- 4. Write the response. ----- // A Stream body on HTTP/1.0 has no chunked framing: the body is // delimited by EOF, so keep-alive is forced off for this response - // (and `connection: close` goes on the wire). + // (and `connection: close` goes on the wire). Error statuses on + // 1.0 also force close: we don't advertise keep-alive on a + // 4xx/5xx to a protocol generation where reuse is opt-in. let is_stream = matches!(response_conn.resp_body, RespBody::Stream(_)); + let is_error = response_conn.status.unwrap_or(200) >= 400; let keep_alive = keep_alive - && !(is_stream && version == HttpVersion::Http10); + && !(version == HttpVersion::Http10 && (is_stream || is_error)); let head_bytes = parser::serialise_response(&response_conn, keep_alive); if write_all(raw, &head_bytes, Instant::now() + limits.write_timeout).is_err() { diff --git a/src/parser.rs b/src/parser.rs index 3925d4a..06dff0c 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -252,8 +252,17 @@ pub fn serialise_response(conn: &Conn, keep_alive: bool) -> Vec { out.extend_from_slice(b"\r\n"); } - if !wrote_connection && !keep_alive { - out.extend_from_slice(b"connection: close\r\n"); + if !wrote_connection { + if !keep_alive { + out.extend_from_slice(b"connection: close\r\n"); + } else if conn.version == HttpVersion::Http10 { + // HTTP/1.0 defaults to close: a connection we intend to keep + // open MUST be advertised back, or a spec-following client + // waits for an EOF that never comes (`ab -k` deadlocked on + // exactly this). 1.1 keep-alive is the default and stays + // implicit. + out.extend_from_slice(b"connection: keep-alive\r\n"); + } } out.extend_from_slice(b"\r\n"); @@ -452,6 +461,23 @@ mod tests { assert!(s.contains("connection: close")); } + #[test] + fn serialise_http10_keepalive_is_echoed() { + let mut conn = Conn::new().put_status(200).put_body("hi"); + conn.version = HttpVersion::Http10; + let bytes = serialise_response(&conn, true); + let s = std::str::from_utf8(&bytes).unwrap(); + assert!(s.contains("connection: keep-alive"), "head: {s}"); + } + + #[test] + fn serialise_http11_keepalive_stays_implicit() { + let conn = Conn::new().put_status(200).put_body("hi"); + let bytes = serialise_response(&conn, true); + let s = std::str::from_utf8(&bytes).unwrap(); + assert!(!s.contains("connection:"), "head: {s}"); + } + #[test] fn serialise_user_content_length_is_respected() { let conn = Conn::new().put_status(200) diff --git a/tests/integration.rs b/tests/integration.rs index a04a6c7..e5cd91c 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -660,6 +660,51 @@ fn keep_alive_after_chunked_response() { assert_eq!(http_body(&resp), b"hello"); } +/// HTTP/1.0 keep-alive must be advertised back (`connection: keep-alive`) +/// when honored — 1.0 defaults to close, so a silent keep-open leaves +/// spec-following clients (ab -k) waiting for EOF. The socket then serves +/// a second request. +#[test] +fn http10_keepalive_advertised_and_socket_reused() { + let pipe = Pipeline::new().plug( + Router::new().get("/", |c: Conn, _n: Next| c.put_status(200)) + ); + let port = spawn_server(pipe); + let mut s = TcpStream::connect(("127.0.0.1", port)).unwrap(); + s.set_read_timeout(Some(Duration::from_secs(5))).unwrap(); + + let req = b"GET / HTTP/1.0\r\nHost: x\r\nConnection: keep-alive\r\n\r\n"; + s.write_all(req).unwrap(); + let head = String::from_utf8(read_response_head(&mut s)).unwrap(); + assert!(head.starts_with("HTTP/1.0 200 OK\r\n"), "head: {head}"); + assert!(head.contains("connection: keep-alive"), "head: {head}"); + + // Same socket, second request — the keep-alive was real. + s.write_all(req).unwrap(); + let head2 = String::from_utf8(read_response_head(&mut s)).unwrap(); + assert!(head2.starts_with("HTTP/1.0 200 OK\r\n"), "head: {head2}"); +} + +/// Error statuses on HTTP/1.0 force close even when the client asked for +/// keep-alive: `connection: close` on the wire, then EOF. +#[test] +fn http10_keepalive_forced_off_on_error_status() { + let pipe = Pipeline::new().plug( + Router::new().get("/", |c: Conn, _n: Next| c.put_status(200)) + ); + let port = spawn_server(pipe); + let mut s = TcpStream::connect(("127.0.0.1", port)).unwrap(); + s.set_read_timeout(Some(Duration::from_secs(5))).unwrap(); + + s.write_all(b"GET /missing HTTP/1.0\r\nHost: x\r\nConnection: keep-alive\r\n\r\n") + .unwrap(); + let mut resp = Vec::new(); + s.read_to_end(&mut resp).unwrap(); // EOF: server closed + let head = String::from_utf8_lossy(&resp).to_string(); + assert!(head.starts_with("HTTP/1.0 404 Not Found\r\n"), "head: {head}"); + assert!(head.contains("connection: close"), "head: {head}"); +} + /// HTTP/1.0 has no chunked framing: a stream body goes out raw, the /// response carries `connection: close` and no transfer-encoding, and EOF /// delimits the body.