Skip to content

Skip request body drain when connection will close#2504

Merged
yhirose merged 2 commits into
yhirose:masterfrom
emreay-:skip-drain-on-closing-connection
Jul 22, 2026
Merged

Skip request body drain when connection will close#2504
yhirose merged 2 commits into
yhirose:masterfrom
emreay-:skip-drain-on-closing-connection

Conversation

@emreay-

@emreay- emreay- commented Jul 20, 2026

Copy link
Copy Markdown

Summary

Skip draining an unconsumed framed request body when the response has already committed the connection to close.

The post-response drain prevents unread body bytes from being parsed as the next request on a keep-alive connection. When the response contains Connection: close, there is no next request to protect.

Problem

If a ContentReader aborts an unterminated chunked upload, the current drain waits for the terminal chunk after sending the response. A client that keeps producing chunks continually resets the read timeout, so:

  • the announced connection close never occurs;
  • the client receives no transport-level failure signal; and
  • a server worker remains occupied discarding data.

This also means the server can send Connection: close while continuing to consume the request indefinitely.

Fix

Use the finalized response header as the close decision:

if (!req.body_consumed_ && detail::has_framed_body(req)) {
  if (res.get_header_value("Connection") == "close") {
    connection_closed = true;
  } else {
    // Drain before reusing the connection.
  }
}

write_response_core already sets this header for keep-alive exhaustion, request-directed closure, handler-directed closure, and error responses. Reusable connections retain the existing drain behavior.

Relationship to #2450

This is a follow-up to #2450:

Both changes restrict the drain to cases where it can serve its request-smuggling-prevention purpose.

Test

A raw-socket regression test sends an unterminated chunked request whose ContentReader rejects the first chunk. It verifies that the client receives the 409 Connection: close response followed by EOF.

On unpatched master, the final recv times out because the server remains in the drain. With this change, it returns 0 immediately.

The new test and the existing #2450 regression both pass on Windows with MSVC.

The post-response drain exists to keep unread framed body bytes from being parsed as a subsequent request on a persistent connection. Once response generation has committed the connection to close, there can be no subsequent request, so draining no longer provides that protection.

Continuing to drain is especially harmful when a ContentReader aborts an unterminated chunked upload: the server can wait indefinitely for the terminal chunk even after sending Connection: close. This delays the transport close that tells an in-flight uploader to stop and leaves a worker occupied consuming discarded data.

Use the finalized response Connection header as the single source of truth for whether to skip the drain. write_response_core already sets this header for keep-alive exhaustion, request-directed closure, handler-directed closure, and error responses. Marking connection_closed then terminates the keep-alive loop and closes the socket.

Add a raw-socket regression test whose ContentReader rejects the first chunk of an unterminated upload. The test verifies that the 409 response announces Connection: close and that the peer observes EOF rather than timing out while the server drains.
@yhirose

yhirose commented Jul 21, 2026

Copy link
Copy Markdown
Owner

@emreay- thanks for this PR. Could you please take a look at the test/style-check error?

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@yhirose
yhirose merged commit c64bf21 into yhirose:master Jul 22, 2026
43 of 44 checks passed
@yhirose

yhirose commented Jul 22, 2026

Copy link
Copy Markdown
Owner

@emreay- thanks for this fine contribution!

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.

2 participants