Skip to content

Conversation

@fabianfett
Copy link
Member

Note:

This is a long LLM generated PR description. However it captures very well, what has been changed and has already been reduced for brevity. The PR is sadly quite complex but I think the description captures the changes quite well.

This is foundational work needed to properly support HTTP trailers and scenarios where the server sends a complete response before the client finishes uploading (e.g., early rejection, 100-continue flows, or bidirectional streaming protocols).

Changes

State Machine Improvements

  • Added endForwarded state to Transaction.StateMachine.RequestStreamState

    • This new state distinguishes between "request data forwarded to the channel" and "request data written to the network"
    • Properly handles the race condition where response completes before the request write completes
  • Renamed succeedRequestforwardResponseEnd in both HTTPRequestStateMachine.Action and HTTP1ConnectionStateMachine.Action

    • Better reflects the semantic meaning: we're forwarding the end of the response stream, not necessarily succeeding the entire request yet
    • More accurate naming for bidirectional streaming scenarios

Protocol Changes

  • Added requestBodyStreamSent() to HTTPExecutableRequest protocol
    • Called by the channel handler when the request body stream has been fully written to the network
    • Allows proper coordination between request and response stream completion
    • Implemented in both Transaction and RequestBag

Request State Machine Updates

  • Updated FinalSuccessfulRequestAction

    • Changed .sendRequestEnd(EventLoopPromise<Void>?) to simpler .requestDone
    • Added .none case for when response completes but request is still in-flight
    • Removed the need to pass promises around, simplifying the state machine
  • sendRequestEnd action now includes FinalSuccessfulRequestAction

    • Allows the state machine to signal what should happen after the request completes
    • Enables proper cleanup coordination (idle connection, close, or continue)

Channel Handler Updates

  • HTTP1ClientChannelHandler

    • sendRequestEnd now properly handles scenarios where response has already completed
    • Added future callback to coordinate request completion with final actions
    • Properly manages connection state (idle vs close) based on both streams completing
  • HTTP2ClientRequestHandler

    • Updated to handle new sendRequestEnd signature
    • Properly ignores HTTP/1-specific final actions (like .requestDone)

RequestBag State Machine

  • Added endReceived state to ResponseStreamState

    • Tracks when the response has completed while request is still ongoing
    • Enables proper sequencing: response end → request end → task completion
  • Updated FinishAction

    • Added .forwardStreamFinishedAndSucceedTask for the case where both streams complete simultaneously
    • Ensures delegate methods are called in the correct order

Error Handling

  • Improved failure handling in Transaction.StateMachine
    • Now properly handles errors that occur after response completes but before request finishes
    • Added cancelExecutor action to the fail path
    • Executor is now passed to failRequestStreamContinuation for proper cleanup

Technical Details

The Problem

Previously, when a server sent a complete response before the client finished uploading the request body, AHC would:

  1. Receive the full response (head, body, end)
  2. But NOT inform the user that the response was complete if the request was still streaming
  3. Only succeed the request after both streams completed

This made it impossible to implement proper bidirectional streaming or handle scenarios like:

  • Server rejecting a large upload early (e.g., 413 Payload Too Large)
  • 100-continue flows where the server responds before request completes
  • HTTP trailers sent by the server

The Solution

The new state machine properly tracks four completion states:

  1. Neither complete: Normal request/response in flight
  2. Response complete, request ongoing: New endForwarded/endReceived states
  3. Request complete, response ongoing: Existing logic
  4. Both complete: Request succeeds

The key insight is the endForwarded state, which represents "we've given all request data to the channel, but it hasn't been written to the network yet". This allows us to:

  • Immediately forward response completion to the user
  • Wait for the write to complete before cleaning up resources
  • Properly sequence connection state transitions

Future Work

This PR lays the groundwork for:

  • Proper internal HTTP trailer support (both sending and receiving)

@fabianfett fabianfett added 🆕 semver/minor Adds new public API. 🔨 semver/patch No public API change. and removed 🔨 semver/patch No public API change. labels Jan 19, 2026
@fabianfett fabianfett changed the title Ff add support for trailers Full support for bidirectional streaming Jan 19, 2026
.executing(_, .finished, _),
.executing(_, .producing, _),
.executing(_, .paused, _):
preconditionFailure("Invalid state: \(self.state)")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should think very hard about introducing more precondition failures in these state machines. Consider having this be an assert + throw.

return .sendRequestEnd(writePromise)
case .sendRequestEnd(let writePromise, let finalAction):
guard case .inRequest(_, close: let close) = self else {
fatalError("Invalid state: \(self)")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same note here about these crashes: unless we can write a comment explaining why this is impossible and a code reviewer can check it, better to do assert + throw.

}

private func requestBodyStreamSent0() {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentionally empty?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆕 semver/minor Adds new public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants