Skip to content

Commit b6e827e

Browse files
rschmittrschmitt
committed
Improve socket timeout tests
This change simplifies the socket timeout tests in order to improve their reliability. Instead of asserting that the actual socket timeout was in a certain range, we take more of a pass-fail approach: if a `SocketTimeoutException` is thrown at all, then the configured timeout values must have been applied correctly within the client. This should greatly reduce or eliminate spurious test failures.
1 parent 6a1150d commit b6e827e

2 files changed

Lines changed: 44 additions & 65 deletions

File tree

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

Lines changed: 23 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -42,18 +42,15 @@
4242
import org.junit.jupiter.api.Nested;
4343
import org.junit.jupiter.api.Timeout;
4444
import org.junit.jupiter.params.ParameterizedTest;
45-
import org.junit.jupiter.params.provider.ValueSource;
45+
import org.junit.jupiter.params.provider.CsvSource;
4646

4747
import java.net.SocketTimeoutException;
4848
import java.util.concurrent.ExecutionException;
49-
import java.util.concurrent.TimeUnit;
5049

51-
import static java.lang.String.format;
5250
import static java.util.concurrent.TimeUnit.MILLISECONDS;
5351
import static org.apache.hc.core5.util.ReflectionUtils.determineJRELevel;
5452
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
5553
import static org.junit.jupiter.api.Assertions.assertThrows;
56-
import static org.junit.jupiter.api.Assertions.assertTrue;
5754
import static org.junit.jupiter.api.Assumptions.assumeFalse;
5855
import static org.junit.jupiter.api.Assumptions.assumeTrue;
5956

@@ -65,22 +62,14 @@ protected AbstractTestSocketTimeout(final URIScheme scheme, final ClientProtocol
6562

6663
@Timeout(5)
6764
@ParameterizedTest
68-
@ValueSource(strings = {
69-
"150,0,150,false",
70-
"0,150,150,false",
71-
"150,0,150,true",
72-
"0,150,150,true",
65+
@CsvSource({
66+
"10,0",
67+
"0,10",
7368
// ResponseTimeout overrides socket timeout
74-
"2000,150,150,false",
75-
"2000,150,150,true"
69+
"10000,10",
7670
})
77-
void testReadTimeouts(final String param) throws Throwable {
71+
void testReadTimeouts(final int connConfigTimeout, final int responseTimeout) throws Throwable {
7872
checkAssumptions();
79-
final String[] params = param.split(",");
80-
final int connConfigTimeout = Integer.parseInt(params[0]);
81-
final long responseTimeout = Integer.parseInt(params[1]);
82-
final long expectedDelayMs = Long.parseLong(params[2]);
83-
final boolean drip = Boolean.parseBoolean(params[3]);
8473
configureServer(bootstrap -> bootstrap
8574
.register("/random/*", AsyncRandomHandler::new));
8675
final HttpHost target = startServer();
@@ -92,26 +81,29 @@ void testReadTimeouts(final String param) throws Throwable {
9281
.setSocketTimeout(connConfigTimeout, MILLISECONDS)
9382
.build());
9483
}
84+
85+
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);
92+
}
93+
94+
closeClient(client);
95+
}
96+
97+
private SimpleHttpRequest getRequest(final int responseTimeout, final boolean drip, final HttpHost target)
98+
throws Exception {
9599
final SimpleHttpRequest request = SimpleHttpRequest.create(Method.GET, target,
96-
"/random/10240?delay=500&drip=" + (drip ? 1 : 0));
100+
"/random/10240?delay=2500&drip=" + (drip ? 1 : 0));
97101
if (responseTimeout > 0) {
98102
request.setConfig(RequestConfig.custom()
99103
.setUnixDomainSocket(getUnixDomainSocket())
100104
.setResponseTimeout(responseTimeout, MILLISECONDS).build());
101105
}
102-
103-
final long startTime = System.nanoTime();
104-
final Throwable cause = assertThrows(ExecutionException.class, () -> client.execute(request, null).get())
105-
.getCause();
106-
final long actualDelayMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime);
107-
108-
assertInstanceOf(SocketTimeoutException.class, cause);
109-
assertTrue(actualDelayMs > expectedDelayMs / 2,
110-
format("Socket read timed out too soon (only %,d out of %,d ms)", actualDelayMs, expectedDelayMs));
111-
assertTrue(actualDelayMs < expectedDelayMs * 2,
112-
format("Socket read timed out too late (%,d out of %,d ms)", actualDelayMs, expectedDelayMs));
113-
114-
closeClient(client);
106+
return request;
115107
}
116108

