You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "rschmitt (via GitHub)" <gi...@apache.org> on 2026/01/15 01:29:55 UTC
[PR] PoolingHttpClientConnectionManager: Re-set socket timeout from SocketConfig upon lease [httpcomponents-client]
rschmitt opened a new pull request, #785:
URL: https://github.com/apache/httpcomponents-client/pull/785
This change fixes a bug in the synchronous client where socket timeouts were not being set correctly on reused TLS connections. It also changes the existing socket timeout tests to ensure that the various read and socket timeout configs are respected in the context of connection reuse.
See also https://github.com/apache/httpcomponents-client/commit/57423c02a7ac075cff7204b596c3da5c3cfea4e4
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] PoolingHttpClientConnectionManager: Re-set socket timeout from SocketConfig upon lease [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #785:
URL: https://github.com/apache/httpcomponents-client/pull/785#discussion_r2695797845
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java:
##########
@@ -444,6 +444,8 @@ public ConnectionEndpoint get(
conn.activate();
if (connectionConfig.getSocketTimeout() != null) {
conn.setSocketTimeout(connectionConfig.getSocketTimeout());
+ } else {
+ conn.setSocketTimeout(resolveSocketConfig(route).getSoTimeout());
Review Comment:
@rschmitt Why is this necessary? `SocketConfig` defines the initial socket parameters only. It should have no bearing on the persistent connections once they have been initialized. This change will also likely make behavior of the classic connection manager inconsistent with that of the async one.
If users want specific socket timeout for their persistent connections they should be using `ConnectionConfig`
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] PoolingHttpClientConnectionManager: Re-set socket timeout from SocketConfig upon lease [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #785:
URL: https://github.com/apache/httpcomponents-client/pull/785#discussion_r2696098721
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java:
##########
@@ -444,6 +444,8 @@ public ConnectionEndpoint get(
conn.activate();
if (connectionConfig.getSocketTimeout() != null) {
conn.setSocketTimeout(connectionConfig.getSocketTimeout());
+ } else {
+ conn.setSocketTimeout(resolveSocketConfig(route).getSoTimeout());
Review Comment:
@rschmitt The only way to resolve this problem in a reasonable manner if you do not want the users to make the decision is to make than decision for them by using non-null socket timeout in `ConnectionConfig`.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] DefaultManagedHttpClientConnection: Restore original socket timeout [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #785:
URL: https://github.com/apache/httpcomponents-client/pull/785#discussion_r2700949796
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/DefaultManagedHttpClientConnection.java:
##########
@@ -182,15 +181,15 @@ public void close(final CloseMode closeMode) {
@Override
public void bind(final Socket socket) throws IOException {
super.bind(WIRE_LOG.isDebugEnabled() ? new LoggingSocketHolder(socket, this.id, WIRE_LOG) : new SocketHolder(socket));
- socketTimeout = Timeout.ofMilliseconds(socket.getSoTimeout());
+ origSocketTimeout = Timeout.ofMilliseconds(socket.getSoTimeout());
}
@Override
public void bind(final SSLSocket sslSocket, final Socket socket) throws IOException {
super.bind(WIRE_LOG.isDebugEnabled() ?
new LoggingSocketHolder(sslSocket, socket, this.id, WIRE_LOG) :
new SocketHolder(sslSocket, socket));
- socketTimeout = Timeout.ofMilliseconds(sslSocket.getSoTimeout());
+ origSocketTimeout = Timeout.ofMilliseconds(sslSocket.getSoTimeout());
Review Comment:
@rschmitt Could we keep the original variable name or make a symmetric change in `DefaultManagedAsyncClientConnection`?
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/DefaultHttpClientConnectionOperator.java:
##########
@@ -283,11 +283,10 @@ private void connectToUnixDomainSocket(
}
final Socket newSocket = unixDomainSocketFactory.createSocket();
try {
- conn.bind(newSocket);
Review Comment:
@rschmitt There is a reason why the socket get bound to the connection before it gets corrected. This enables the user to abort the connection while it is being blocked in the connect method. This line cannot be removed.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] PoolingHttpClientConnectionManager: Re-set socket timeout from SocketConfig upon lease [httpcomponents-client]
Posted by "rschmitt (via GitHub)" <gi...@apache.org>.
rschmitt commented on code in PR #785:
URL: https://github.com/apache/httpcomponents-client/pull/785#discussion_r2696205735
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java:
##########
@@ -444,6 +444,8 @@ public ConnectionEndpoint get(
conn.activate();
if (connectionConfig.getSocketTimeout() != null) {
conn.setSocketTimeout(connectionConfig.getSocketTimeout());
+ } else {
+ conn.setSocketTimeout(resolveSocketConfig(route).getSoTimeout());
Review Comment:
I think the issue is in `DefaultHttpClientConnectionOperator`:
```java
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);
```
These last two lines are in the wrong order. `DefaultManagedHttpClientConnection#bind` stores `sslSocket.getSoTimeout()`, which is still the _handshake_ timeout:
```java
socketTimeout = Timeout.ofMilliseconds(sslSocket.getSoTimeout());
```
Then, we go behind the `conn`'s back and set the timeout directly on the socket:
```java
socket.setSoTimeout(soTimeout);
```
After the connection is bound, the correct way to do this would be to set the socket timeout through the connection, which updates the `conn.socketTimeout` field:
```java
conn.setSocketTimeout(Timeout.ofMilliseconds(soTimeout));
```
So anyway, when the connection gets leased from the pool, the stale `socketTimeout` (which is actually the TLS handshake timeout from the last request) is reapplied by `activate()`:
```java
@Override
public void activate() {
super.setSocketTimeout(socketTimeout);
}
```
There's a similar bug where the `RequestConfig.responseTimeout`, if set, gets captured by `conn` and reapplied by `activate()`. I'm not immediately sure how to fix that, but it's also a less impactful bug: you'd have to send one request with a `responseTimeout` set, and then a subsequent request without one. (My test does exactly this, but that's pretty artificial.)
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] PoolingHttpClientConnectionManager: Re-set socket timeout from SocketConfig upon lease [httpcomponents-client]
Posted by "rschmitt (via GitHub)" <gi...@apache.org>.
rschmitt commented on code in PR #785:
URL: https://github.com/apache/httpcomponents-client/pull/785#discussion_r2695888959
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java:
##########
@@ -444,6 +444,8 @@ public ConnectionEndpoint get(
conn.activate();
if (connectionConfig.getSocketTimeout() != null) {
conn.setSocketTimeout(connectionConfig.getSocketTimeout());
+ } else {
+ conn.setSocketTimeout(resolveSocketConfig(route).getSoTimeout());
Review Comment:
> `SocketConfig` defines the initial socket parameters only.
Strictly speaking, this is part of the stated contract for many of the methods in `SocketConfig`, but not `setSoTimeout`:
* `setSoLinger`: "Determines the default value of the {@link SocketOptions#SO_LINGER} parameter for newly created sockets."
* `setSoReuseAddress`: "Determines the default value of the {@link SocketOptions#SO_REUSEADDR} parameter for newly created sockets."
* `setSoTimeout`: "Determines the default socket timeout value for blocking I/O operations."
The failure to re-set `SocketConfig.soTimeout` on reused connections is a behavior change in 5.6 that I learned about from a user whose integration tests started failing. I interpret it as a regression.
> It should have no bearing on the persistent connections once they have been initialized.
Why is the socket timeout getting changed in the first place? I don't actually know what's changing it or why.
> This change will also likely make behavior of the classic connection manager inconsistent with that of the async one.
There was no such regression in the async manager, so there's already an inconsistency somewhere; the sync manager is screwing with the socket timeout when the async manager isn't.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] PoolingHttpClientConnectionManager: Re-set socket timeout from SocketConfig upon lease [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #785:
URL: https://github.com/apache/httpcomponents-client/pull/785#discussion_r2695982589
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java:
##########
@@ -444,6 +444,8 @@ public ConnectionEndpoint get(
conn.activate();
if (connectionConfig.getSocketTimeout() != null) {
conn.setSocketTimeout(connectionConfig.getSocketTimeout());
+ } else {
+ conn.setSocketTimeout(resolveSocketConfig(route).getSoTimeout());
Review Comment:
@rschmitt I cannot agree with that. I see no good reason to make behavior of this class illogical and inconsistent because there is (or was) supposedly another inconsistency somewhere else.
Again, if people want specific socket timeout they should explicitly set it with `ConnectionConfig`. If they leave it null they are fine with it being undefined.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] DefaultManagedHttpClientConnection: Restore original socket timeout [httpcomponents-client]
Posted by "rschmitt (via GitHub)" <gi...@apache.org>.
rschmitt merged PR #785:
URL: https://github.com/apache/httpcomponents-client/pull/785
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] PoolingHttpClientConnectionManager: Re-set socket timeout from SocketConfig upon lease [httpcomponents-client]
Posted by "rschmitt (via GitHub)" <gi...@apache.org>.
rschmitt commented on code in PR #785:
URL: https://github.com/apache/httpcomponents-client/pull/785#discussion_r2696017538
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java:
##########
@@ -444,6 +444,8 @@ public ConnectionEndpoint get(
conn.activate();
if (connectionConfig.getSocketTimeout() != null) {
conn.setSocketTimeout(connectionConfig.getSocketTimeout());
+ } else {
+ conn.setSocketTimeout(resolveSocketConfig(route).getSoTimeout());
Review Comment:
We have to do _something_, we can't just set users up to fail. Can we deprecate the `setSoTimeout` methods in `IOReactorConfig` and `SocketConfig`?
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] PoolingHttpClientConnectionManager: Re-set socket timeout from SocketConfig upon lease [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #785:
URL: https://github.com/apache/httpcomponents-client/pull/785#discussion_r2696059241
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java:
##########
@@ -444,6 +444,8 @@ public ConnectionEndpoint get(
conn.activate();
if (connectionConfig.getSocketTimeout() != null) {
conn.setSocketTimeout(connectionConfig.getSocketTimeout());
+ } else {
+ conn.setSocketTimeout(resolveSocketConfig(route).getSoTimeout());
Review Comment:
@rschmitt Why should we deprecate `#setSoTimeout` methods in `IOReactorConfig` and `SocketConfig`? What is wrong with them? Those are core level configuration that ensure connect _initialization_ of newly established connections.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] PoolingHttpClientConnectionManager: Re-set socket timeout from SocketConfig upon lease [httpcomponents-client]
Posted by "rschmitt (via GitHub)" <gi...@apache.org>.
rschmitt commented on code in PR #785:
URL: https://github.com/apache/httpcomponents-client/pull/785#discussion_r2696079222
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java:
##########
@@ -444,6 +444,8 @@ public ConnectionEndpoint get(
conn.activate();
if (connectionConfig.getSocketTimeout() != null) {
conn.setSocketTimeout(connectionConfig.getSocketTimeout());
+ } else {
+ conn.setSocketTimeout(resolveSocketConfig(route).getSoTimeout());
Review Comment:
Let me look into this some more. For one thing, it appears that the `responseTimeout` on `RequestConfig` is leaking across requests.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] PoolingHttpClientConnectionManager: Re-set socket timeout from SocketConfig upon lease [httpcomponents-client]
Posted by "rschmitt (via GitHub)" <gi...@apache.org>.
rschmitt commented on code in PR #785:
URL: https://github.com/apache/httpcomponents-client/pull/785#discussion_r2696270846
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java:
##########
@@ -444,6 +444,8 @@ public ConnectionEndpoint get(
conn.activate();
if (connectionConfig.getSocketTimeout() != null) {
conn.setSocketTimeout(connectionConfig.getSocketTimeout());
+ } else {
+ conn.setSocketTimeout(resolveSocketConfig(route).getSoTimeout());
Review Comment:
OK, a minimal change to fix `responseTimeout` leaking across requests is to change `DefaultManagedHttpClientConnection#setSocketTimeout` to simply not update `this.socketTimeout`. In other words, the `socketTimeout` would be captured at `bind` time and not updated after that, thus ensuring that `activate()` always restores the connection default socket timeout and not a request-scoped override value from `RequestConfig`. I have to slightly rework UDS connection establishment, but it seems to work.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] DefaultManagedHttpClientConnection: Restore original socket timeout [httpcomponents-client]
Posted by "rschmitt (via GitHub)" <gi...@apache.org>.
rschmitt commented on code in PR #785:
URL: https://github.com/apache/httpcomponents-client/pull/785#discussion_r2701373542
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/DefaultManagedHttpClientConnection.java:
##########
@@ -182,15 +181,15 @@ public void close(final CloseMode closeMode) {
@Override
public void bind(final Socket socket) throws IOException {
super.bind(WIRE_LOG.isDebugEnabled() ? new LoggingSocketHolder(socket, this.id, WIRE_LOG) : new SocketHolder(socket));
- socketTimeout = Timeout.ofMilliseconds(socket.getSoTimeout());
+ origSocketTimeout = Timeout.ofMilliseconds(socket.getSoTimeout());
}
@Override
public void bind(final SSLSocket sslSocket, final Socket socket) throws IOException {
super.bind(WIRE_LOG.isDebugEnabled() ?
new LoggingSocketHolder(sslSocket, socket, this.id, WIRE_LOG) :
new SocketHolder(sslSocket, socket));
- socketTimeout = Timeout.ofMilliseconds(sslSocket.getSoTimeout());
+ origSocketTimeout = Timeout.ofMilliseconds(sslSocket.getSoTimeout());
Review Comment:
I'll keep the original variable name, since `DefaultManagedAsyncClientConnection` already behaves in this way (I guess this is the actual inconsistency I was looking for)
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] DefaultManagedHttpClientConnection: Restore original socket timeout [httpcomponents-client]
Posted by "rschmitt (via GitHub)" <gi...@apache.org>.
rschmitt commented on code in PR #785:
URL: https://github.com/apache/httpcomponents-client/pull/785#discussion_r2701367969
##########
httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/DefaultHttpClientConnectionOperator.java:
##########
@@ -283,11 +283,10 @@ private void connectToUnixDomainSocket(
}
final Socket newSocket = unixDomainSocketFactory.createSocket();
try {
- conn.bind(newSocket);
Review Comment:
Ahh, okay.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org