HTTPCLIENT-2386: Fix TLS handshake timeout precedence #694
Conversation
Default to socketTimeout (not connectTimeout) for TLS handshakes
| // 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) |
There was a problem hiding this comment.
@arturobernalg I am not sure this is correct. Connect timeout should no longer have any effect.
| // 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 |
There was a problem hiding this comment.
@rschmitt Would this change be OK with you? This should fix an inconsistency in the behavior of the classic and async transports.
There was a problem hiding this 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, which can also be set in a variety of ways. Such coverage could be added to the existing integration tests for socket timeouts. 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.
There was a problem hiding this comment.
There was a problem hiding this comment.
You're doing Windows development in mingw? Not WSL2 or native Windows?
There was a problem hiding this comment.
@rschmitt Ubuntu (or Debian) Linux is my primary development environment but sometimes I have to use a client issued laptop that runs Windows. I run Maven inside WinGit console. It is an old habit.
|
Closing in favor of 88c19c0 |
|
@ok2c @rschmitt can you help me with the problem when I use the httpAsyncClient( httpclient 5.4.3 、jdk 21) I have modified the timeout multiple times and obtained the following results.
It seems that the effected ResponseTimeout is being rounded up to the nearest second, and milliseconds are not taking effect. In addition, I used a synchronous httpclient, which works well. I set the ResponseTimeout = 50ms. |
@lethinker Socket timeout granularity of async connections is one second (can be reduced at the cost of the i/o reactor running in a tighter loop and waking up more often, and causing higher CPU utilization). Socket timeout granularity of blocking connections is approximately 10 ms. Timeouts in ms make no sense. |
Thank you for your help. |
@lethinker No, there is not. Please note that the solution mentioned by @rschmitt will cause the JRE run with near 100% CPU utilization. This is not a problem with a short integration test but may be a problem when running in PROD |
4 participants
Changes TLS handshake timeout fallback from
connectTimeouttosocketTimeoutwhen no explicit timeout is configured inTlsConfig.