Reuse idle connections across requests

[toots]

Every request opened its own socket and paid a TCP and TLS handshake for it, although S3 and the S3-compatible endpoints all speak HTTP/1.1 keep-alive.

Per @andersfugmann's review, the http client is a module rather than something baked into the core. Types.Http is a one-value signature (call, plus the Io it is built over); S3.Make_http, Aws.Make_http and Credentials.Make_http take it, and S3.Make(Io) is just S3.Make_http(Http.Make(Io)), so nothing existing changes:

module S3 = Aws_s3.S3.Make(Io)                     (* built-in client, as before *)
module S3 = Aws_s3.S3.Make_http(My_cohttp_client)  (* your own http library *)
module S3 = Aws_s3.S3.Make_http(Aws_s3.Http.Make_pooled(Io)(struct
  let max_idle_per_host = 4
  let max_idle_total = 8
  let max_idle_age = 60.
end))                                              (* built-in client, your limits *)

That answers the hard-coded 32: every pool limit is a functor parameter now, and Http.Make is Make_pooled applied to Default_pool_config. max_idle_total bounds the pool across endpoints, since an endpoint that is never revisited is never swept -- reaching it drops the oldest idle connection anywhere in the pool, which needs no timer and no background task in a library with no shutdown hook. Aws_s3.Http and Aws_s3.Headers are exported, because without them nobody outside the library could write a Types.Http.

A connection is only pooled when the response was framed well enough for its body to have been fully consumed and neither side asked for Connection: close. Peers reap idle connections on their own schedule -- Backblaze B2 aggressively so -- so an entry idle past max_idle_age is dropped rather than handed out, and a failure on a reused connection is retried once on a fresh one, but only for an idempotent bodyless request whose sink is still untouched.

The Copilot findings are addressed: retries restricted to idempotent methods (a bodyless POST could duplicate a multipart upload) and to an untouched sink; framing requires Transfer-Encoding to be exactly chunked, with Body.chunked_transfer consuming a non-empty trailer section; a request-side Connection: close honoured; 204/304 pooled rather than discarded; and the pool bounded globally as above.

Tests live in test/ and need no external service. pooling.ml counts the connections a canned server accepts -- one for three pooled requests, three with max_idle_per_host = 0, none extra when the peer drops a reused socket mid-body, and a reconnect once a second endpoint displaces the first under max_idle_total = 1. custom_http.ml drives S3.delete through a stub http module, so the seam breaks the build if it stops being implementable from outside. integration.sh against the bundled minio passes: upload, head, download, Expect: 100-continue, chunked upload, streaming download, multipart upload.

The first commit is an independent lwt fix that reuse made visible: a write raising EPIPE on a peer-closed socket escaped through Lwt.async and left the in-flight request waiting on a response that could never arrive, and a TLS shutdown over a dead socket did the same while leaving the fd in CLOSE-WAIT.

[Copilot]

Copilot review overview

🟡 Changes recommended

Response framing and retry logic can corrupt responses, duplicate POST operations, and retain idle sockets indefinitely.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 1 · 3

New issues introduced by this change (4)

Severity	Finding
	aws-s3/​http.ml — This treats every Transfer-Encoding value as safely framed, but read_data only consumes an…
	aws-s3/​http.ml — A bodyless request is not necessarily replay-safe. Multipart.initiate issues a bodyless POST…
	aws-s3/​http.ml — pool_queue permanently inserts a queue for every endpoint, and idle age is checked only when that…
	aws-s3/​http.ml — The pooling decision ignores the request's Connection header. Http.call publicly accepts…

What changed in this PR

Adds persistent HTTP/1.1 connection pooling while improving Lwt socket-failure cleanup.

Changes:

• Pools idle connections per endpoint with configurable expiry.
• Retries bodyless requests after reused-connection failures.
• Handles Lwt write and shutdown failures more safely.

File	Description
aws-s3/​s3.mli	Documents connection reuse.
aws-s3/​http.ml	Implements pooling, expiry, and retries.
aws-s3-lwt/​io.ml	Improves failed-socket cleanup.

Suppressed comments (2)

aws-s3/http.ml:259

• An Error may occur after Body.transfer has already written part of a GET response to sink; an error-status path can also close that sink before failing. Re-running with the same sink then appends a complete second response to partial data or writes to a closed sink. Retry only failures known to occur before response processing, or return the error so the caller can retry with a fresh sink.

    | Error _ when reused && body = None ->
      discard_conn conn;
      Net.connect ?connect_timeout_ms ~inet:endpoint.inet ~host:endpoint.host
        ~port:endpoint.port ~scheme:endpoint.scheme () >>= (function
        | Ok conn -> run conn >>= fun result -> finish conn result

aws-s3/http.ml:230

• Responses whose status forbids a message body are also fully consumed without Content-Length or Transfer-Encoding. In particular, S3 DELETE commonly returns 204 No Content, so this predicate unnecessarily discards that healthy connection. Pass the status code into this predicate and treat 204/304 responses as framed (along with HEAD).

    let framed =
      meth = `HEAD
      || Headers.find_opt "content-length" headers <> None
      || Headers.find_opt "transfer-encoding" headers <> None
    in

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[andersfugmann]

Thanks for this, and I do see why this is useful.
I'm not sure about the solution though. I think a better solution would be to pass a http module, that would handle the connection pooling. This would also allow users to choose whatever http library they want, and configure number of connections to be cached. But this would be quite an invasive change.
What concerns me is the hard-coded constant of 32 connections per host.
Any thoughts on this?

[toots]

Thanks for this, and I do see why this is useful. I'm not sure about the solution though. I think a better solution would be to pass a http module, that would handle the connection pooling. This would also allow users to choose whatever http library they want, and configure number of connections to be cached. But this would be quite an invasive change. What concerns me is the hard-coded constant of 32 connections per host. Any thoughts on this?

That sounds like a good separation of concern to me too. Let me see if that can be done easily.

[toots]

@andersfugmann let me know what you think now!