Skip to content

HTTPCLIENT-2386: Classic transport to use connect timeout as a default if TLS timeout has not been explicitly set #699

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

Merged
ok2c merged 1 commit into from
Aug 10, 2025
Merged

HTTPCLIENT-2386: Classic transport to use connect timeout as a default if TLS timeout has not been explicitly set #699

Show file tree
Hide file tree
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
16 changes: 12 additions & 4 deletions ...src/main/java/org/apache/hc/client5/http/impl/io/DefaultHttpClientConnectionOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.apache.hc.client5.http.SchemePortResolver;
import org.apache.hc.client5.http.SystemDefaultDnsResolver;
import org.apache.hc.client5.http.UnsupportedSchemeException;
import org.apache.hc.client5.http.config.TlsConfig;
import org.apache.hc.client5.http.impl.ConnPoolSupport;
import org.apache.hc.client5.http.impl.DefaultSchemePortResolver;
import org.apache.hc.client5.http.io.DetachedSocketFactory;
Expand Down Expand Up @@ -160,7 +161,7 @@ public void connect(
Args.notNull(socketConfig, "Socket config");
Args.notNull(context, "Context");

final Timeout soTimeout = socketConfig.getSoTimeout();
final Timeout socketTimeout = socketConfig.getSoTimeout();
final SocketAddress socksProxyAddress = socketConfig.getSocksProxyAddress();
final Proxy socksProxy = socksProxyAddress != null ? new Proxy(Proxy.Type.SOCKS, socksProxyAddress) : null;

Expand All @@ -186,8 +187,8 @@ public void connect(
socket.bind(localAddress);
}
conn.bind(socket);
if (soTimeout != null) {
socket.setSoTimeout(soTimeout.toMillisecondsIntBound());
if (socketTimeout != null) {
socket.setSoTimeout(socketTimeout.toMillisecondsIntBound());
}
socket.setReuseAddress(socketConfig.isSoReuseAddress());
socket.setTcpNoDelay(socketConfig.isTcpNoDelay());
Expand Down Expand Up @@ -217,16 +218,23 @@ public void connect(
if (LOG.isDebugEnabled()) {
LOG.debug("{} {} connected {}->{}", ConnPoolSupport.getId(conn), endpointHost, conn.getLocalAddress(), conn.getRemoteAddress());
}
conn.setSocketTimeout(soTimeout);

@rschmitt rschmitt Aug 10, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this does, since we set the socket timeout directly on the socket (with a null check) on line 191.

conn.setSocketTimeout(socketTimeout);
final TlsSocketStrategy tlsSocketStrategy = tlsSocketStrategyLookup != null ? tlsSocketStrategyLookup.lookup(endpointHost.getSchemeName()) : null;
if (tlsSocketStrategy != null) {
final NamedEndpoint tlsName = endpointName != null ? endpointName : endpointHost;
onBeforeTlsHandshake(context, endpointHost);
if (LOG.isDebugEnabled()) {
LOG.debug("{} {} upgrading to TLS", ConnPoolSupport.getId(conn), tlsName);
}
final TlsConfig tlsConfig = attachment instanceof TlsConfig ? (TlsConfig) attachment : TlsConfig.DEFAULT;
final int soTimeout = socket.getSoTimeout();
final Timeout handshakeTimeout = tlsConfig.getHandshakeTimeout() != null ? tlsConfig.getHandshakeTimeout() : connectTimeout;
if (handshakeTimeout != null) {
socket.setSoTimeout(handshakeTimeout.toMillisecondsIntBound());
}
final SSLSocket sslSocket = tlsSocketStrategy.upgrade(socket, tlsName.getHostName(), tlsName.getPort(), attachment, context);
conn.bind(sslSocket, socket);
socket.setSoTimeout(soTimeout);
Comment on lines +233 to +237

@rschmitt rschmitt Aug 10, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is much clearer than before.

onAfterTlsHandshake(context, endpointHost);
if (LOG.isDebugEnabled()) {
LOG.debug("{} {} upgraded to TLS", ConnPoolSupport.getId(conn), tlsName);
Expand Down
4 changes: 2 additions & 2 deletions ...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 @@ -141,7 +141,7 @@ public void completed(final IOSession session) {
if (tlsStrategy != null) {
try {
final Timeout socketTimeout = connection.getSocketTimeout();
final Timeout handshakeTimeout = tlsConfig.getHandshakeTimeout();
final Timeout handshakeTimeout = tlsConfig.getHandshakeTimeout() != null ? tlsConfig.getHandshakeTimeout() : connectTimeout;
final NamedEndpoint tlsName = endpointName != null ? endpointName : endpointHost;
onBeforeTlsHandshake(context, endpointHost);
if (LOG.isDebugEnabled()) {
Expand All @@ -151,7 +151,7 @@ public void completed(final IOSession session) {
connection,
tlsName,
attachment,
handshakeTimeout != null ? handshakeTimeout : connectTimeout,
handshakeTimeout,
new FutureContribution<TransportSecurityLayer>(future) {

@Override
Expand Down
8 changes: 0 additions & 8 deletions httpclient5/src/main/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,6 @@ private void executeHandshake(
final SSLSocket upgradedSocket,
final String target,
final Object attachment) throws IOException {
final TlsConfig tlsConfig = attachment instanceof TlsConfig ? (TlsConfig) attachment : TlsConfig.DEFAULT;

final SSLParameters sslParameters = upgradedSocket.getSSLParameters();
if (supportedProtocols != null) {
sslParameters.setProtocols(supportedProtocols);
Expand All @@ -238,17 +236,11 @@ private void executeHandshake(
}
upgradedSocket.setSSLParameters(sslParameters);

final Timeout handshakeTimeout = tlsConfig.getHandshakeTimeout();
if (handshakeTimeout != null) {
upgradedSocket.setSoTimeout(handshakeTimeout.toMillisecondsIntBound());
}

initializeSocket(upgradedSocket);

if (LOG.isDebugEnabled()) {
LOG.debug("Enabled protocols: {}", (Object) upgradedSocket.getEnabledProtocols());
LOG.debug("Enabled cipher suites: {}", (Object) upgradedSocket.getEnabledCipherSuites());
LOG.debug("Starting handshake ({})", handshakeTimeout);
}
upgradedSocket.startHandshake();
verifySession(target, upgradedSocket.getSession());
Expand Down
Loading