117109
void checkAssumptions() {

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

Lines changed: 21 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -41,16 +41,13 @@
4141
import org.junit.jupiter.api.Nested;
4242
import org.junit.jupiter.api.Timeout;
4343
import org.junit.jupiter.params.ParameterizedTest;
44-
import org.junit.jupiter.params.provider.ValueSource;
44+
import org.junit.jupiter.params.provider.CsvSource;
4545

4646
import java.net.SocketTimeoutException;
4747
import java.net.URI;
48-
import java.util.concurrent.TimeUnit;
4948

50-
import static java.lang.String.format;
5149
import static java.util.concurrent.TimeUnit.MILLISECONDS;
5250
import static org.junit.jupiter.api.Assertions.assertThrows;
53-
import static org.junit.jupiter.api.Assertions.assertTrue;
5451

5552
abstract class AbstractTestSocketTimeout extends AbstractIntegrationTestBase {
5653
protected AbstractTestSocketTimeout(final URIScheme scheme, final ClientProtocolLevel clientProtocolLevel,
@@ -60,27 +57,16 @@ protected AbstractTestSocketTimeout(final URIScheme scheme, final ClientProtocol
6057

6158
@Timeout(5)
6259
@ParameterizedTest
63-
@ValueSource(strings = {
64-
"150,0,0,150,false",
65-
"0,150,0,150,false",
66-
"150,0,0,150,true",
67-
"0,150,0,150,true",
60+
@CsvSource({
61+
"10,0,0",
62+
"0,10,0",
6863
// ConnectionConfig overrides SocketConfig
69-
"50,150,0,150,false",
70-
"1000,150,0,150,false",
71-
"50,150,0,150,true",
72-
"1000,150,0,150,true",
64+
"10000,10,0",
7365
// ResponseTimeout overrides socket timeout
74-
"2000,2000,150,150,false",
75-
"2000,2000,150,150,true"
66+
"10000,10000,10",
7667
})
77-
void testReadTimeouts(final String param) throws Exception {
78-
final String[] params = param.split(",");
79-
final int socketConfigTimeout = Integer.parseInt(params[0]);
80-
final int connConfigTimeout = Integer.parseInt(params[1]);
81-
final long responseTimeout = Integer.parseInt(params[2]);
82-
final long expectedDelayMs = Long.parseLong(params[3]);
83-
final boolean drip = Boolean.parseBoolean(params[4]);
68+
void testReadTimeouts(final int socketConfigTimeout, final int connConfigTimeout, final int responseTimeout)
69+
throws Exception {
8470
configureServer(bootstrap -> bootstrap
8571
.register("/random/*", new RandomHandler()));
8672
final HttpHost target = startServer();
@@ -97,23 +83,24 @@ void testReadTimeouts(final String param) throws Exception {
9783
.setSocketTimeout(connConfigTimeout, MILLISECONDS)
9884
.build());
9985
}
100-
final HttpGet request = new HttpGet(new URI("/random/10240?delay=1000&drip=" + (drip ? 1 : 0)));
86+
87+
for (final boolean drip : new boolean[]{ false, true }) {
88+
final HttpGet request = getRequest(responseTimeout, drip);
89+
90+
assertThrows(SocketTimeoutException.class, () ->
91+
client.execute(target, request, new BasicHttpClientResponseHandler()));
92+
}
93+
}
94+
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)));
10197
if (responseTimeout > 0) {
10298
request.setConfig(RequestConfig.custom()
10399
.setUnixDomainSocket(getUnixDomainSocket())
104100
.setResponseTimeout(responseTimeout, MILLISECONDS)
105101
.build());
106102
}
107-
108-
final long startTime = System.nanoTime();
109-
assertThrows(SocketTimeoutException.class, () ->
110-
client.execute(target, request, new BasicHttpClientResponseHandler()));
111-
final long actualDelayMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime);
112-
113-
assertTrue(actualDelayMs > expectedDelayMs / 2,
114-
format("Socket read timed out too soon (only %,d out of %,d ms)", actualDelayMs, expectedDelayMs));
115-
assertTrue(actualDelayMs < expectedDelayMs * 3,
116-
format("Socket read timed out too late (%,d out of %,d ms)", actualDelayMs, expectedDelayMs));
103+
return request;
117104
}
118105
}
119106

@@ -128,7 +115,7 @@ public Http() {
128115
@Nested
129116
class Https extends AbstractTestSocketTimeout {
130117
public Https() {
131-
super(URIScheme.HTTP, ClientProtocolLevel.STANDARD, false);
118+
super(URIScheme.HTTPS, ClientProtocolLevel.STANDARD, false);
132119
}
133120
}
134121

0 commit comments

Comments
 (0)