Repository navigation
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
44d2a2f to
619e7fb
Compare
619e7fb to
e52a36b
Compare
There was a problem hiding this comment.
✅ 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
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.