bryancall commented on code in PR #12860:
URL: https://github.com/apache/trafficserver/pull/12860#discussion_r2879835307
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -1579,6 +1579,12 @@ SSLNetVConnection::sslClientHandShakeEvent(int &err)
Metrics::Counter::increment(ssl_rsb.total_success_handshake_count_out);
sslHandshakeStatus = SSLHandshakeStatus::SSL_HANDSHAKE_DONE;
+
+ // Record TLS handshake end time for outbound connections
+ if (this->get_tls_handshake_begin_time()) {
+ this->_record_tls_handshake_end_time();
+ }
Review Comment:
The naming in this file is from the TLS role perspective, not the ATS role.
`sslClientHandShakeEvent()` handles the **outbound** side where ATS acts as the
TLS *client* connecting to the origin. `sslServerHandShakeEvent()` handles the
**inbound** side where ATS acts as the TLS *server* accepting client
connections.
The inbound side already records the end time (line 1383), but the outbound
side was missing it — this is a bug fix. Without this,
`get_tls_handshake_end_time()` returns zero when HttpSM tries to copy the
timestamps into milestones.
I've updated the comment to clarify the naming convention and the two-stage
pattern.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -1881,21 +1881,39 @@ HttpSM::state_http_server_open(int event, void *data)
case CONNECT_EVENT_RETRY:
do_http_server_open();
break;
- case CONNECT_EVENT_TXN:
+ case CONNECT_EVENT_TXN: {
+ // Multiplexed connection path (e.g. HTTP/2): ConnectingEntry created the
session
+ // and dispatched this event. Get the netvc from the PoolableSession.
SMDbg(dbg_ctl_http, "Connection handshake complete via CONNECT_EVENT_TXN");
- if (this->create_server_txn(static_cast<PoolableSession *>(data))) {
+ PoolableSession *session = static_cast<PoolableSession *>(data);
+ ink_assert(session != nullptr);
+ // Capture server TLS handshake timing from the session's netvc
+ if (auto *netvc = session->get_netvc()) {
+ if (auto tbs = netvc->get_service<TLSBasicSupport>()) {
+ milestones[TS_MILESTONE_SERVER_TLS_HANDSHAKE_START] =
tbs->get_tls_handshake_begin_time();
+ milestones[TS_MILESTONE_SERVER_TLS_HANDSHAKE_END] =
tbs->get_tls_handshake_end_time();
Review Comment:
The HttpSM doesn't have access to the netvc during the handshake in the
multiplexed path — ConnectingEntry owns the connection and dispatches
`CONNECT_EVENT_TXN` after the handshake completes. So this is the earliest
point the SM can grab the timestamps.
The actual start/end times are captured in real-time at the netvc layer
(`sslClientHandShakeEvent()` records the end time, and the begin time is set
when the handshake starts). We're just copying them into milestones here. If
this code isn't reached (e.g. connection failure), the milestones stay at zero,
which is correct — `difference_msec()` checks for zero before computing the
difference.
Updated the comments to make this clearer.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -1881,21 +1881,39 @@ HttpSM::state_http_server_open(int event, void *data)
case CONNECT_EVENT_RETRY:
do_http_server_open();
break;
- case CONNECT_EVENT_TXN:
+ case CONNECT_EVENT_TXN: {
+ // Multiplexed connection path (e.g. HTTP/2): ConnectingEntry created the
session
+ // and dispatched this event. Get the netvc from the PoolableSession.
SMDbg(dbg_ctl_http, "Connection handshake complete via CONNECT_EVENT_TXN");
- if (this->create_server_txn(static_cast<PoolableSession *>(data))) {
+ PoolableSession *session = static_cast<PoolableSession *>(data);
+ ink_assert(session != nullptr);
+ // Capture server TLS handshake timing from the session's netvc
+ if (auto *netvc = session->get_netvc()) {
+ if (auto tbs = netvc->get_service<TLSBasicSupport>()) {
+ milestones[TS_MILESTONE_SERVER_TLS_HANDSHAKE_START] =
tbs->get_tls_handshake_begin_time();
+ milestones[TS_MILESTONE_SERVER_TLS_HANDSHAKE_END] =
tbs->get_tls_handshake_end_time();
+ }
+ }
+ if (this->create_server_txn(session)) {
t_state.current.server->clear_connect_fail();
handle_http_server_open();
} else { // Failed to create transaction. Maybe too many active
transactions already
// Try again (probably need a bounding counter here)
do_http_server_open(false);
}
return 0;
+ }
case VC_EVENT_READ_COMPLETE:
case VC_EVENT_WRITE_READY:
case VC_EVENT_WRITE_COMPLETE:
- // Update the time out to the regular connection timeout.
+ // Direct connection path (e.g. HTTP/1): HttpSM owns the netvc directly
+ // and creates the session itself.
SMDbg(dbg_ctl_http_ss, "Connection handshake complete");
+ // Capture server TLS handshake timing if this is a TLS connection
+ if (auto tbs = _netvc->get_service<TLSBasicSupport>()) {
+ milestones[TS_MILESTONE_SERVER_TLS_HANDSHAKE_START] =
tbs->get_tls_handshake_begin_time();
+ milestones[TS_MILESTONE_SERVER_TLS_HANDSHAKE_END] =
tbs->get_tls_handshake_end_time();
+ }
Review Comment:
These aren't duplicates — they're two mutually exclusive connection paths.
`CONNECT_EVENT_TXN` fires for multiplexed connections (H2/H3) where
ConnectingEntry manages the connection. `VC_EVENT_WRITE_COMPLETE` fires for
direct connections (H1) where the SM owns the netvc. A given connection goes
through one path or the other, never both.
Updated both comments to cross-reference each other and explicitly state
they're mutually exclusive.
--
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]
