rschmitt commented on code in PR #694:
URL:
https://github.com/apache/httpcomponents-client/pull/694#discussion_r2252475258
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/DefaultAsyncClientConnectionOperator.java:
##########
@@ -165,7 +165,13 @@ public void completed(final IOSession session) {
if (tlsStrategy != null) {
try {
final Timeout socketTimeout =
connection.getSocketTimeout();
- final Timeout handshakeTimeout =
tlsConfig.getHandshakeTimeout();
+ // TLS handshake timeout precedence:
+ // 1. Explicitly configured handshake timeout
from TlsConfig
+ // 2. Current socket timeout of the connection
(if set)
+ // 3. Falls back to connectTimeout if neither
is specified (handled later)
+ final Timeout handshakeTimeout =
tlsConfig.getHandshakeTimeout() != null
Review Comment:
It should default to the `connectTimeout`. The whole point of having
separate TLS handshake timeout configuration is that defaulting to the
`socketTimeout` (which is what would naturally happen) means that TLS
handshakes take at least an order of magnitude longer to time out than is
sensible. Response data can take an arbitrarily long amount of time to come
back, whereas a TLS handshake should take roughly `2*RTT` irrespective of the
nature of the request being sent. If there's an inconsistency here between
classic and async then it sounds like the classic behavior is wrong.
It also sounds like we could use an integration test here [similar to the
one for socket
timeouts](https://github.com/apache/httpcomponents-client/blob/master/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestSocketTimeout.java#L63-L77),
which can also be set in a variety of ways. Such coverage could be added to
[the existing integration tests for socket
timeouts](https://github.com/apache/httpcomponents-client/blob/master/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestSocketTimeout.java#L63-L77).
I added these tests in order to prevent _precisely_ these kinds of regressions
which I've had to deal with in the past, and it's not good that a change like
the one in this PR doesn't break any test cases.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
