-
Notifications
You must be signed in to change notification settings - Fork 138
Full support for bidirectional streaming #879
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Full support for bidirectional streaming #879
Conversation
| .executing(_, .finished, _), | ||
| .executing(_, .producing, _), | ||
| .executing(_, .paused, _): | ||
| preconditionFailure("Invalid state: \(self.state)") |
There was a problem hiding this comment.
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)") |
There was a problem hiding this comment.
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() { | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentionally empty?
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
endForwardedstate toTransaction.StateMachine.RequestStreamStateRenamed
succeedRequest→forwardResponseEndin bothHTTPRequestStateMachine.ActionandHTTP1ConnectionStateMachine.ActionProtocol Changes
requestBodyStreamSent()toHTTPExecutableRequestprotocolTransactionandRequestBagRequest State Machine Updates
Updated
FinalSuccessfulRequestAction.sendRequestEnd(EventLoopPromise<Void>?)to simpler.requestDone.nonecase for when response completes but request is still in-flightsendRequestEndaction now includesFinalSuccessfulRequestActionChannel Handler Updates
HTTP1ClientChannelHandler
sendRequestEndnow properly handles scenarios where response has already completedHTTP2ClientRequestHandler
sendRequestEndsignature.requestDone)RequestBag State Machine
Added
endReceivedstate toResponseStreamStateUpdated
FinishAction.forwardStreamFinishedAndSucceedTaskfor the case where both streams complete simultaneouslyError Handling
Transaction.StateMachinecancelExecutoraction to the fail pathfailRequestStreamContinuationfor proper cleanupTechnical Details
The Problem
Previously, when a server sent a complete response before the client finished uploading the request body, AHC would:
This made it impossible to implement proper bidirectional streaming or handle scenarios like:
The Solution
The new state machine properly tracks four completion states:
endForwarded/endReceivedstatesThe key insight is the
endForwardedstate, 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:Future Work
This PR lays the groundwork for: