Skip to content

HTTPCLIENT-2386: Fix TLS handshake timeout precedence #694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
arturobernalg wants to merge 1 commit into from
Closed

HTTPCLIENT-2386: Fix TLS handshake timeout precedence #694

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion ...c/main/java/org/apache/hc/client5/http/impl/nio/DefaultAsyncClientConnectionOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

@ok2c ok2c Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arturobernalg I am not sure this is correct. Connect timeout should no longer have any effect.

final Timeout handshakeTimeout = tlsConfig.getHandshakeTimeout() != null
Copy link
Member

@ok2c ok2c Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rschmitt Would this change be OK with you? This should fix an inconsistency in the behavior of the classic and async transports.

Copy link
Contributor

@rschmitt rschmitt Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ok2c ok2c Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rschmitt Fair enough. Please take a look at an alternative #699 and corrects the behavior of the classic connection operator instead.

With regards to the connect timeout integration tests they consistently fail for me locally (Windows MINGW64_NT-10.0-26100). I need to get them fixed first.

Copy link
Contributor

@rschmitt rschmitt Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're doing Windows development in mingw? Not WSL2 or native Windows?

Copy link
Member

@ok2c ok2c Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

? tlsConfig.getHandshakeTimeout()
: socketTimeout;
final NamedEndpoint tlsName = endpointName != null ? endpointName : endpointHost;
onBeforeTlsHandshake(context, endpointHost);
if (LOG.isDebugEnabled()) {
Expand Down
Loading