cmcfarlen commented on PR #13490:
URL: https://github.com/apache/trafficserver/pull/13490#issuecomment-5196086397
The retry loop is correct, though it took some staring to convince myself.
`is_response_available` guarantees `resp_derlen != 0`, so on the first pass
`resp_derlen > 0 == resp_capacity` and the `memcpy(p, ...)` with `p == nullptr`
is unreachable. Responses are bounded at cache time (`resp_derlen >
MAX_STAPLING_DER` is rejected at line 845), so the loop realistically runs at
most twice. The `!is_response_available` exit frees `p` correctly.
`is_prefetched` becoming `const` and read unlocked is right — it is
construction-time only.
**Main suggestion: use `ts::bravo::shared_mutex` instead of
`std::shared_mutex`.**
That is the codebase's own idiom for exactly this shape — read-mostly state
with hot readers. `src/iocore/net/ConnectionTracker.cc` uses it throughout
(`std::lock_guard<ts::bravo::shared_mutex>` for writers,
`ts::bravo::shared_lock<...>` for readers), as does `P_SSLCertLookup.h`. BRAVO
is biased toward readers specifically to avoid the reader cache-line contention
a plain `std::shared_mutex` suffers, which is the whole motivation for this
change on a TLS handshake path.
Two reasons beyond consistency:
1. **`ts::bravo::shared_mutex` is annotated for `-Wthread-safety`** —
`include/tsutil/Bravo.h` carries `TS_CAPABILITY("shared_mutex")`,
`TS_SCOPED_CAPABILITY`, `TS_ACQUIRE_SHARED`, `TS_RELEASE_SHARED`.
`std::shared_mutex` is annotated by libc++ but not by libstdc++, so using it
here produces clang thread-safety findings on macOS that Linux CI will not show.
2. `include/tsutil/Bravo.h` is present on both master and 10.2.x, so this
costs nothing for the backport.
**Possible crossed wire:** your reply to Copilot above says the writer path
"now explicitly uses `std::lock_guard<ts::bravo::shared_mutex>`", but the
current diff has `std::lock_guard<std::shared_mutex>` and `mutable
std::shared_mutex resp_mutex`. The substance of Copilot's point is addressed —
it is an explicit type rather than CTAD — but the type named in the reply isn't
what landed. Did a Bravo conversion get lost somewhere, or was that describing
intent?
Minor: the description says certinfo is allocated with `new`/`delete`
"instead of `OPENSSL_malloc`", but master is already on
`std::make_unique<certinfo>()` (line 885). Stale wording only, no code impact.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
