Skip to content

Commit da424f2

Browse files
rschmittrschmitt
committed
DefaultManagedHttpClientConnection: Restore original socket timeout
This change fixes a bug in the synchronous client where socket timeouts were not being set correctly on reused connections, and the client was instead setting it to either the TLS handshake timeout or the `responseTimeout` from the `RequestConfig` of the previous request. The fix works by changing `DefaultManagedHttpClientConnection` to store the `socketTimeout` set on the socket at the time `bind` is called, and always restore that value when the `activate` method is called. Note that this requires the caller to call `setSoTimeout` on the socket _before_ binding the connection to it, hence the adjustments to some of the code in `DefaultHttpClientConnectionOperator`.
1 parent 3159813 commit da424f2

4 files changed

Lines changed: 32 additions & 32 deletions

File tree

‎httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/TestAsyncSocketTimeout.java‎

Lines changed: 14 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@
3838
import org.apache.hc.core5.http.Method;
3939
import org.apache.hc.core5.http.URIScheme;
4040
import org.apache.hc.core5.io.CloseMode;
41-
import org.apache.hc.core5.util.VersionInfo;
4241
import org.junit.jupiter.api.Nested;
4342
import org.junit.jupiter.api.Timeout;
4443
import org.junit.jupiter.params.ParameterizedTest;
@@ -51,7 +50,6 @@
5150
import static org.apache.hc.core5.util.ReflectionUtils.determineJRELevel;
5251
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
5352
import static org.junit.jupiter.api.Assertions.assertThrows;
54-
import static org.junit.jupiter.api.Assumptions.assumeFalse;
5553
import static org.junit.jupiter.api.Assumptions.assumeTrue;
5654

5755
abstract class AbstractTestSocketTimeout extends AbstractIntegrationTestBase {
@@ -83,21 +81,26 @@ void testReadTimeouts(final int connConfigTimeout, final int responseTimeout) th
8381
}
8482

8583
for (final boolean drip : new boolean[]{ false, true }) {
86-
final SimpleHttpRequest request = getRequest(responseTimeout, drip,
87-
target);
88-
89-
final Throwable cause = assertThrows(ExecutionException.class, () -> client.execute(request, null).get())
90-
.getCause();
91-
assertInstanceOf(SocketTimeoutException.class, cause);
84+
for (final boolean reuseConnection : new boolean[]{ false, true }) {
85+
if (reuseConnection) {
86+
client.execute(getRequest(2500, 0, false, target), null).get();
87+
}
88+
final SimpleHttpRequest request = getRequest(responseTimeout, 2500, drip, target);
89+
90+
final Throwable cause = assertThrows(ExecutionException.class,
91+
() -> client.execute(request, null).get()).getCause();
92+
assertInstanceOf(SocketTimeoutException.class, cause,
93+
String.format("drip=%s, reuseConnection=%s", drip, reuseConnection));
94+
}
9295
}
9396

9497
closeClient(client);
9598
}
9699

