Repository navigation
Add reusable Tokio TLS client connector - #550
Conversation
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
60a4538a.
This review, of 5e655f50
💬 Approved with comments — One WARNING: every new Connector registers a new global SSL ex-data index that is never freed, which slowly leaks and grows per-Ssl cost when connectors are created repeatedly. One INFO about a change in what capacity 0 means.
Findings: 1 warning · 1 info (2 are inline comments on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #550.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (1)
### [Code Quality] Every `Connector` registers a new process-global SSL ex-data index that is never freed. The old code used one static index.
- **File:** tokio-boring/src/client.rs:180
## INFO (1)
### [Code Quality] In hyper-boring, `session_cache_capacity(0)` used to keep one session per key. It now disables caching.
- **File:** tokio-boring/src/client.rs:104
📋 Code Quality: ⚠️ 1 warning · 1 info
The connector refactor is clean. Each Connector registers a new global ex-data index, and capacity-0 behaviour changes silently for hyper-boring.
Recommendation: 📝 Minor Comments
- [WARNING] Every
Connectorregisters a new process-global SSL ex-data index that is never freed. The old code used one static index. (tokio-boring/src/client.rs:180) - [INFO] In hyper-boring,
session_cache_capacity(0)used to keep one session per key. It now disables caching. (tokio-boring/src/client.rs:104)
View full review
- New global ex-data index for every connector (
tokio-boring/src/client.rs:180):with_session_cache_capacitycallsSsl::new_ex_index::<SessionKey>()each time it runs. The old hyper-boring code kept one index in a static. BoringSSL cannot unregister ex-data indices, and everySslreserves a slot for each registered index. Code that builds connectors repeatedly (HttpsConnector::new/HttpsLayer::newper client or per task) grows that global table without bound. Each laterSslallocation then costs a little more. - Capacity 0 (
tokio-boring/src/client.rs:104): withper_key_session_capacity == 0, the old hyper-boring cache still kept one session per key. The new cache turns caching off. The new behaviour is more sensible and is documented onConnector, butHttpsLayerSettingsusers get the change without notice.
✅ All other checks passed
🔒 Security: ✅ LGTM
Session isolation by peer name and port is unchanged, TLS 1.3 tickets are still single-use, and IPv6 bracket stripping still happens before hostname verification.
📖 AGENTS.md: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
No performance issues beyond the ex-data index growth covered in the code quality section.
📝 Documentation: ✅ LGTM
The new public API has rustdoc comments.
Reviewed commit: 5e655f50
| capacity: usize, | ||
| ) -> Result<Self, ErrorStack> { | ||
| let cache = Arc::new(Mutex::new(SessionCache::with_capacity(capacity))); | ||
| let key_index = Ssl::new_ex_index::<SessionKey>()?; |
There was a problem hiding this comment.
Every Connector registers a new process-global SSL ex-data index that is never freed. The old code used one static index.
Ssl::new_ex_index adds an index to BoringSSL's global SSL ex-data class, and that class cannot unregister indices. Before this change, hyper-boring kept one index in a static, so every connector shared it. Now Connector::new, HttpsConnector::new and HttpsLayer::new each add one more. A process that creates connectors repeatedly will grow the global index table without bound, and every Ssl allocated later reserves a slot for each index. Since SessionKey is private, a single lazily initialised static index (for example a LazyLock<Index<Ssl, SessionKey>>, or Ssl::cached_ex_index) is enough.
There was a problem hiding this comment.
Fixed in 60a4538. The session key index is now initialized once with a process-wide LazyLock and shared by every Connector.
| } | ||
|
|
||
| fn insert(&mut self, key: SessionKey, session: SslSession) { | ||
| if self.per_target_capacity == 0 { |
There was a problem hiding this comment.
ℹ️ INFO · Code Quality
In hyper-boring, session_cache_capacity(0) used to keep one session per key. It now disables caching.
With capacity 0, the old hyper-boring SessionCache::insert evicted nothing and still inserted the session, so one session per key stayed cached. This early return turns caching off completely. That is a reasonable meaning for 0, but HttpsLayerSettings users get the change without notice. Consider mentioning it in the changelog.
There was a problem hiding this comment.
Addressed in 60a4538. hyper-boring now maps its legacy capacity of 0 to 1 and has regression coverage, while the new tokio-boring Connector keeps its documented 0 = disabled behavior.
5e655f5 to
60a4538
Compare
There was a problem hiding this comment.
Superseded by the review of
63447672.
This review, of 60a4538a
✅ Approved — Both earlier findings (per-connector ex-data index, capacity-0 semantics in hyper-boring) are fixed, and the changed code has no new issues.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
Sessions are still isolated by peer name and port, TLS 1.3 tickets are still single-use, and hostname verification still sees the host with IPv6 brackets removed.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
The ex-data index is now a single process-wide LazyLock (tokio-boring/src/client.rs:117-118). hyper-boring maps capacity 0 to 1 to keep its previous behaviour, and a test covers this. Callback order, IPv6 bracket stripping and TLS 1.3 ticket removal match the removed hyper-boring implementation.
⚡ Performance: ✅ LGTM
No issues found.
📝 Documentation: ✅ LGTM
No issues found.
Reviewed commit: 60a4538a
60a4538 to
6344767
Compare
There was a problem hiding this comment.
Superseded by the review of
fbc30ad4.
This review, of 63447672
✅ Approved — No issues found. The hyper-boring/src/v1.rs change keeps the existing callback, error and session-cache behaviour, and both earlier comments are still fixed.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
- Hostname verification still gets the host with IPv6 brackets removed.
- Sessions are still kept separate per peer name and port.
- TLS 1.3 tickets are still used only once.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
effective_session_cache_capacityturns a capacity of 0 into 1, and a unit test covers it.HttpTarget::try_newkeeps the missing-host error.- IPv6 brackets are still stripped in
Target::new, as the removed hyper-boring code did. - The
localhosttest now checks thatset_callbackgets the URI on each connection.
⚡ Performance: ✅ LGTM
- One process-wide ex-data index is shared by all connectors.
- Cache insert and lookup stay O(1).
- Each connection allocates one extra
Stringfor the peer name, which is negligible.
📝 Documentation: ✅ LGTM
No issues found.
Reviewed commit: 63447672
6344767 to
fbc30ad
Compare
There was a problem hiding this comment.
Superseded by the review of
6674fe4d.
This review, of fbc30ad4
✅ Approved — No issues found. Since the last review, the only change pins the CI runner to ubuntu-24.04, and the connector and the hyper-boring migration keep the behaviour approved earlier.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
No issues found. Hostname verification, keeping sessions separate per peer, and using each TLS 1.3 ticket only once all work as before.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
No issues found.
- CI change:
.github/workflows/ci.ymlnow pins thetest-boringsslrunner toubuntu-24.04. It does not affect the code. - hyper-boring: it keeps its earlier behaviour:
- The config callback still runs before a session is chosen.
- A missing host still gives the same error.
- IPv6 brackets are still stripped.
- A cache capacity of 0 still maps to 1.
- Removed dependencies: nothing in
hyper-boringstill usesantidoteorlinked_hash_set.
⚡ Performance: ✅ LGTM
No issues found.
- Session cache insert and lookup stay O(1).
- All connectors share one process-wide ex-data index.
- Each connection adds one small
Stringallocation, which is negligible.
📝 Documentation: ✅ LGTM
No issues found.
Reviewed commit: fbc30ad4
fbc30ad to
6674fe4
Compare
There was a problem hiding this comment.
Superseded by the review of
e382a152.
This review, of 6674fe4d
✅ Approved — No issues found. Since the last review, only .github/workflows/ci.yml changed: it pins the runner and adds a BoringSSL build flag to avoid a GCC Keccak overflow false positive. The connector and the hyper-boring migration behave as approved before.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
No issues found. Hostname verification still works, sessions stay separate per peer, and each TLS 1.3 ticket is still used only once.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
No issues found.
Since the last review, the only change is in .github/workflows/ci.yml: it pins the CI runner and adds a BoringSSL CMake flag. Neither touches the library code.
The connector and the hyper-boring migration behave as before:
- The config callback still runs before a session is chosen.
- A missing host still returns the same error.
- A cache capacity of 0 still maps to 1 in
hyper-boring. - IPv6 brackets are still stripped.
- All connectors share one session key index, created once.
⚡ Performance: ✅ LGTM
No issues found.
- Session cache insert and lookup are still O(1), and each target's cache stays bounded.
- All connectors share one process-wide ex-data index.
- Each connection adds a few small
Stringallocations. That cost is negligible next to a TLS handshake.
📝 Documentation: ✅ LGTM
No issues found in the changed files.
Reviewed commit: 6674fe4d
6674fe4 to
e382a15
Compare
There was a problem hiding this comment.
Superseded by the review of
023f7393.
This review, of e382a152
✅ Approved — No issues found. The two earlier discussion points (shared ex-data index, capacity-0 mapping in hyper-boring) are fixed at this commit.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
No issues found.
- Hostname verification still uses the peer name.
- Sessions are cached per peer name and port, so peers do not share them.
- Each TLS 1.3 ticket is removed from the cache under the lock before use, so concurrent handshakes cannot pick the same one.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
Both earlier points are fixed:
- The session key index is initialised once through a process-wide
LazyLockand every connector shares it. effective_session_cache_capacityinhyper-boring/src/v1.rsmaps 0 to 1.Connectorkeeps 0 = disabled, and a test covers that.
The hyper-boring migration keeps the existing behaviour:
- The config callback still runs before a session is chosen.
- The default port is still 443, and a missing host returns the same error.
- IPv6 brackets are still stripped.
⚡ Performance: ✅ LGTM
No issues found.
- Cache insert and lookup are O(1), and each target's cache stays bounded by its capacity.
- The ex-data index is process-wide.
- Each connection allocates a few small strings, which is negligible next to a TLS handshake.
📝 Documentation: ✅ LGTM
No issues found in the changed files.
Reviewed commit: e382a152
e382a15 to
023f739
Compare
There was a problem hiding this comment.
Superseded by the review of
44d2a2f1.
This review, of 023f7393
✅ Approved — No issues found at this commit. The new partitioned session cache keeps identities apart, and the earlier discussion points are still fixed.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
No issues found.
- Session partitions compare type and value, so sessions cannot be reused across identities.
- Hostname verification still uses the peer name.
- TLS 1.3 tickets are single-use: each is removed from the cache under the lock before it is returned.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
No issues found.
- The session-key index is process-wide.
effective_session_cache_capacitykeeps hyper-boring's old behaviour for capacity 0.- Setup errors are returned as
HandshakeError(SetupFailure(..)). - The tests cover partition isolation, a cache shared by clones and callback ordering.
⚡ Performance: ✅ LGTM
No issues found.
📝 Documentation: ✅ LGTM
No issues found in the changed files.
Reviewed commit: 023f7393
023f739 to
44d2a2f
Compare
There was a problem hiding this comment.
Superseded by the review of
e52a36b1.
This review, of 44d2a2f1
✅ Approved — No issues found in the code changed since the last review. Session isolation, single-use tickets and hyper-boring's existing behaviour all hold up.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
- Session keys: they include the peer name, the port and the partition's type and value. Partitions are compared for equality, not only by hash.
- Hostname verification: it still uses the peer name.
- TLS 1.3 tickets: each one is removed from the cache under the lock before it is returned, so it is used only once.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
- Behaviour kept: the callback order is unchanged, IPv6 brackets are now stripped in
Target::new, and the error for a URI with no host is still returned. - Capacity 0:
effective_session_cache_capacitykeepshyper-boring's old behaviour. - Dependencies:
antidoteandlinked_hash_setare no longer used inhyper-boring, and both are declared in the workspaceCargo.toml, so the newtokio-boringdependencies resolve. - Tests: they cover resumption, isolation by partition and by port, a cache shared by clones, callback ordering and zero capacity.
⚡ Performance: ✅ LGTM
- Per-connection cost: each connection adds one small
Stringand oneArcallocation. - Cache lock: it is never held across an
.await. - Ex-data index: the process-wide
LazyLockindex avoids registering a new global index for every connector.
📝 Documentation: ✅ LGTM
No issues found in the changed files.
Reviewed commit: 44d2a2f1
619e7fb to
e52a36b
Compare
There was a problem hiding this comment.
Superseded by the review of
8eae3bbd.
This review, of e52a36b1
✅ Approved — No issues found in the code changed since the last review. The ex-data index fix and the capacity-zero fix from the earlier discussion are both in this commit, and session isolation and single-use TLS 1.3 tickets still hold.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
- Session keys: they include the peer name, the port and the partition value, and partitions are compared with
Eq. - TLS 1.3 tickets: each one is removed from the cache under the lock before it is returned, so it is used only once.
- Hostname verification: it still uses the peer name.
- Public
Ssl::cached_ex_index: each index is typed, andSessionKey<P>is private totokio-boring, so no other code can read or overwrite the session key.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
- Shared ex-data index: the process-wide
Ssl::cached_ex_index::<SessionKey<P>>()avoids registering a new global index for every connector. - Cache lock: it is never held across an
.await. - Capacity zero:
hyper-boringstill treats a capacity of 0 the way it always did. - Tests: they cover resumption, isolation by partition and by port, a cache shared by clones, callback order and zero capacity.
⚡ Performance: ✅ LGTM
- Per-connection cost: each connection adds one small
Stringand oneArcallocation. - Cache lock: it is held only for short operations and never across an
.await. - Cache size: the cache is still bounded per key, as before.
📝 Documentation: ✅ LGTM
No issues found in the changed files.
Reviewed commit: e52a36b1
| } | ||
|
|
||
| /// Provides a borrowed [`Target`] view of a richer connection target. | ||
| pub trait AsTarget { |
e52a36b to
8eae3bb
Compare
There was a problem hiding this comment.
✅ Approved — The only code changed since the last review is a dependency addition and a module export, and both are correct. The session isolation, the single-use TLS 1.3 tickets and the ex-data index fix still hold.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
Session keys still include the peer name, the port and the partition value. Each TLS 1.3 ticket is removed from the cache under the lock before it is used, so it is used only once. Hostname verification still uses the peer name.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
antidote and linked_hash_set are declared as workspace dependencies, and client.rs uses both. pub mod client; exposes the new connector. The AsTarget trait fits better than AsRef<Target> because Target<'a> is a borrowed view returned by value.
⚡ Performance: ✅ LGTM
The new dependencies add no cost at runtime. The cache lock is held only for short synchronous operations and never across an .await.
📝 Documentation: ✅ LGTM
No issues found in the changed files.
Reviewed commit: 8eae3bbd
This pull request adds a reusable client connector to
tokio-boring. The connector accepts an already-established asynchronous stream, applies the configured connection and SSL callbacks, and performs the TLS handshake. Callers provide the peer name and session-cache port throughTarget, whileAsTargetlets their callbacks continue to receive a richer application-specific type.Connector<T, P = ()>parameterizes its cache by the partition type. The ordinaryconnectmethod is available onConnector<T, ()>and isolates sessions by target.connect_partitionedaccepts aPvalue and resumes sessions only when the peer name, port, and partition value all match, allowing applications to include identity-affecting context such as a selected client certificate when sharing one connector across differently configured clients.SessionKey<P>stores the partition directly, preserving normalEqandHashsemantics.Connector clones share a bounded session cache, and TLS 1.3 tickets are removed before use so concurrent handshakes cannot select the same ticket. The complete generic key is stored in a lazily allocated, process-wide ex-data index for
SessionKey<P>, following Boring’s existing typed cached-index pattern without leaking one index per connector.It also updates
hyper-boringto use this connector instead of maintaining its own session cache and handshake setup. The existing URI-based callback API, HTTP scheme handling, stream wrapper, Hyper connection metadata, and historical capacity-zero behavior remain unchanged.