Repository navigation
HTTP client pool deadlines and stale ids, multipart encoder, host-built rustls configs and provider choice - #101
Merged
Conversation
turnloop_tls::ClientConfig and ServerConfig could only be built from roots or a chain, a key and ALPN, with rustls::crypto::ring hardwired as the provider. Everything else rustls can decide at config level (client certificates, SNI resolvers, custom verifiers, ticketers, protocol versions) was unreachable, so Perry's P5 lane bypassed the wrappers and P11 could not attach a client certificate to outbound requests. And a host that installs aws-lc-rs had no way to keep ring out of its binary. Wrapping an existing rustls config (#48): both configs gain `from_rustls(Arc<rustls::…Config>, HostTime)` and `rustls_config()`. The wrappers' only state besides the rustls config is the host-supplied wall clock, so that clock becomes the public `HostTime`: the host passes `host_time.time_provider()` to `builder_with_details`, and certificate validity keeps following the `unix_seconds` handed to `process`, exactly as with `new`. `new` is now built on `from_rustls`. A constructor over public fields was preferred because it keeps the crate's connection handling and time plumbing in one place. Choosing the provider (#53): `ClientOptions::provider` and `ServerConfig::with_provider` take any `Arc<CryptoProvider>`; `None` keeps the explicit ring provider, so current behaviour is unchanged. Selecting a provider alone would still link ring, so `ring` is now a default feature (`dep:ring` + `rustls/ring`). turnloop-tls declares rustls itself rather than inheriting the workspace entry, which enables ring for every member; the resolved rustls version and Cargo.lock are unchanged. Without `ring`, `new` uses the given provider or the rustls process default and returns an error if there is neither, and `tls_server_end_point` (which returns a ring Digest) is compiled out; the new provider-neutral `tls_server_end_point_hash` names the RFC 5929 hash for the host to compute. turnloop-http forwards the feature (`ring`, default on) and depends on turnloop-tls with default features off, since it never builds a config itself. `cargo tree -e normal -i ring` for turnloop-tls and turnloop-http with `--no-default-features` now prints nothing. Evidence (tests/tls.rs, tests/channel_binding.rs): - a client whose provider has only AES-128-GCM and a server whose provider has only ChaCha20 fail with NoCipherSuitesInCommon, and succeed when both carry ChaCha20; with ring hardwired they would have agreed on a suite; - a server built with from_rustls and a WebPkiClientVerifier receives exactly the client certificate a from_rustls client presents, and refuses a plain client with "peer sent no certificates"; the client's HostTime, created at 0, reads the time `process` supplied; - tls_server_end_point_hash agrees with the ring digest length for all 14 certificate fixtures (6 SHA-256, 4 SHA-384, 3 SHA-512, Ed25519 None). Breaking: ClientOptions has a new public field (struct literals need `..Default::default()`); turnloop-tls without default features no longer has `tls_server_end_point`. Closes #48 Closes #53
Two gaps in client::Pool left every consumer improvising. A ConnectionId from Acquire::Reuse could not be checked or retired (#51). When the host's socket table and the pool disagreed, the only recovery was a fabricated release-then-close sequence, which errors for an id the pool already dropped. The pool now has `contains(id)` and `forget(id)`: forget drops the connection whatever its state (outstanding acquisitions included, so the per-host place is free for the next acquire), clears its idle deadline and returns whether the id was live; a stale id is reported, not an error. Both were added rather than one: contains answers the question the host asks first, forget is the well-defined recovery. ConnectionId now documents that ids are never reused, so a stale id cannot alias a newer connection. There was no client-wide next_timeout (#52). Pool::next_timeout rescanned every slot, and request lifecycles were not covered at all, so a host with N requests in flight had to min() N structures per turn; Perry's P6 client skipped per-phase deadlines (connect/headers/body) for exactly that reason. The new `client::Deadlines<K>` is a keyed deadline heap: next_timeout is a peek, set and pop_expired are O(log n). Replaced deadlines are discarded lazily when they surface and the heap is rebuilt when they outnumber live entries, so its size stays within 2n+16 even for a body deadline refreshed on every chunk. The pool owns two of them: its idle deadlines (next_timeout and handle_timeout no longer scan slots) and host-registered request deadlines (`set_request_deadline(RequestId, lifecycle.next_timeout())`, drained with `handle_request_timeout`). Pool::next_timeout is the minimum of both, so one call arms the host's timer for every connection and request. The existing handle_timeout signature and behaviour are unchanged; a heap in the pool was chosen over a timer wheel because deadlines are arbitrary Instants the host supplies and no tick granularity is imposed. Evidence (tests/client.rs, client.rs unit test): - a Reuse id the host forgets is no longer contained, a second forget and a release report it stale, the per-host place goes to a new id, and a forgotten idle connection's deadline never fires; - with 50 idle connections and 100 registered request lifecycles one next_timeout reports the earliest deadline, follows a request moving to 10s and back out when its phase drops the deadline, expires the 50 idle connections and then exactly the two requests due, each yielding UND_ERR_HEADERS_TIMEOUT from its Lifecycle; - an Http1Connection's 100-continue and headers deadlines feed the pool; - 100 000 refreshes of one key keep the heap within bound and the refreshed key still fires last. Closes #51 Closes #52
turnloop_http had no multipart builder, which blocked Perry's CLI migration off reqwest: `perry publish`, `audit`, `verify` and `run --remote` all send multipart bodies, and P11 had to carry its own 280-line builder. `multipart::Form` holds ordered text and file parts (`Part::text`, `Part::file`, optional `content_type`; files default to application/octet-stream) and `encode(entropy)` returns the body plus its boundary; `Encoded::content_type()` gives the header value and `apply` installs both on a client::Request. Names and filenames are escaped as browsers do (`"`, CR, LF percent-encoded) and a content type with a control character is rejected, so no field can inject a header line. The boundary is verified, not just random. A base64 field can hold a boundary-shaped run, and a collision silently truncates the upload, so every candidate is checked against each part's bytes, name, filename and content type, and the next candidate is tried until none contains it. Entropy comes from the host as 16 bytes: the crate reads no randomness source (sans-I/O), and its dependencies offer none once ring is optional. Candidates are SplitMix64 outputs over distinct inputs and therefore pairwise distinct, so the search ends within (total part bytes + 1) tries. The format is "turnloop-" plus 32 hex digits, 41 characters with nothing to quote. In-memory encoding matches every current Perry caller (only Part::text is used; no streamed parts). Evidence (tests/multipart.rs): a four-part form (escaped name, CRLF in a value, binary file, typed file) encodes byte-for-byte to the expected body, deterministically per entropy; planting the first two candidates in a base64-looking field and a filename makes the encoder pick a third boundary whose delimiter occurs exactly three times, two openings and one closing; `apply` leaves exactly one content-type header; a CRLF content type is refused with UND_ERR_INVALID_ARG. Closes #78
|
Warning Review limit reachedNext included review available in 41 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Sep 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves five issues in the HTTP client (
protocols/turnloop-http/src/client.rs, newmultipartmodule) andturnloop-tls.#51: check or drop a stale
ConnectionIdPool::contains(id)reports whether an id still names a live connection.Pool::forget(id)drops it whatever its state, including outstanding acquisitions, so the per-host place is free again. It clears the idle deadline and returns whether the id was live. A stale id is reported, not an error, so a host whose socket table disagrees with the pool no longer has to make up a release-then-close sequence.ConnectionIdnow documents that ids are never reused.Closes #51
#52: one client-wide
next_timeoutNew
client::Deadlines<K>, a keyed deadline heap:next_timeoutis O(1), andset/pop_expiredare O(log n). Replaced entries are dropped lazily and the heap is rebuilt when they pile up, so its size stays within 2n+16.Poolowns two of these. One holds the idle deadlines, sonext_timeoutandhandle_timeoutno longer scan slots. The other holds request deadlines the host registers withset_request_deadline(RequestId, lifecycle.next_timeout())and drains withhandle_request_timeout.Pool::next_timeoutcovers both, so the host arms one timer per turn for every connection and in-flight request, and per-phase connect/headers/body deadlines no longer cost a rescan.Closes #52
#78:
multipart/form-databuilderturnloop_http::multipart::{Form, Part, Encoded}builds text and file parts in memory. It escapes names and filenames the way browsers do and rejects a content type that contains control characters.Encoded::applyputs the body and content type on aclient::Request. The boundary comes from 16 bytes of entropy that the host supplies (the crate stays sans-I/O and adds no new dependency). Each candidate boundary is checked against every part's bytes, name, filename and content type, and a new one is generated on collision. Candidates are pairwise distinct, so the search always ends. A test puts the first two candidates inside a base64-like field and a filename. It asserts that the chosen delimiter appears exactly once per part plus once as the closing delimiter.Closes #78
#48:
from_rustlson both TLS configsClientConfig::from_rustlsandServerConfig::from_rustlswrap anArc<rustls::…Config>that the host built itself, which covers client certificates, SNI resolvers, custom verifiers and ticketers. Both configs also getrustls_config(). The host-supplied clock is now the publicHostTime. Passhost_time.time_provider()tobuilder_with_detailsand certificate checks keep following the time given toprocess. Tested with mutual TLS: the server sees exactly the client's certificate, and a client without a certificate is refused.Closes #48
#53: host-chosen crypto provider,
ringoptionalClientOptions::providerandServerConfig::with_provideraccept any rustlsCryptoProvider. WithNone, the client keeps using explicit ring, so behaviour is unchanged.ringis now a default feature of turnloop-tls (dep:ring+rustls/ring). turnloop-http forwards it as its own defaultringfeature and depends on turnloop-tls with default features off, because it never builds a TLS config itself. turnloop-tls now declaresrustlsdirectly instead of inheriting the workspace entry, which turns on ring for every member. The resolved versions and Cargo.lock do not change. With--no-default-features,cargo tree -e normal -i ringprints nothing for either crate. In that modenewuses the provider the host passes or the rustls process default, and returns an error if there is neither.tls_server_end_point, which returns a ringDigest, is compiled out, and the new provider-neutraltls_server_end_point_hashtells the host which hash to compute. A test gives each side a different single-suite provider and checks that the handshake fails withNoCipherSuitesInCommon. The same test passes when both sides share the suite. deny.toml is unchanged, andfeature_modes.pyandcheck-paths.pypass.Closes #53
Breaking changes
turnloop_tls::ClientOptionshas a new public field,provider. Struct literals need..Default::default(). Every literal in the workspace already uses it.default-features = false, turnloop-tls has notls_server_end_pointand links no crypto provider. Default builds are unaffected.turnloop-httpdepends on turnloop-tls through its own defaultringfeature, not directly through the workspace entry.Checks
cargo +nightly-2026-08-20 fmt --all --checkcargo +nightly-2026-08-20 clippy --locked --workspace --all-targets --all-features -- -D warnings -D clippy::undocumented_unsafe_blocksRUSTDOCFLAGS='-D warnings' cargo +nightly-2026-08-20 doc --locked --workspace --all-features --no-depscargo +nightly-2026-08-20 test --locked -p turnloop-tls --all-featurescargo +nightly-2026-08-20 test --locked -p turnloop-http --all-featurescargo +1.97.1 check --locked --workspace --all-targets --all-featurespython3 scripts/ci/check-paths.py,python3 scripts/ci/feature_modes.pycargo +nightly-2026-08-20 check --locked -p turnloop-tls --no-default-featuresand-p turnloop-http --no-default-features --all-targets