Skip to content

composable connection pool - #4708

Merged
aajtodd merged 47 commits into
connection-pool-mainfrom
aajtodd/http-conn-pool
Jun 26, 2026
Merged

composable connection pool#4708
aajtodd merged 47 commits into
connection-pool-mainfrom
aajtodd/http-conn-pool

Conversation

@aajtodd

@aajtodd aajtodd commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds an opt-in HTTP client built on hyper-util's composable connection pools, exposed under aws_smithy_http_client::pool. It provides connection-count limits, connection lifecycle events and connection-state queries, and a partition model that binds connections to a driver runtime and an optional network interface while sharing one global connection budget. The default client and the existing Builder are unchanged.

Motivation

  • Connection bounding. The pool-based client has no connection-count limit. A process under high concurrency can open unbounded sockets and exhaust file descriptors or overwhelm the peer (SIM P407972172). This adds max_connections and max_connections_per_host as connection-level caps — distinct from request concurrency, which H2 multiplexing decouples from connection count.
  • hyper-util composable pools. hyper-util's legacy::Client is positioned for eventual removal. Its replacement is a set of composable pool layers — cache, singleton, negotiate, map (overview). This builds the client on those layers, which is also where connection-count limiting becomes expressible (a tower concurrency limit at the connection-establishment layer rather than a request gate).
  • Connection-state visibility and locality. A single shared pool can hold a cross-connection view and bind connections to a runtime and interface. N independent pools cannot: each sees only its own connections, and a per-pool connection cap fragments the global budget. The lifecycle events, the connection-state read API, and the partition model below all follow from holding that view in one place.

Scope and compatibility

  • Additive. New public surface under aws_smithy_http_client::pool; one additive field (ConnectionId) on aws-smithy-runtime-api's ConnectionMetadata.
  • The default HTTP client selection is unchanged. No behavior version gates on this. Existing Builder::new() and its connectors are untouched.
  • Opt-in: a consumer reaches it through pool::SharedPool::builder().

Approach

The existing client's observable behavior was captured as a test suite before the pool was written, and the pool was built against it. The suite is therefore both a parity gate — the pool must not regress connection reuse, idle eviction, timeout, poisoning, or proxy behavior — and the specification the implementation targets. The net-new partition surface is tested on top of that baseline. This is why roughly a third of the diff is tests, and why the wire harness (below) is itself substantial: the behavior under test lives at the socket, so the harness had to come first.

Model

A pool serves many authorities; per authority, a request flows through a stack of hyper-util layers. Reuse short-circuits before the connection limit and the handshake; only new connections take the full path.

Connector stack (per authority)

request
  │
  ▼
Map ───────────────── route by (scheme, authority)
  └─ Negotiate ────── select leg by ALPN result
       ├─ H1: Cache ─────────── reuse idle, else ↓
       └─ H2: Singleton ─────── reuse shared conn, else ↓
                          │
                   ConnectionLimit ──── acquire per-host then global permit
                          │              (reuse bypasses this entirely)
                          ▼
                     handshake ──────── driver task spawned on the
                          │             partition's runtime
                          ▼
                        TLS ─────────── TCP

Ownership: per-partition vs shared

Connections never move: a connection is a driver task pinned to the runtime that created it plus a socket bound to that runtime's reactor and a NIC. So storage is per-partition. The budget and the connection-state index are shared, which is what makes one global cap across partitions possible.

SharedPool                       cloneable handle
└─ ConnectionPool
   ├─ shared state ............. global_sem · per_host_sems ·
   │                             connection-state index · event hooks
   ├─ registry (immutable) ..... partitions by id, partitions by NIC group
   └─ partitions
        └─ PartitionState ...... driver spawner · optional NIC
             └─ per-authority storage
                  └─ Negotiate(Cache | Singleton) + connection counters

Client ──▶ one PartitionState    resolved once at construction;
                                 the reuse hot path takes no global lock

The single-partition default is this same structure with one anonymous partition: no NIC groups, no peers, the cap is just that partition's.

Public API and use cases

Path: aws_smithy_http_client::pool::*. The configuration surface is SharedPool (built via SharedPool::builder()); Client is a lightweight handle that implements HttpClient.

1. Bound connection use

max_connections caps total connections; max_connections_per_host caps per authority. Acquired only when establishing a connection — reuse and H2 multiplexing do not consume the budget. Per-host is acquired before global, so a saturated authority blocks only its own connects.

let pool = SharedPool::builder()
    .tls_provider(tls::Provider::Rustls(CryptoMode::AwsLc))
    .max_connections(125)
    .max_connections_per_host(10)
    .build_https();