97-
private SimpleHttpRequest getRequest(final int responseTimeout, final boolean drip, final HttpHost target)
98-
throws Exception {
100+
private SimpleHttpRequest getRequest(final int responseTimeout, final int delay, final boolean drip,
101+
final HttpHost target) throws Exception {
99102
final SimpleHttpRequest request = SimpleHttpRequest.create(Method.GET, target,
100-
"/random/10240?delay=2500&drip=" + (drip ? 1 : 0));
103+
"/random/10240?delay=" + delay + "&drip=" + (drip ? 1 : 0));
101104
if (responseTimeout > 0) {
102105
request.setConfig(RequestConfig.custom()
103106
.setUnixDomainSocket(getUnixDomainSocket())
@@ -138,13 +141,6 @@ public Uds() {
138141
@Override
139142
void checkAssumptions() {
140143
assumeTrue(determineJRELevel() >= 16, "Async UDS requires Java 16+");
141-
final String[] components = VersionInfo
142-
.loadVersionInfo("org.apache.hc.core5", getClass().getClassLoader())
143-
.getRelease()
144-
.split("[-.]");
145-
final int majorVersion = Integer.parseInt(components[0]);
146-
final int minorVersion = Integer.parseInt(components[1]);
147-
assumeFalse(majorVersion <= 5 && minorVersion <= 3, "Async UDS requires HttpCore 5.4+");
148144
}
149145
}
150146
}

‎httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestSocketTimeout.java‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -85,15 +85,21 @@ void testReadTimeouts(final int socketConfigTimeout, final int connConfigTimeout
8585
}
8686

8787
for (final boolean drip : new boolean[]{ false, true }) {
88-
final HttpGet request = getRequest(responseTimeout, drip);
88+
for (final boolean reuseConnection : new boolean[]{ false, true }) {
89+
if (reuseConnection) {
90+
client.execute(target, getRequest(5000, 0, false), new BasicHttpClientResponseHandler());
91+
}
92+
final HttpGet request = getRequest(responseTimeout, 2500, drip);
8993

90-
assertThrows(SocketTimeoutException.class, () ->
91-
client.execute(target, request, new BasicHttpClientResponseHandler()));
94+
assertThrows(SocketTimeoutException.class, () ->
95+
client.execute(target, request, new BasicHttpClientResponseHandler()),
96+
String.format("drip=%s, reuseConnection=%s", drip, reuseConnection));
97+
}
9298
}
9399
}
94100

95-
private HttpGet getRequest(final int responseTimeout, final boolean drip) throws Exception {
96-
final HttpGet request = new HttpGet(new URI("/random/10240?delay=2500&drip=" + (drip ? 1 : 0)));
101+
private HttpGet getRequest(final int responseTimeout, final int delay, final boolean drip) throws Exception {
102+
final HttpGet request = new HttpGet(new URI("/random/10240?delay=" + delay + "&drip=" + (drip ? 1 : 0)));
97103
if (responseTimeout > 0) {
98104
request.setConfig(RequestConfig.custom()
99105
.setUnixDomainSocket(getUnixDomainSocket())

‎httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/DefaultHttpClientConnectionOperator.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -260,8 +260,8 @@ private void upgradeToTls(final ManagedHttpClientConnection conn, final HttpHost
260260
socket.setSoTimeout(handshakeTimeout.toMillisecondsIntBound());
261261
}
262262
final SSLSocket sslSocket = tlsSocketStrategy.upgrade(socket, tlsName.getHostName(), tlsName.getPort(), attachment, context);
263-
conn.bind(sslSocket, socket);
264263
socket.setSoTimeout(soTimeout);
264+
conn.bind(sslSocket, socket);
265265
onAfterTlsHandshake(context, endpointHost);
266266
if (LOG.isDebugEnabled()) {
267267
LOG.debug("{} {} upgraded to TLS", ConnPoolSupport.getId(conn), tlsName);
@@ -283,11 +283,10 @@ private void connectToUnixDomainSocket(
283283
}
284284
final Socket newSocket = unixDomainSocketFactory.createSocket();
285285
try {
286-
conn.bind(newSocket);
287286
final Socket socket = unixDomainSocketFactory.connectSocket(newSocket, unixDomainSocket,
288287
connectTimeout);
289-
conn.bind(socket);
290288
configureSocket(socket, socketConfig, false);
289+
conn.bind(socket);
291290
onAfterSocketConnect(context, endpointHost);
292291
if (LOG.isDebugEnabled()) {
293292
LOG.debug("{} {} connected to {}", ConnPoolSupport.getId(conn), endpointHost, unixDomainSocket);

‎httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/DefaultManagedHttpClientConnection.java‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ public class DefaultManagedHttpClientConnection
6868
private final String id;
6969
private final AtomicBoolean closed;
7070

71-
private Timeout socketTimeout;
71+
private Timeout origSocketTimeout;
7272

7373
public DefaultManagedHttpClientConnection(
7474
final String id,
@@ -132,7 +132,7 @@ public void bind(final SocketHolder socketHolder) throws IOException {
132132
throw new InterruptedIOException("Connection already shutdown");
133133
}
134134
super.bind(socketHolder);
135-
socketTimeout = Timeout.ofMilliseconds(socketHolder.getSocket().getSoTimeout());
135+
origSocketTimeout = Timeout.ofMilliseconds(socketHolder.getSocket().getSoTimeout());
136136
}
137137

138138
@Override
@@ -166,7 +166,6 @@ public void setSocketTimeout(final Timeout timeout) {
166166
LOG.debug("{} set socket timeout to {}", this.id, timeout);
167167
}
168168
super.setSocketTimeout(timeout);
169-
socketTimeout = timeout;
170169
}
171170

172171
@Override
@@ -182,15 +181,15 @@ public void close(final CloseMode closeMode) {
182181
@Override
183182
public void bind(final Socket socket) throws IOException {
184183
super.bind(WIRE_LOG.isDebugEnabled() ? new LoggingSocketHolder(socket, this.id, WIRE_LOG) : new SocketHolder(socket));
185-
socketTimeout = Timeout.ofMilliseconds(socket.getSoTimeout());
184+
origSocketTimeout = Timeout.ofMilliseconds(socket.getSoTimeout());
186185
}
187186

188187
@Override
189188
public void bind(final SSLSocket sslSocket, final Socket socket) throws IOException {
190189
super.bind(WIRE_LOG.isDebugEnabled() ?
191190
new LoggingSocketHolder(sslSocket, socket, this.id, WIRE_LOG) :
192191
new SocketHolder(sslSocket, socket));
193-
socketTimeout = Timeout.ofMilliseconds(sslSocket.getSoTimeout());
192+
origSocketTimeout = Timeout.ofMilliseconds(sslSocket.getSoTimeout());
194193
}
195194

196195
@Override
@@ -222,7 +221,7 @@ public void passivate() {
222221

223222
@Override
224223
public void activate() {
225-
super.setSocketTimeout(socketTimeout);
224+
super.setSocketTimeout(origSocketTimeout);
226225
}
227226

228227
}

0 commit comments

Comments
 (0)