Skip to content

fix(ext/http): keep request body readable after response is sent - #36629

Merged
bartlomieju merged 4 commits into
mainfrom
fix/serve-request-body-outlives-response-36624
Aug 19, 2026
Merged

bartlomieju merged 4 commits into
mainfrom
fix/serve-request-body-outlives-response-36624

Conversation

@bartlomieju

Copy link
Copy Markdown
Member

Fixes #36624.

What

Since 2.8.3, Deno.serve ties the request-body stream to the response
lifecycle on the Deno-owned HTTP/1.1 path (#34446). A handler that responds
before fully consuming request.body can no longer finish reading it:

  1. Streaming response held open: if the handler crosses a macrotask
    boundary and then returns a streaming Response while request.body is
    still being piped, the remaining request-body chunks and EOS were never
    delivered — the read stalled forever.
  2. Completed response: if the handler returns a complete (e.g. bodyless
    201) response while the request body is read in the background, the read
    failed with Cannot read request body as underlying resource unavailable.

Both work on ≤ 2.8.2 (hyper) and now work again.

Why

The raw H1 path shares a single connection between the request-body reader
and the response writer. Two problems fell out of that:

  1. While a streaming response is held open, the response writer's
    poll_peer_closed_with reads bytes off the socket — including request-body
    bytes — into an internal buffer, but only woke itself. The body reader
    parked forever with its bytes stranded. It now registers a waker in
    SharedConn that poll_peer_closed_with wakes whenever it buffers data
    (client-disconnect detection is preserved).

  2. perf(ext/http): add Deno-owned HTTP/1.1 serve path #34446 added an eager core.tryClose(#streamRid) to InnerRequest.close(),
    which closed the request-body resource out from under an in-flight
    background read. Hyper never hit this because it defers cleanup to
    HttpRequestBodyAutocloser (fires on record drop, after the read
    completes). InnerRequest.close() no longer force-closes a body that is
    still being consumed; ownership passes to the stream (autoClose: true),
    which closes the resource on end-of-stream, cancel, or error. The
    connection loop also waits for the reader to finish (a new reader_done
    signal) before reclaiming the shared connection.

Tests

New spec test tests/specs/serve/request_body_outlives_response/ covering
both facets (with a timeout watchdog so a future regression fails fast rather
than hanging CI).

Verified locally: all serve specs, serve_test/http_test/fetch_test/
websocket_test units, unit_node::http_test/http2_test, deno_http_h1
(83 tests), and serve integration tests pass. Also checked manually:
keep-alive with fully-read bodies, never-read body + keep-alive (no hang),
the microtask variant, and a 20-chunk / 10 KB streamed body under interleaved
reads.

)

Since 2.8.3 the Deno-owned HTTP/1.1 serve path shared a single connection
between the request-body reader and the response writer, so responding
before `request.body` was fully read could stall the read or fail it with
`Cannot read request body as underlying resource unavailable`.

Two problems, both fixed here:

1. While a streaming response is held open, the response writer's
   `poll_peer_closed_with` reads bytes off the socket (including request-body
   bytes) into a buffer but only woke itself, leaving the body reader parked
   with its bytes stranded. The body reader now registers a waker in
   `SharedConn` that `poll_peer_closed_with` wakes when it buffers data.

2. `InnerRequest.close()` eagerly closed the request-body resource when the
   response was dispatched, cutting off a background read. It no longer
   force-closes a body that is still being consumed; ownership passes to the
   stream (`autoClose: true`), which closes the resource on end-of-stream,
   cancel, or error. The connection loop also waits for the reader to finish
   (via a new `reader_done` signal) before reclaiming the connection, so it
   doesn't take the shared connection out from under an in-flight read.