let client = Client::new(&pool);

Prevents a process from exhausting sockets or file descriptors under high concurrency. See examples/pool-basic.rs.

2. Observe connections

A ConnectionEventListener receives created / reused / closed / failed events carrying a stable ConnectionId, the authority, the negotiated protocol, a close reason, and connect timing. stats(&authority) returns a point-in-time, per-partition read of connection counts. Both are read-only; neither couples the pool to a metrics crate.

struct Metrics;
impl ConnectionEventListener for Metrics {
    fn on_created(&self, event: &ConnectionCreatedEvent) {
        record(event.conn_id(), event.authority(), event.protocol());
    }
    fn on_closed(&self, event: &ConnectionClosedEvent) {
        record_close(event.authority(), event.reason());
    }
}

let pool = SharedPool::builder()
    .tls_provider(tls::Provider::Rustls(CryptoMode::AwsLc))
    .connection_event_listener(Arc::new(Metrics))
    .build_https();

let stats = pool.stats(&Authority::from_host("example.com"));
for (partition, s) in stats.iter() {
    // established: connections that exist; idle(): those not in use.
    report(partition, s.established, s.idle());
}

Surfaces connection churn and cache-hit behavior, and attributes failures to a connection.

3. Keep I/O local on topology-aware runtimes

A partition is a group of connections that share a driver runtime and an optional network interface. On a current-thread-per-core runtime, a connection's driver task is pinned to the runtime that created it; using a connection from another runtime costs a cross-thread wakeup per request. One partition per runtime keeps each connection's I/O on its owning runtime, while a single shared pool still enforces one global connection budget — which N independent per-runtime pools cannot.

Partition::interface(nic) binds that partition's sockets to a network interface (SO_BINDTODEVICE on Linux), so each partition's connections egress its declared NIC. A partition builds its own connector bound to its NIC; the binding is also what defines the NIC groups borrow and reclaim respect (use case 4).

   core 0 runtime          core 1 runtime          core 2 runtime
   partition 0             partition 1             partition 2
   ├─ connections          ├─ connections          ├─ connections
   │  (drivers pinned)     │                        │
   └───────────────┬───────┴────────────┬───────────┘
                   one shared connection budget
let pool = SharedPool::builder()
    .tls_provider(tls::Provider::Rustls(CryptoMode::AwsLc))
    .max_connections(2_000)
    .partitions((0..n).map(|i| {
        Partition::new(PartitionId::from_index(i), spawner_for(i))
    }))
    .build_https();
let client = Client::from_partition(&pool, PartitionId::from_index(i));

See examples/pool-partitioned.rs.

4. Cross-partition behavior under a cap

When a partition has no local idle connection and the cap binds, CrossPartitionPolicy governs what happens. Both behaviors fire only under a binding cap and only within a NIC group — borrowing or reclaiming across a NIC is physically wrong, since the peer's connection is on the wrong interface.

let pool = SharedPool::builder()
    .tls_provider(tls::Provider::Rustls(CryptoMode::AwsLc))
    .max_connections(2_000)
    .cross_partition_policy(CrossPartitionPolicy::PreferLocal)
    .partitions([
        Partition::new(PartitionId::from_index(0), spawner_for(0)).interface("eth0"),
        Partition::new(PartitionId::from_index(1), spawner_for(1)).interface("eth1"),
    ])
    .build_https();
  • Never (default) — strict locality. The partition reclaims a peer's permit (then connects locally) or waits; it always serves the request on its own connection.
  • PreferLocal — borrow. The partition dispatches the request through a same-NIC peer's existing connection to bridge a transient burst, with no handshake and no permit.

Cap-bound decision (synchronous, per request)

A single decision per request, rooted at the permit acquire:

                      request, local cache miss
                                │
                        try_acquire permit
                                │
              ┌─────────────────┴──────────────────┐
          acquired                            NoPermits  (cap bound)
              │                                     │
        connect locally           binding constraint = PerHost | Global
      (no cross-partition path)                     │
                            ┌─────────────────┬──────┴─────────────┐
                       policy Never                      policy PreferLocal
                            │                                     │
                         RECLAIM                               BORROW
              candidates: same-NIC peers          candidates: same-NIC peers
              the index shows holding idle         the index shows holding idle
              (rotate by peer_cursor)              (rotate by peer_cursor)
                            │                                     │
              first peer whose idle pops+         first peer whose idle pops:
              drops frees a permit:               dispatch request through the
                PerHost → free idle to that       PEER's connection (its driver
                  authority                       stays on the peer's runtime)
                Global → free any peer's idle                    │
                  (permit is fungible)            no peer idle ──▶ falls back to
                            │                       the Never blocking acquire
              blocking-acquire freed permit
                ──▶ connect LOCAL
                            │
              none reclaimable ──▶ block;
              eviction is the fallback ──────────▶ (see eviction diagram)

