Skip to content

Commit acc8c6f

Browse files
authored
fix(http): preserve keep-alive after reading request body (#36690)
Fixes #36657. The raw HTTP/1 server path treated every non-prebuffered request body as non-reusable, even after JavaScript had reached end-of-stream. Track when the JS reader finishes, preserve keep-alive only after the reader releases the connection, and drain any cancelled remainder before parsing the next pipelined request. Unread and in-flight bodies still close the connection. Tests: - `cargo build --bin deno` - `cargo test -p deno_http_h1` - `tests/unit/serve_test.ts` - scoped Clippy and formatting checks AI disclosure: This change was developed with assistance from OpenAI Codex.
1 parent 5d5600f commit acc8c6f

2 files changed

Lines changed: 115 additions & 17 deletions

File tree

ext/http/http_next.rs

Lines changed: 57 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1606,6 +1606,13 @@ impl<I> RawH1RequestBody<I> {
16061606
self.taken.get() && !self.reader_done.get()
16071607
}
16081608

1609+
/// Whether JS is finished with the body and the connection can be reclaimed.
1610+
/// A cancelled connection or failed read is not reusable, even though it also
1611+
/// marks the reader done.
1612+
fn reader_finished_for_reuse(&self) -> bool {
1613+
self.taken.get() && self.reader_done.get() && !self.canceled.get()
1614+
}
1615+
16091616
fn try_take_full(&self) -> Option<Vec<u8>> {
16101617
if self.canceled.get() {
16111618
return None;
@@ -1653,11 +1660,13 @@ where
16531660
)));
16541661
}
16551662
let result = conn.poll_read_body(cx, this.limit);
1656-
// An empty read is end-of-stream: the reader is finished with the body.
1657-
if let Poll::Ready(Ok(buf)) = &result
1658-
&& buf.is_empty()
1659-
{
1660-
this.body.mark_reader_done();
1663+
if let Poll::Ready(result) = &result {
1664+
match result {
1665+
// An empty read is end-of-stream: the reader is finished with the body.
1666+
Ok(buf) if buf.is_empty() => this.body.mark_reader_done(),
1667+
Err(_) => this.body.cancel(),
1668+
Ok(_) => {}
1669+
}
16611670
}
16621671
result
16631672
}
@@ -1691,7 +1700,13 @@ where
16911700
)));
16921701
}
16931702
let buf = this.buf.as_mut().unwrap();
1694-
let read = ready!(conn.poll_read_body_byob(cx, buf))?;
1703+
let read = match ready!(conn.poll_read_body_byob(cx, buf)) {
1704+
Ok(read) => read,
1705+
Err(error) => {
1706+
this.body.cancel();
1707+
return Poll::Ready(Err(error));
1708+
}
1709+
};
16951710
// A zero-length read is end-of-stream: the reader is finished with the body.
16961711
if read == 0 {
16971712
this.body.mark_reader_done();
@@ -3743,7 +3758,7 @@ async fn wait_raw_response_ready(
37433758
/// still owns the request body, once the response has been written.
37443759
///
37453760
/// The request-body reader and the response writer share a single connection.
3746-
/// A request whose body was not fully read never reuses its connection, so
3761+
/// A request whose body reader is still active cannot reuse its connection, so
37473762
/// rather than reclaiming it here -- which would fail an in-flight read with a
37483763
/// spurious "resource unavailable" error and truncate the body -- ownership is
37493764
/// handed to the body resource. The socket is then closed when JS reaches
@@ -3827,6 +3842,20 @@ async fn write_direct_response_for_reader(
38273842
Ok(())
38283843
}
38293844

3845+
async fn drain_raw_h1_request_body<I>(
3846+
mut state: RawH1ConnectionState<I>,
3847+
) -> Option<RawH1ConnectionState<I>>
3848+
where
3849+
I: tokio::io::AsyncRead + tokio::io::AsyncWrite + Unpin,
3850+
{
3851+
state
3852+
.conn
3853+
.discard_body_with_scratch(&mut state.scratch)
3854+
.await
3855+
.ok()?;
3856+
Some(state)
3857+
}
3858+
38303859
async fn wait_raw_response_ready_or_closed<I>(
38313860
record: &RawHttpRecord,
38323861
conn: &mut h1::SharedConn<I>,
@@ -4855,23 +4884,27 @@ async fn serve_http11_raw(
48554884
continue;
48564885
}
48574886
}
4887+
let request_body_finished = request_body_for_cancel
4888+
.as_ref()
4889+
.is_some_and(|body| body.reader_finished_for_reuse());
4890+
let response_keep_alive =
4891+
keep_alive && (!parsed.has_body || request_body_finished);
48584892
match body {
48594893
RawResponseBody::Flat(body) => {
4860-
let keep_alive = keep_alive && !parsed.has_body;
48614894
write_h1_flat_response_shared(
48624895
body_conn.clone(),
48634896
parsed.version,
48644897
response_parts,
48654898
body,
4866-
keep_alive,
4899+
response_keep_alive,
48674900
head,
48684901
)
48694902
.await?;
48704903
}
48714904
RawResponseBody::Stream(body) => {
48724905
let response_context = RawH1ResponseContext {
48734906
version: response_context.version,
4874-
keep_alive: response_context.keep_alive && !parsed.has_body,
4907+
keep_alive: response_keep_alive,
48754908
head: response_context.head,
48764909
};
48774910
write_h1_stream_response_shared(
@@ -4911,17 +4944,24 @@ async fn serve_http11_raw(
49114944
let Some(state) = body_conn.borrow_mut().take() else {
49124945
return Err(raw_h1_connection_closed());
49134946
};
4947+
if response_keep_alive && !cancel.is_canceled() {
4948+
let Some(state) = Box::pin(drain_raw_h1_request_body(state)).await
4949+
else {
4950+
record_cancel_guard.disarm();
4951+
return Ok(());
4952+
};
4953+
conn = state.conn;
4954+
scratch = state.scratch;
4955+
record_cancel_guard.disarm();
4956+
continue;
4957+
}
49144958
conn = state.conn;
49154959
scratch = state.scratch;
4916-
if !keep_alive || parsed.has_body || cancel.is_canceled() {
4917-
if parsed.has_body {
4918-
let _ = conn.discard_body_with_scratch(&mut scratch).await;
4919-
}
4920-
record_cancel_guard.disarm();
4921-
return Ok(());
4960+
if parsed.has_body {
4961+
let _ = conn.discard_body_with_scratch(&mut scratch).await;
49224962
}
49234963
record_cancel_guard.disarm();
4924-
continue;
4964+
return Ok(());
49254965
}
49264966

49274967
let record = RawHttpRecord::new(

tests/unit/serve_test.ts

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3301,6 +3301,64 @@ Deno.test(
33013301
},
33023302
);
33033303

3304+
// https://github.com/denoland/deno/issues/36657
3305+
Deno.test(
3306+
{ permissions: { net: true } },
3307+
async function httpServerKeepsAliveAfterReadingStreamingRequestBody() {
3308+
const listening = Promise.withResolvers<void>();
3309+
const ac = new AbortController();
3310+
const received: string[] = [];
3311+
3312+
await using server = Deno.serve({
3313+
handler: async (request) => {
3314+
if (request.url.endsWith("/cancel")) {
3315+
await request.body!.cancel();
3316+
received.push("cancelled");
3317+
return new Response(stream("ok"));
3318+
}
3319+
received.push(await request.text());
3320+
return new Response("ok");
3321+
},
3322+
port: servePort,
3323+
signal: ac.signal,
3324+
onListen: onListen(listening.resolve),
3325+
onError: createOnErrorCb(ac),
3326+
});
3327+
3328+
await listening.promise;
3329+
const conn = await Deno.connect({ port: servePort });
3330+
const body = "x".repeat(4096);
3331+
const request = (path: string) =>
3332+
`POST ${path} HTTP/1.1\r\nHost: example.domain\r\nContent-Length: ${body.length}\r\n\r\n${body}`;
3333+
await writeAll(
3334+
conn,
3335+
new TextEncoder().encode(
3336+
request("/text") + request("/cancel") + request("/text"),
3337+
),
3338+
);
3339+
3340+
const decoder = new TextDecoder();
3341+
let response = "";
3342+
while (response.split("HTTP/1.1 200 OK").length - 1 < 3) {
3343+
const buf = new Uint8Array(1024);
3344+
const read = await conn.read(buf);
3345+
if (read === null) break;
3346+
response += decoder.decode(buf.subarray(0, read), { stream: true });
3347+
}
3348+
3349+
assertEquals(
3350+
response.split("HTTP/1.1 200 OK").length - 1,
3351+
3,
3352+
);
3353+
assertEquals(received, [body, "cancelled", body]);
3354+
assertEquals(/^connection:\s*close/im.test(response), false);
3355+
3356+
conn.close();
3357+
ac.abort();
3358+
await server.finished;
3359+
},
3360+
);
3361+
33043362
Deno.test(
33053363
{ permissions: { net: true } },
33063364
async function httpServerPostWithInvalidPrefixContentLength() {

0 commit comments

Comments
 (0)