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: [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]