The index only narrows candidates (advisory); the cache pop is the authoritative gate. A stale index entry can only shrink the candidate set — never cause a wrong action.

Cap-bound fallback (asynchronous, eviction tick)

When Never finds no reclaimable peer idle, the starved partition unblocks through the shared semaphore on the next eviction tick — not through a shared eviction view:

a single eviction task (one per pool) walks ALL partitions each tick:
  for each partition → each authority → retain idle
                                        (drop expired / poisoned)

  P1's idle connection expires ─▶ dropped ─▶ its ConnectionPermit returns
                                                 │
                                          global_sem += 1
                                                 │
        P0 blocked on global_sem (on another runtime) WAKES ─▶ connects LOCAL

Active reclaim does this synchronously at the cap point; eviction does it on the tick. The unblock rides the shared semaphore either way, so it works regardless of which partition evicts.

Testing

The behavior under test sits at the socket boundary — reuse, idle eviction, stale-connection detection, cap pressure, cross-partition borrow and reclaim are only observable there — so the transport is not mocked. Tests run against a purpose-built loopback wire harness: real TCP bound on distinct loopback IPs, per-connection programmable behavior (respond / reset / hold / idle-close), and a server-side event log of what each endpoint saw. The harness (src/test_util/wire/connection.rs) is the lens for the whole suite and is itself substantial net-new infrastructure.