Adds a spec test covering both facets (streaming response held open, and a
completed response while the body is read in the background).
@bartlomieju bartlomieju changed the title fix(ext/http): keep request body readable after response is sent (#36624) fix(ext/http): keep request body readable after response is sent Aug 19, 2026
Address review feedback on the raw H1 request-body lifetime:

- Waiting for the reader to finish before reclaiming the connection parks
  the connection task forever when a handler abandons `req.body` after a
  partial read (nothing ever closes the resource), which also blocks
  `server.shutdown()`. Instead, leave the connection with the body
  resource: a request whose body was not fully read never reuses its
  connection anyway, and the socket is closed when the stream reaches
  end-of-stream or is closed, cancelled or dropped.
- Make body reads cancellable via the connection cancel handle, so a read
  that outlives its response cannot keep a torn-down server alive.
- Don't reclaim the connection on the synchronous native-response path
  either while a reader still owns the body.
- Clear the body-read waker once a read completes, so `poll_peer_closed_with`
  doesn't wake a task that is no longer waiting.

Adds an http_h1 unit test for the waker hand-off and extends the spec test
with the fast static response and abandoned-reader cases, plus byte counts
so a truncated body fails as loudly as a stalled one.
Both specs listened on the fixed port 12345, so they fail whenever they run
at the same time (or alongside anything else holding that port). They now
bind port 0 and take the assigned port from `onListen`, which also removes
the fetch retry loop and lets the windows/unix variants of `basic` collapse
into one test. `--allow-net` is needed now because `deno serve` derives its
implicit net grant from the `--port` value.
@bartlomieju
bartlomieju merged commit 46baf26 into main Aug 19, 2026
268 of 270 checks passed
@bartlomieju
bartlomieju deleted the fix/serve-request-body-outlives-response-36624 branch August 19, 2026 23:08
dinwwwh added a commit to middleapi/standard-server that referenced this pull request Sep 4, 2026
Deno 2.8.3 through 2.9.5 stopped delivering request body chunks to a
Deno.serve handler once it started writing a streaming response, so the
deno-fetch event stream echo tests deliberately asserted the stall. Deno 2.9.6
fixed it (denoland/deno#36629), which made those assertions fail in CI.

Both Deno adapters are full-duplex now, so the FULL_DUPLEX_ADAPTERS gate and
its stall branches are removed.
dinwwwh added a commit to middleapi/standard-server that referenced this pull request Sep 5, 2026
…pnpm (#82)

Bun 1.4 and Deno 2.9.6 both fixed stream behavior that the Bun and Deno
test suites deliberately asserted as broken, so CI on main has been red
since Bun 1.4.0 landed and the Deno failures surfaced right after. The
failures are runtime behavior changes, not standardserver bugs, and the
tests now assert the fixed behavior instead of skipping anything.
Alongside that, `@types/node` and `@types/bun` are bumped,
`EventStreamDecoderStream` no longer declares `implements
ReadableWritablePair`, pnpm is now declared through `devEngines` so it
self-provisions, and Node 20 is dropped from the CI matrix.

## Bun 1.4: stream cancellation now propagates

Pure-Bun repros with no standardserver code show the same change between
Bun 1.3.14 and 1.4.0:

- Cancelling a response body now closes the connection, so the server
signal aborts and the stream cancel fires.
- A streaming request body is now cancelled on the client once the
server has responded, with or without keep-alive.
- An errored server response stream now reaches the client as an
`ECONNRESET` error instead of a clean end of body.

Both Bun adapters propagate request and response cancellation, so the
`REQUEST_STREAM_CANCEL_ADAPTERS` and `RESPONSE_STREAM_CANCEL_ADAPTERS`
gates and their else-branches are gone. One difference remains behind a
new `RESPONSE_STREAM_ERROR_ABORT_ADAPTERS` set: `Bun.serve` only aborts
`request.signal` when the client goes away, not when the server itself
closes the connection after its response stream errors. The test asserts
that current behavior so a future Bun change surfaces as a failure, same
convention as before.

## Deno 2.9.6: request body stays readable while streaming a response

Deno 2.8.3 through 2.9.5 stopped delivering request body chunks to a
`Deno.serve` handler once it started writing a streaming response, so
the `deno-fetch` event stream echo tests asserted the stall. Deno 2.9.6
fixed it (denoland/deno#36629), and CI picks up the latest Deno 2, so
those assertions started failing. Both Deno adapters are full-duplex
now, so the `FULL_DUPLEX_ADAPTERS` gate and its stall branches are
removed, with the minimum Deno version noted in the test.

## Types

- `@types/node` 26.1 → 26.4.1 across the workspace, `@types/bun` 1.3 →
1.4.0 for the Bun tests.
- `EventStreamDecoderStream` drops `implements ReadableWritablePair`.
Node's types only expose that name under `stream/web`, not as a global,
so the clause failed to resolve for consumers type-checking against Node
types alone. The class still exposes the same `readable`/`writable` pair
structurally, so nothing changes at runtime or for callers.

## Toolchain and CI

- pnpm is declared via `devEngines.packageManager` (`^11.0.0`, `onFail:
download`) instead of a `packageManager` pin, so any pnpm on a
contributor's machine or in CI provisions a compatible 11.x
automatically. The lockfile records the resolved 11.25.0 and picks up
refreshed resolutions within the existing dependency ranges.
- Node 20 is removed from the CI matrix. pnpm 11 requires Node 22.13+
and crashed on `node:sqlite` before running a single test on Node 20,
and Node 20 reached end-of-life in April 2026. The matrix is now 22, 24
and 26.

## Testing

- CI is green on Node 22, 24 and 26 at the current head: lint and
typecheck, tests with coverage, publish preview, benchmarks and codecov.
- Full `pnpm -w test` passes locally on the refreshed lockfile with pnpm
11.25.0: Bun 50 pass, Deno 4 suites / 50 steps pass, vitest 1076 tests
across 59 files pass.
- `bun test` on Bun 1.4.1 and `deno test` on Deno 2.9.6 each pass 3 of 3
repeated runs.
- `pnpm run type:check` passes for all 11 workspace projects with the
new types.
- Bun older than 1.4.0 or Deno older than 2.9.6 will now fail these
tests, matching the CI baseline (`pnpm runtime set bun 1` / `deno 2`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deno.serve: request body stream stalls or errors when the response is sent before the body is fully read (regression in 2.8.3)

2 participants