fix(http): advertise keep-alive on HTTP/1.0 responses we keep open
1.0 defaults to close: honoring 'Connection: Keep-Alive' without echoing it back left spec-following clients waiting for an EOF that never came (ab -k deadlocked to its poll timeout on response 1). As agreed: - serialise_response echoes 'connection: keep-alive' when keep_alive is on AND version is 1.0. 1.1 keep-alive stays implicit. - Error statuses (>=400) on 1.0 force keep_alive off in the conn actor (joins the existing 1.0-stream force-close): we don't advertise reuse on a 4xx/5xx to a protocol generation where reuse is opt-in. - HEAD needs nothing: the echo is framing-neutral. - Parse-error responses already carried 'connection: close'. Tests: unit pair (1.0 echo / 1.1 implicit) + integration pair (socket actually reused on 1.0; 404 forces close + EOF). Validated against the original repro: ab -k -n 5000 -c 50 GET /users -> 5000/5000 keep-alive, 0 failed, ~63k rps (~18.9k without keep-alive).
This commit is contained in:
+13
-15
@@ -229,21 +229,19 @@ PubSub underneath.
|
|||||||
|
|
||||||
## Known bugs
|
## Known bugs
|
||||||
|
|
||||||
- **HTTP/1.0 keep-alive: server honors but never advertises** (found
|
- ~~**HTTP/1.0 keep-alive: server honors but never advertises**~~ FIXED
|
||||||
2026-06-12 via `ab -k`). A 1.0 request with `Connection: Keep-Alive`
|
(2026-06-12, same session it was found). As-landed decisions:
|
||||||
IS kept alive (second pipelined request on the same socket gets
|
serialise_response now echoes `connection: keep-alive` on a kept-alive
|
||||||
answered), but the response carries no `Connection: Keep-Alive`
|
1.0 response (1.1 keep-alive stays implicit); error statuses (>=400)
|
||||||
header. Per 1.0 convention the client may only reuse when the server
|
on 1.0 force close — no keep-alive advertised on a 4xx/5xx where reuse
|
||||||
explicitly opts in; absent the header, clients assume the server will
|
is opt-in (conn_actor, alongside the existing 1.0-stream force-close);
|
||||||
close and wait for EOF — which never comes. `ab -k` therefore
|
HEAD needs nothing special (header echo only, framing untouched);
|
||||||
deadlocks until its poll timeout (`apr_pollset_poll: timeout 70007`)
|
parse-error responses already carried `connection: close`. Validated
|
||||||
on the FIRST response; no keep-alive bench numbers obtainable.
|
against the original repro: `ab -k -n 5000 -c 50` on GET /users now
|
||||||
Fix direction: when the conn stays alive on a 1.0 exchange, echo
|
reports 5000/5000 keep-alive requests, 0 failed, ~63k req/s
|
||||||
`Connection: Keep-Alive` in serialise_response — or don't keep 1.0
|
(vs ~18.9k no-keep-alive baseline, same box). Watch out benching:
|
||||||
alive at all. Decide scope (error responses, HEAD) with user first.
|
ab counts a response toward "Non-2xx" silently — a 404'd route still
|
||||||
Baseline for reference: GET /users (crud example, release, loopback,
|
shows "Failed requests: 0" with great rps; check the route first.
|
||||||
stdout logging silenced), no keep-alive, c=50: ~18.9k req/s, 0
|
|
||||||
failed, p50 3ms / p99 5ms. HTTP/1.1 path responds normally.
|
|
||||||
|
|
||||||
## Later / icebox
|
## Later / icebox
|
||||||
|
|
||||||
|
|||||||
+5
-2
@@ -248,10 +248,13 @@ pub fn run_connection(
|
|||||||
// ----- 4. Write the response. -----
|
// ----- 4. Write the response. -----
|
||||||
// A Stream body on HTTP/1.0 has no chunked framing: the body is
|
// 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
|
// 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_stream = matches!(response_conn.resp_body, RespBody::Stream(_));
|
||||||
|
let is_error = response_conn.status.unwrap_or(200) >= 400;
|
||||||
let keep_alive = keep_alive
|
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);
|
let head_bytes = parser::serialise_response(&response_conn, keep_alive);
|
||||||
if write_all(raw, &head_bytes, Instant::now() + limits.write_timeout).is_err() {
|
if write_all(raw, &head_bytes, Instant::now() + limits.write_timeout).is_err() {
|
||||||
|
|||||||
+27
-1
@@ -252,8 +252,17 @@ pub fn serialise_response(conn: &Conn, keep_alive: bool) -> Vec<u8> {
|
|||||||
out.extend_from_slice(b"\r\n");
|
out.extend_from_slice(b"\r\n");
|
||||||
}
|
}
|
||||||
|
|
||||||
if !wrote_connection && !keep_alive {
|
if !wrote_connection {
|
||||||
|
if !keep_alive {
|
||||||
out.extend_from_slice(b"connection: close\r\n");
|
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");
|
out.extend_from_slice(b"\r\n");
|
||||||
@@ -452,6 +461,23 @@ mod tests {
|
|||||||
assert!(s.contains("connection: close"));
|
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]
|
#[test]
|
||||||
fn serialise_user_content_length_is_respected() {
|
fn serialise_user_content_length_is_respected() {
|
||||||
let conn = Conn::new().put_status(200)
|
let conn = Conn::new().put_status(200)
|
||||||
|
|||||||
@@ -660,6 +660,51 @@ fn keep_alive_after_chunked_response() {
|
|||||||
assert_eq!(http_body(&resp), b"hello");
|
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
|
/// HTTP/1.0 has no chunked framing: a stream body goes out raw, the
|
||||||
/// response carries `connection: close` and no transfer-encoding, and EOF
|
/// response carries `connection: close` and no transfer-encoding, and EOF
|
||||||
/// delimits the body.
|
/// delimits the body.
|
||||||
|
|||||||
Reference in New Issue
Block a user