By behavior class, with where each lives:

  • Pool behavior (parity) — reuse, idle eviction, stale connection detected at checkout, poisoning (a poisoned connection is not reused), connect and read timeouts, max_connections enforcement, and per-host cap isolation. tests/pool_behavior_test.rs.
  • Protocol (H2) — multiplexing on one connection, GOAWAY handling, poisoning, ALPN leg selection. tests/h2_pool_test.rs.
  • Topology (partitions) — per-partition spawner isolation, per-partition storage isolation, cross-partition reclaim (Never), cross-partition borrow (PreferLocal, asserting the request ran on the peer's exact connection), and the NIC-group boundary. tests/pool_behavior_test.rs.
  • Connection state — counters track the create / reuse / evict lifecycle; stats() reads are sparse (only touched partitions appear), reflect an in-flight checkout, and prune after eviction. tests/pool_behavior_test.rs.
  • Concurrency — a multi-partition stress test driving concurrent borrow / reclaim / eviction, run under ThreadSanitizer in the per-crate CI hook (additional-ci). The connection-state counters are advisory and relaxed — drift-tolerant, but not race-tolerant (a relaxed-atomic data race is still undefined behavior), which is what TSan guards. tests/pool_behavior_test.rs.
  • TLS and proxy (parity) — provider matrix and proxy routing, run for the pool-based client alongside the existing client. tests/tls.rs, tests/proxy_tests.rs.
  • Cross-stack — the orchestrator reconnect-on-transient-error suite runs against the pool-based client alongside the existing hyper stacks. aws-smithy-runtime/tests/reconnect_on_transient_error.rs.

How to review

The review follows a request through the pool, then covers the cross-cutting concerns a single request does not touch. The send path and the connection guards are where correctness concentrates.

The Model section above is the mental model; read it first. Sizes: ~12k insertions across 31 files; ~57% pool source, ~36% tests, the remainder vendored cache, TLS wiring, runtime-api, and build.

Tracing a request

The request enters. Client resolved its partition at construction, so the request goes straight to that partition's authority map. src/client/pool/client.rs, src/client/pool.rs (send_request).

Notes:

  • The partition is resolved once, at Client construction, not per request. The reuse hot path takes no global lock — storage is per-partition, and only the cap and the connection-state index are shared.

Reuse, or decide to connect. The Negotiate stack tries an idle H1 connection or the shared H2 connection before anything else; only a miss proceeds toward a new connection. src/client/pool.rs (the Negotiate assembly).

Notes:

  • Reuse short-circuits above the connection limit and the handshake — a cache hit acquires no permit and runs no handshake. The permit and handshake are only on the new-connection path.

The permit (the cap). A new connection acquires the per-host permit, then the global permit. src/client/pool/handshake.rs (ConnectionLimit).

Notes:

  • Per-host is acquired before global, deliberately: the reverse order lets a saturated authority hold global permits while waiting on its own, which would stall connects to unrelated authorities.
  • A failed try_acquire here is the entry to the cross-partition path (covered under Cross-cutting concerns).

Handshake and the driver. The connection is established and its driver task is spawned on the partition's runtime, not the ambient one that issued the request. The managed connection and its RAII guards are created here. src/client/pool/handshake.rs, src/client/pool/connection.rs.

Notes:

  • The guards are the subtle part. established is decremented exactly once across the N H2 clones of a connection — it rides the shared inner, not each clone, so dropping a multiplexed-stream handle does not under-count.
  • The establishing guard uses promote-vs-drop: a connect that is cancelled before it completes drops the guard and decrements, so a cancelled handshake does not leak a permanent "warming" count.

Response, then return. The connection returns to the pool only when the response body guard drops. src/client/pool/connection.rs (the body guard).

Notes:

  • The connection stays checked out until the body is fully consumed (the guard rides the response body), so a half-read body does not return a connection that is still in use.
  • H1 holds the connection for the single stream; H2 releases per-stream while the connection itself stays multiplexed.

Cross-cutting concerns

Connection identity and events. A stable ConnectionId and the lifecycle listener (created / reused / closed / failed). aws-smithy-runtime-api/src/client/connection.rs (the additive ConnectionId — semver-relevant public surface), the event types in src/client/pool/connection.rs.

Notes:

  • The runtime-api change is purely additive: a new field on ConnectionMetadata and its builder, no change to existing signatures.

Connection state. The counters and the per-(partition, authority) index behind stats(). src/client/pool/stats.rs.

Notes:

  • The index is advisory and relaxed by design. A stale read costs a missed routing optimization, never correctness — a request dispatched on a stale hint still succeeds. This is what lets the read path stay lock-free.

Cross-partition behavior. The cap-bound decision (the two diagrams above), the reclaim and borrow handles, and the policy. src/client/pool.rs (the cap-bound branch and the handles), src/client/pool/partition.rs (CrossPartitionPolicy, NIC grouping).

Notes:

  • Candidates are NIC-scoped: borrow and reclaim only consider same-NIC-group peers, because a peer's connection physically lives on the peer's interface.
  • The connection-state index only narrows the candidate set; the cache pop is the authoritative gate. A stale index entry can shrink the candidate set but cannot cause a wrong action.

Supporting material

Lower scrutiny, in support of the above:

  • src/client/pool/vendored_cache.rs — vendored from hyper-util with two additions marked // SDK MODIFICATION; see NOTICE.
  • src/client/pool/builder.rs — each partition builds its own connector via a Fn(&Partition) -> Connector factory; bind_interface is the single seam that applies set_interface (NIC binding), guarded to Linux-like targets. The factories differ only in their TLS/proxy wrap.
  • src/client/tls/{rustls,s2n_tls}_provider.rs — connector wrapping for connect timing.
  • src/client.rs, src/client/timeout.rs, src/client/proxy.rs — visibility changes, shared proxy-auth helper, timeout helper.
  • tests/* — grouped by the behavior classes in Testing above; read the harness (src/test_util/wire/connection.rs) first, as it is the lens for every test.

Not in this PR

Deferred, tracked separately:

  • DNS resolver feedback (caching, shuffling, failed-IP deprioritization).
  • ECONNRESET first-read detection and zero-delay retry.
  • Switching the default HTTP client to this implementation behind a behavior version.
  • Per-phase connect timing (TCP vs TLS) on connection events.

@github-actions

Copy link
Copy Markdown

A new generated diff is ready to view.

A new doc preview is ready to view.

* SPDX-License-Identifier: Apache-2.0
*/

//! HTTP connection pool.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Quick thought (just reading the description now, haven't dived into the code yet), but do we want to focus on the pool here? I know that is our main motivator for shipping this, but I think hyper wants to include more of this composable functionality in the future that isn't just focus on the pool. Maybe composable would be a better module name?

Maybe it comes down to whether we envision having multiple clients that are focused on different use cases, or just one more configurable/composable client that we expand on in the future.

@ysaito1001 ysaito1001 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed primarily around

  • static ownership relationships between key players (e.g. SharedPool, ConnectionPool, SharedPoolState, PartitionRegistry, PartitionState, TypedPoolEntry, ConnectionLimit)
  • runtime behaviors around
    • establishing a new connection
    • connection cache hit
    • per-host connection max reached
    • global connection max reached

Looks fantastic. Could be review fallout, but the change carries lots of value as-is.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Modifications can be useful for hyper_util users in general. Plan to upstream these to hyper-util and remove the vendored copy at some point?

pool_idle_timeout = ?timeout,
"pool: eviction task spawned"
);
tokio::spawn(eviction_task(weak, rx, timeout));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks like it's a bare invocation of tokio::spawn in a non-feature-gated place like rt-tokio, but I suspect this place assumes async runtime being used is tokio by virtue of using hyper-util?

