feat: add HTTP client connection - #14547
Conversation
…ia/async-http-client-config
|
Mathlib CI status (docs):
|
|
Reference manual CI status:
|
TwoFX
left a comment
There was a problem hiding this comment.
Just some preliminary comments, I will have to read through the logic in more detail in August.
f06afea to
708dfa7
Compare
TwoFX
left a comment
There was a problem hiding this comment.
Two more issues discovered by Codex, which seem legit to me:
module
import Std.Http.Test.Helpers
/-!
Regression tests for HTTP client connection lifecycle failures found while reviewing #14547,
involving response backpressure, request-body producer errors, and graceful retirement with queued
requests.
-/
open Std.Async
open Std Http Internal
open Std.Http.Client
open Std.Http.Internal.Test.ClientHelpers
namespace ClientConnectionLifecycleTests
private def mkRequest (path : String)
(body : Body.Any := Body.Any.ofBody ({} : Body.Empty)) : Request Body.Any :=
{ (Request.new |>.method .post |>.uri! path |>.header! "Host" "example.com").body body with }
private def sendInBackground (connection : Connection) (request : Request Body.Any) :
Async (IO.Promise (Except Error (Response Body.Stream × IO.Promise (Except Error Unit)))) := do
let promise ← IO.Promise.new
background do
promise.resolve (← connection.sendTracked request)
pure promise
private def expectResponse
(promise : IO.Promise (Except Error (Response Body.Stream × IO.Promise (Except Error Unit)))) :
Async (Response Body.Stream × IO.Promise (Except Error Unit)) := do
match ← await promise.result! with
| .ok result => pure result
| .error e => throw (IO.userError s!"expected a response, got {e}")
private def readHead (peer : Mock.Server) : Async String := do
let mut bytes := ByteArray.empty
repeat
if (String.fromUTF8! bytes).contains "\r\n\r\n" then break
let some chunk ← peer.recv?
| throw (IO.userError "connection closed before the request head")
bytes := bytes ++ chunk
pure (String.fromUTF8! bytes)
private def errorConstructor : Error → String
| .connect _ => ".connect"
| .timeout => ".timeout"
| .closed _ => ".closed"
| .protocol _ => ".protocol"
| .bodyLimitExceeded => ".bodyLimitExceeded"
| .invalidRequest _ => ".invalidRequest"
| .io _ => ".io"
/-!
The read timeout bounds waits for network data, not time spent waiting for the caller to consume
body bytes that have already arrived.
-/
#eval show IO _ from runWithTimeout "buffered response body survives readTimeout" 3000 <|
Async.block do
let (client, peer) ← Mock.new
let connection ← Connection.new client
({ readTimeout := ⟨100, by decide⟩, requestTimeout := ⟨2000, by decide⟩,
keepAliveTimeout := ⟨2000, by decide⟩ } : Client.Config)
try
let result ← sendInBackground connection (mkRequest "/buffered")
discard <| readHead peer
peer.send (rawResp "200 OK" #[("Content-Length", "2")] "ok")
let (response, _) ← expectResponse result
sleep 250
let body : String ← response.body.readAll
unless body == "ok" do
throw (IO.userError s!"expected buffered body \"ok\", got {body.quote}")
finally
connection.close
/-!
An error raised by a request-body producer is an `.io` failure. It must not be collapsed into the
retryable `.closed` constructor merely because it arrives through a selector.
-/
#eval show IO _ from runWithTimeout "request body selector preserves producer error" 3000 <|
Async.block do
let (client, peer) ← Mock.new
let connection ← Connection.new client
({ readTimeout := ⟨2000, by decide⟩, requestTimeout := ⟨2000, by decide⟩ } : Client.Config)
try
let stream ← Body.mkStream
stream.setKnownSize (some (.fixed 1))
let result ← sendInBackground connection
(mkRequest "/body-error" (Body.Any.ofBody stream))
discard <| readHead peer
-- Let the connection register the request-body selector before failing the producer.
sleep 50
stream.closeWithError (IO.userError "producer failed")
match ← await result.result! with
| .error (.io _) => pure ()
| .error e =>
throw (IO.userError s!"expected .io, got {errorConstructor e}")
| .ok _ =>
throw (IO.userError "expected the request-body failure to reject the request")
finally
connection.close
end ClientConnectionLifecycleTests| private inductive Recv | ||
| | bytes (x : Option ByteArray) | ||
| | requestBody (x : Option Chunk) | ||
| | bodyInterest (x : Bool) |
There was a problem hiding this comment.
Optional: this is a private type, so it's fine, but reviewing this would be easier if the variable names weren't all x :)
| -/ | ||
| private def stopAcceptingRequests (requestChannel : Std.CloseableChannel PendingRequest) : | ||
| IO Unit := do | ||
| try requestChannel.close catch _ => pure () |
There was a problem hiding this comment.
This seems like it should be a proper tryClose function on CloseableChannel instead.
| A request queued to the background connection loop, paired with the promises that deliver its | ||
| outcome. | ||
| -/ | ||
| structure PendingRequest where |
There was a problem hiding this comment.
I think most of the material in this file should be considered internal implementation, but it is not marked as such.
| selectables := selectables.push | ||
| (.case responseBody.interestSelector (pure <| .bodyInterest ·)) | ||
|
|
||
| try Selectable.one selectables catch _ => pure .close |
There was a problem hiding this comment.
Do we just lose all error information here? Seems quite crude.
| Transport.close socket | ||
|
|
||
| /-- | ||
| Queues a request and awaits its response, together with a completion promise that |
There was a problem hiding this comment.
Does it really "await" the response?
| -/ | ||
| def sendTracked (connection : Connection) (request : Request Body.Any) | ||
| (requestOverrides : RequestOverrides := {}) : | ||
| Async (Except Error (Response Body.Stream × IO.Promise (Except Error Unit))) := do |
There was a problem hiding this comment.
This is a super complicated return type which needs to be simplified or at least explained in the docstring.
|
|
||
| /-- | ||
| Retires the connection without disturbing the exchange running on it: no further request is | ||
| accepted, and the background loop shuts down once it next goes idle. `isClosed` reports `true` |
There was a problem hiding this comment.
This should clarify that pending requests are still handled, not just the currently in flight request.
This PR adds a
Connectionmodule that bridges the async runtime with the H1 state machine with with aSessionstructure that manages the lifecycle of a singleConnectionhandling both outgoing requests and incoming responses.