masaori335 commented on PR #13811: URL: https://github.com/apache/trafficserver/pull/13811#issuecomment-6284776928
Thanks for the thorough review. 1. **Direct path:** You're right, my claim that the `ConnectingEntry` forward covers both paths was wrong. Fixed in f6ff53c9bd by writing the decoded errno into `connect_result` and `cause_of_death_errno` directly, like before #7580. I didn't take either `EIO` variant: `EIO` as a "connecting" placeholder in `connect_result` is what makes this path order-dependent in the first place, and presetting it in more places spreads that. I'd rather remove the placeholder in a follow-up PR. 2. **Test:** Agreed, the `excludes` can't fail on Linux. f6ff53c9bd adds `connect_attempts_bind_fail.replay.yaml`, using `outgoing_ip_to_bind: 192.0.2.1` as you suggested. It checks `connection_result=EADDRNOTAVAIL` with a positive `contains` for one shared txn and one `Authorization` txn, and uses the debug log to confirm each txn took the path it's meant to cover. I kept the `excludes` on the four existing replays because they still catch the regression where `connect()` to `0.0.0.1` fails synchronously. 3. **TS-1424 / `ENET_THROTTLING`:** Yes, intended. Both branches and their conditions are unchanged from before #7580, including the `is_transparent`-only check and the `origin_connections_throttled` increment. This PR only makes them reachable again. I agree the `EADDRNOTAVAIL` condition is broader than the TS-1424 comment describes and the metric is miscounted, but narrowing them would be a behaviour change of its own, so I'd keep that out of this PR (and out of the 10.2.x backport). I've added this to the description. 4. **SOCKS reason string:** Good catch, this one was a regression from this PR. Fixed in be3dc816fb: `get_error_string()` now maps internal codes of either sign, with Catch2 coverage. -- 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]