@landonxjames landonxjames left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved. One maybe bug with H1 connections you should probably look at before merging, other things are mostly just questions.

Overall feedback, this is a ton of code to own for what we get out of it. Agree with Yuki that we should look into upstreaming whatever parts of this we can.

}
}

fn is_empty(&self) -> bool {

@landonxjames landonxjames Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think an H1 host entry can be evicted while a request is still in flight. Meaning a healthy
connection is dropped instead of reused.

retain_idle (line 1074) removes a host entry when is_empty() is true, and the H1 retainer's is_empty is Cache::is_empty() -> shared.services.is_empty(). A checked-out H1 connection has been take() out of services, so for a host whose only connection is mid-request, services is empty -> the entry is removed and the Cache (Arc<Mutex<Shared>>) drops. The in-flight checkout holds only a Weak, so when the body finishes, Cached::Drop's shared.upgrade() returns None and the connection is dropped rather than returned.

H2 doesn't seem to have the same issue (the connection stays in Singleton).

I think this could be fixed by adding a check to this function like:

fn is_empty(&self) -> bool {
    // an entry with in-flight checkouts (`active`) or in-progress connects
    // (`establishing`) must not be removed: a checked-out H1 connection is
    // not in the cache idle set, so the retainers report empty while it is
    // still out. Removing the entry drops the cache it would return to.
    if self.counters.active.load(Ordering::Relaxed) > 0
        || self.counters.establishing.load(Ordering::Relaxed) > 0
    {
        return false;
    }
    let retainers = self.retainers.lock().expect("retainer slot poisoned");
    retainers.iter().all(|r| r.is_empty())
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Quick tests for this added in rust-runtime/aws-smithy-http-client/tests/pool_behavior_test.rs

/// Repro: hold an H1 response (body undrained, connection checked out, so the
/// H1 cache's idle set is empty) across an eviction tick, then drain it and
/// issue a second request to the same host. If the host entry is removed while
/// the request is in flight, the checked-out connection cannot return to its
/// (dropped) cache, so request 2 must reconnect (tcp_accepted == 2).
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
async fn review_repro_h1_entry_eviction_during_in_flight_request() {
    use http_body_util::BodyExt;

    let harness = ConnectionTestHarness::builder()
        .endpoint(
            IP1,
            vec![
                ConnectionBehavior::RespondKeepAlive { status: 200, body: b"one" },
                ConnectionBehavior::RespondKeepAlive { status: 200, body: b"two" },
            ],
        )
        .build()
        .await;

    let idle_timeout = Duration::from_millis(100);
    let pool = SharedPool::builder()
        .dns_resolver(harness.dns_resolver())
        .pool_idle_timeout(idle_timeout)
        .build_http();

    let port = harness.endpoints[0].port();
    let url = format!("http://127.0.0.1:{port}/");
    let client = PoolClient::new(&pool).into_shared();

    // Request 1: hold the response without draining, connection checked out.
    let resp1 = send_to(&client, &url).await.expect("req1 should succeed");
    assert_eq!(resp1.status().as_u16(), 200);

    // Let the eviction task tick (>= 2 ticks) while the request is in flight.
    tokio::time::sleep(idle_timeout * 3).await;

    // Drain req1's body: the body guard drops, CachedConnection::Drop fires.
    let _ = BodyExt::collect(resp1.into_body()).await.expect("body1 readable").to_bytes();
    tokio::task::yield_now().await;

    // Request 2 to the same host.
    let resp2 = send_to(&client, &url).await.expect("req2 should succeed");
    assert_eq!(resp2.status().as_u16(), 200);
    let _ = BodyExt::collect(resp2.into_body()).await.expect("body2 readable").to_bytes();

    let accepts = harness.tcp_accepted_count();
    eprintln!("[review-repro] tcp_accepted_count = {accepts}");
    assert_eq!(accepts, 2, "Finding #1: entry evicted in-flight -> req2 reconnects");
}

/// Control: same setup, no eviction tick (60s idle timeout) -> req2 reuses (1 accept).
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
async fn review_control_h1_in_flight_no_eviction_reuses() {
    use http_body_util::BodyExt;

    let harness = ConnectionTestHarness::builder()
        .endpoint(
            IP1,
            vec![
                ConnectionBehavior::RespondKeepAlive { status: 200, body: b"one" },
                ConnectionBehavior::RespondKeepAlive { status: 200, body: b"two" },
            ],
        )
        .build()
        .await;

    let pool = SharedPool::builder()
        .dns_resolver(harness.dns_resolver())
        .pool_idle_timeout(Duration::from_secs(60))
        .build_http();

    let port = harness.endpoints[0].port();
    let url = format!("http://127.0.0.1:{port}/");
    let client = PoolClient::new(&pool).into_shared();

    let resp1 = send_to(&client, &url).await.expect("req1 should succeed");
    assert_eq!(resp1.status().as_u16(), 200);
    let _ = BodyExt::collect(resp1.into_body()).await.expect("body1 readable").to_bytes();
    tokio::task::yield_now().await;

    let resp2 = send_to(&client, &url).await.expect("req2 should succeed");
    assert_eq!(resp2.status().as_u16(), 200);
    let _ = BodyExt::collect(resp2.into_body()).await.expect("body2 readable").to_bytes();

    let accepts = harness.tcp_accepted_count();
    eprintln!("[review-control] tcp_accepted_count = {accepts}");
    assert_eq!(accepts, 1, "control: no eviction -> req2 reuses the connection");
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch, added tests and fix.

if let Some(cached) = self.inner.take() {
let managed = cached.inner();
let conn_id = managed.conn_id;
if managed.is_poisoned() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Think there should be an on_closed() here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There is below.

if let Some(interface) = nic {
tcp.set_interface(interface);
}
let _ = nic;

@landonxjames landonxjames Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Probably worth a tracing::warn here (or maybe even some kind of failure) for users on unsupported platforms (Mac/Windows) who set a nic. The silent drop seems like surprising behavior.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Was missing the cfg guards at the configuration level, now you can only set a nic on linux which was the intent and how current configuration has it.

/// when the pool is at capacity; existing connections must be evicted
/// or closed before another can be created.
///
/// Should be at least [`max_connections_per_host`](Self::max_connections_per_host)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is it worth confirming the requirement at runtime?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Logged at build time now.

}

/// Construct a `Client` targeting a specific declared partition.
/// Panics if `id` was not declared on the pool builder (programming

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could this be a Result instead of a panic?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I left it as is for now. On one hand I agree on the other you defined the topology, punting for now, can revisit before main.

fn is_singleton_canceled(err: &(dyn std::error::Error + 'static)) -> bool {
let mut e = Some(err);
while let Some(cur) = e {
if cur.to_string() == "singleton connection canceled" {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Error matching on the exact string value feels sketchy, will break unexpectedly is hyper ever updates this error. But maybe there is no way to get something better to match on?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, I opened hyperium/hyper#4119 for this, this is a workaround currently.

@aajtodd
aajtodd requested review from jlizen and rcoh June 23, 2026 23:48
@aajtodd
aajtodd changed the base branch from main to connection-pool-main June 26, 2026 13:34
aajtodd added 2 commits June 26, 2026 10:03
…dation

Fix three issues raised in PR review of the composable connection pool.

H1 host entry evicted during an in-flight request
  A checked-out H1 connection is taken out of the cache's idle set, so the
  H1 retainer reports the host entry empty while the connection is still
  out. retain_idle then removes the entry and drops its cache; the
  in-flight checkout holds only a Weak, so on body drain the connection is
  dropped instead of returned. TypedPoolEntry::is_empty now reports the
  entry non-empty while active or establishing counters are non-zero, so an
  entry with in-flight checkouts or in-progress connects is never evicted.
  The counters field, previously write-only, gains this reader. Adds a
  characterization test (an in-flight request held across an eviction tick
  reuses its connection) and a no-eviction control.

NIC interface setter available on platforms that cannot bind
  Partition::interface accepted an interface on every target, but the bind
  only applies on Android, Fuchsia, and Linux; elsewhere it was silently
  ignored. Gate the setter to those targets, matching the v1 connector's
  set_interface. The nic field stays cross-platform: it is also the
  cross-partition borrow-group key, which is platform-independent. Tests
  and the example that use interface() as a group label are gated to match.

max_connections below max_connections_per_host
  A global cap below the per-host cap clamps every host to the global
  value, so the per-host limit can never be reached. build_pool logs a
  warning when both are set and the global is lower; the per-host setter
  documents the clamp.

sdk-lints compatibility for vendored_cache.rs
  The copyright check scans the first ten lines; the vendored file's MIT
  attribution preamble pushed the Amazon header past that window, so it
  read as missing. Move the Amazon header above the preamble. The file's
  TODOs are upstream's and kept verbatim, so add it to the todos
  IGNORE_DIRS rather than reword them.
Pick up runtime crate version bumps in aws/rust-runtime (aws-credential-types,
aws-runtime, aws-smithy-* and friends) and a transitive socket2 0.6.3 -> 0.6.4
bump in rust-runtime. Both lockfiles verified consistent with `cargo --locked`.
@aajtodd
aajtodd marked this pull request as ready for review June 26, 2026 14:51
@aajtodd
aajtodd requested review from a team as code owners June 26, 2026 14:51
@aajtodd
aajtodd force-pushed the connection-pool-main branch from c14d4ef to 2972ba7 Compare June 26, 2026 15:16
@aajtodd
aajtodd merged commit 1fb129d into connection-pool-main Jun 26, 2026
@aajtodd
aajtodd deleted the aajtodd/http-conn-pool branch June 26, 2026 15:21
@aajtodd

aajtodd commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator Author

Upstream PR for vendored cache additions: hyperium/hyper-util#295

aajtodd added a commit that referenced this pull request Aug 13, 2026
## Motivation and Context

[smithy-rs#4708](#4708)
contains a connection-pool rewrite and the connection-level tests
developed alongside it. This PR extracts and expands those tests into
independent `aws-smithy-http-client` test infrastructure.

The contracts pin observable HTTP/1.1 and HTTP/2 behavior of the
existing `hyper_util::client::legacy::Client` pool so another client
implementation can run against the same expectations. No production
connector or pool code is changed.

## Design

### Per-connection scripts

The `wire-mock` feature exports
`aws_smithy_http_client::test_util::wire::connection`. Each accepted
socket receives one complete `ConnectionScript`; concurrent or
speculative connections cannot consume actions from a shared behavior
queue.

- `EndpointPlan` assigns queued, repeated, or unbounded scripts to each
loopback endpoint.
- `Http1Script` parses bounded HTTP/1.1 requests and emits typed
response sequences.
- `SocketScript` provides bounded reads, exact byte assertions, writes,
gates, delays, FIN, close, and TCP reset for transport-level cases.
- `ManualGate` coordinates client and server state and supports waiting
for multiple arrivals.
- `ConnectionEvent` records DNS lookups, accepts, parsed requests, and
close reasons. Socket lifecycle events carry stable connection IDs.
- Explicit shutdown joins listener and connection tasks. Background
script failures are returned by waits and shutdown.

The harness supports multiple loopback IP addresses on one port and
supplies a matching `ResolveDns` implementation. The README documents
macOS loopback aliases, and the multi-address self-test fails with setup
guidance when `127.0.0.2` is unavailable.

### Harness example

This plan holds the first accepted connection mid-response at
`body_gate`. Once the test observes the gate, it can issue a second
request while the first HTTP/1.1 connection is unavailable for reuse;
that request opens the second scripted connection and receives a TCP
reset. Releasing the gate lets the first response complete.

```rust
use aws_smithy_http_client::test_util::wire::connection::{
    BodyPlan, ConnectionScript, ConnectionTestHarness, EndpointPlan, Http1Response, Http1Script,
    ManualGate, SocketScript,
};
use std::net::{IpAddr, Ipv4Addr};

let body_gate = ManualGate::new();
let harness = ConnectionTestHarness::builder()
    .endpoint(
        IpAddr::V4(Ipv4Addr::LOCALHOST),
        EndpointPlan::queue([
            ConnectionScript::http1(Http1Script::serve(
                Http1Response::ok().body_plan(BodyPlan::split_at_gate(
                    "before",
                    body_gate.waiter(),
                    "after",
                )),
            )),
            ConnectionScript::socket(
                SocketScript::new()
                    .read_http1_request()
                    .reset(),
            ),
        ]),
    )
    .dns_all("service.test")
    .build()
    .await?;
```

A contract drives the plan in this order:

1. Configure the client with `harness.dns_resolver()` and send requests
to `service.test` on `harness.port()`.
2. Send the first request and retain its incomplete response body.
3. Await `body_gate.wait_until_reached(...)` so the server, rather than
elapsed time, proves the first connection is still occupied.
4. Send the second request; it opens the second scripted connection and
receives a TCP reset.
5. Call `body_gate.release()`, collect the first response body, and shut
down the harness.

### HTTP/1.1 contracts

Twenty contracts run against the `HyperUtilLegacyPool` adapter:

- reuse after complete responses, idle eviction, active response
ownership, and opening a second connection while a response body is
held;
- reuse after dropping an already-buffered chunk terminator versus
retirement when the response remainder is unavailable;
- stale idle replacement and `Connection: close`;
- origin-form request targets, `Host`, and origin isolation;
- connection metadata, local and remote addresses, and poisoning before
and during body completion;
- raw server errors that do not poison an otherwise reusable connection;
- TCP reset before a response, after a request, and during a response
body;
- clean EOF classification and response read timeout classification.

Each contract is an implementation-neutral function with an explicit
`test_*_with_hyper_util_legacy_pool` runner. Test names remain visible
to IDEs and the backend boundary remains explicit.

### HTTP/2 contracts

The integration-test support includes a private Rustls HTTP/2 server
built on `h2`. It assigns typed scripts per connection and per path,
records connection and stream events, observes GOAWAY frames, and joins
listener, connection, and stream tasks during shutdown.

Ten contracts cover:

- sequential reuse and multiplexing on an established connection;
- concurrent cold starts, including abandoned speculative handshakes and
convergence on one established HTTP/2 session;
- connection poisoning;
- stream-reset isolation and dropped-body `RST_STREAM(CANCEL)`;
- graceful GOAWAY while an accepted stream remains active, followed by
replacement;
- fully idle eviction and active-stream idle-timeout behavior;
- HTTP/2 ALPN and reuse with Rustls/AWS-LC, plus an equivalent s2n-tls
provider check.

The HTTP/2 test binary also contains two fixture self-tests for route
selection and fragmented GOAWAY observation.

## Changes

```text
.changelog/
  http-connection-test-harness.md                         NEW - public wire harness changelog entry
rust-runtime/aws-smithy-http-client/
  src/test_util/
    wire.rs                                               connection harness module wiring
    wire/connection.rs                                    NEW - public endpoint plans, HTTP/1.1 and socket scripts, gates, events, DNS, joined shutdown
    dvr.rs                                                align DVR-only test imports with the legacy-test-util feature
  tests/
    common/
      mod.rs                                              NEW - shared integration-test module wiring
      client.rs                                           NEW - backend identity, runtime components, bounded calls, response collection
      tls.rs                                              NEW - certificate, key, trust context, Rustls server, selectable ALPN
      h2.rs                                               NEW - private typed HTTP/2 connection and stream fixture
    connection_harness_test.rs                            NEW - 17 harness behavior and failure-propagation tests
    h1_connection_behavior_test.rs                        NEW - 20 HTTP/1.1 connection behavior contracts
    h2_connection_behavior_test.rs                        NEW - 10 HTTP/2 connection behavior contracts
    tls.rs                                                use shared TLS setup without changing existing scenarios
    proxy_tests.rs                                        gate TLS-only proxy helpers by TLS features
  Cargo.toml                                              wire-mock dependencies and crate version
  README.md                                               harness documentation and multi-address loopback setup
  additional-ci                                          comprehensive feature run and HTTP-only proxy compile check
rust-runtime/Cargo.lock                                   crate version, httparse, and socket2 lockfile entries
aws/sdk/Cargo.lock                                        crate version, httparse, and socket2 lockfile entries
tools/ci-scripts/
  test-windows.sh                                          run wire-mock harness and HTTP/1.1 contracts with Rustls/Ring
```

## Review Guide

1. Review `src/test_util/wire/connection.rs` with
`tests/connection_harness_test.rs` for the public scripting model, task
ownership, and failure handling.
2. Review `tests/h1_connection_behavior_test.rs` for the HTTP/1.1
observable contracts.
3. Review `tests/common/client.rs`, `tests/common/tls.rs`, and the
`tests/tls.rs` migration for shared integration support.
4. Review `tests/common/h2.rs` with
`tests/h2_connection_behavior_test.rs` for the HTTP/2 fixture and
contracts.
5. Review `Cargo.toml`, `README.md`, `additional-ci`,
`tools/ci-scripts/test-windows.sh`, and the proxy/DVR feature guards for
the public feature and CI boundaries.

## Testing

The following checks pass:

```text
cargo check -p aws-smithy-http-client
./additional-ci
cargo clippy --features wire-mock,rustls-aws-lc,s2n-tls --tests -- -D warnings
cargo fmt --all -- --check
```

The Windows workflow enables `wire-mock` with `rustls-ring` without
requiring AWS-LC. The same feature combination passes all 17 harness and
20 HTTP/1.1 tests locally; the Windows runner supplies platform
validation.

The comprehensive feature run discovers 30 unit tests, 17 harness tests,
20 HTTP/1.1 contracts, 12 HTTP/2 fixture and contract tests, 19 proxy
tests, 3 smoke tests, 5 TLS tests, and 18 doctests with one ignored.

The Rustls HTTP/2 test binary also passes ten consecutive serial runs.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._

---------

Co-authored-by: Landon James <lnj@amazon.com>
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.

3 participants