Skip to content

Commit 88fb41f

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 was instead setting 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 88fb41f

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)