Skip to content

Commit 883f123

Browse files
cfredri4rstoyanchev
authored andcommitted
Use only one timeout in JdkClientHttpRequest
Previously, a timeout was set both on HttpRequest, and used on httpClient.sendAsync().get(). This leads to inconsistent behaviour depending on which timeout gets triggered first. See gh-33090
1 parent 073cfd4 commit 883f123

File tree

2 files changed

+41
-29
lines changed

2 files changed

+41
-29
lines changed

spring-web/src/main/java/org/springframework/http/client/JdkClientHttpRequest.java

Lines changed: 33 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -16,24 +16,26 @@
1616

1717
package org.springframework.http.client;
1818

19+
import java.io.FilterInputStream;
1920
import java.io.IOException;
2021
import java.io.InputStream;
2122
import java.io.UncheckedIOException;
2223
import java.net.URI;
2324
import java.net.http.HttpClient;
2425
import java.net.http.HttpRequest;
2526
import java.net.http.HttpResponse;
27+
import java.net.http.HttpTimeoutException;
2628
import java.nio.ByteBuffer;
2729
import java.time.Duration;
2830
import java.util.Collections;
2931
import java.util.Set;
3032
import java.util.TreeSet;
33+
import java.util.concurrent.CancellationException;
34+
import java.util.concurrent.CompletableFuture;
3135
import java.util.concurrent.ExecutionException;
3236
import java.util.concurrent.Executor;
3337
import java.util.concurrent.Flow;
3438
import java.util.concurrent.TimeUnit;
35-
import java.util.concurrent.TimeoutException;
36-
3739
import org.springframework.http.HttpHeaders;
3840
import org.springframework.http.HttpMethod;
3941
import org.springframework.lang.Nullable;
@@ -92,28 +94,46 @@ public URI getURI() {
9294
@Override
9395
@SuppressWarnings("NullAway")
9496
protected ClientHttpResponse executeInternal(HttpHeaders headers, @Nullable Body body) throws IOException {
97+
HttpRequest request = buildRequest(headers, body);
98+
CompletableFuture<HttpResponse<InputStream>> responsefuture =
99+
this.httpClient.sendAsync(request, HttpResponse.BodyHandlers.ofInputStream());
95100
try {
96-
HttpRequest request = buildRequest(headers, body);
97-
HttpResponse<InputStream> response;
98101
if (this.timeout != null) {
99-
response = this.httpClient.sendAsync(request, HttpResponse.BodyHandlers.ofInputStream())
100-
.get(this.timeout.toMillis(), TimeUnit.MILLISECONDS);
101-
}
102-
else {
103-
response = this.httpClient.send(request, HttpResponse.BodyHandlers.ofInputStream());
102+
CompletableFuture<Void> timeoutFuture = new CompletableFuture<Void>()
103+
.completeOnTimeout(null, this.timeout.toMillis(), TimeUnit.MILLISECONDS);
104+
timeoutFuture.thenRun(() -> {
105+
if (!responsefuture.cancel(true) && !responsefuture.isCompletedExceptionally()) {
106+
try {
107+
responsefuture.resultNow().body().close();
108+
} catch (IOException ignored) {}
109+
}
110+
});
111+
var response = responsefuture.get();
112+
return new JdkClientHttpResponse(response.statusCode(), response.headers(), new FilterInputStream(response.body()) {
113+
114+
@Override
115+
public void close() throws IOException {
116+
timeoutFuture.cancel(false);
117+
super.close();
118+
}
119+
});
120+
121+
} else {
122+
var response = responsefuture.get();
123+
return new JdkClientHttpResponse(response.statusCode(), response.headers(), response.body());
104124
}
105-
return new JdkClientHttpResponse(response);
106-
}
107-
catch (UncheckedIOException ex) {
108-
throw ex.getCause();
109125
}
110126
catch (InterruptedException ex) {
111127
Thread.currentThread().interrupt();
128+
responsefuture.cancel(true);
112129
throw new IOException("Request was interrupted: " + ex.getMessage(), ex);
113130
}
114131
catch (ExecutionException ex) {
115132
Throwable cause = ex.getCause();
116133

134+
if (cause instanceof CancellationException caEx) {
135+
throw new HttpTimeoutException("Request timed out");
136+
}
117137
if (cause instanceof UncheckedIOException uioEx) {
118138
throw uioEx.getCause();
119139
}
@@ -127,17 +147,11 @@ else if (cause instanceof IOException ioEx) {
127147
throw new IOException(cause.getMessage(), cause);
128148
}
129149
}
130-
catch (TimeoutException ex) {
131-
throw new IOException("Request timed out: " + ex.getMessage(), ex);
132-
}
133150
}
134151

135152

136153
private HttpRequest buildRequest(HttpHeaders headers, @Nullable Body body) {
137154
HttpRequest.Builder builder = HttpRequest.newBuilder().uri(this.uri);
138-
if (this.timeout != null) {
139-
builder.timeout(this.timeout);
140-
}
141155

142156
headers.forEach((headerName, headerValues) -> {
143157
if (!DISALLOWED_HEADERS.contains(headerName.toLowerCase())) {

spring-web/src/main/java/org/springframework/http/client/JdkClientHttpResponse.java

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
import java.io.IOException;
2020
import java.io.InputStream;
2121
import java.net.http.HttpClient;
22-
import java.net.http.HttpResponse;
2322
import java.util.List;
2423
import java.util.Locale;
2524
import java.util.Map;
@@ -41,22 +40,21 @@
4140
*/
4241
class JdkClientHttpResponse implements ClientHttpResponse {
4342

44-
private final HttpResponse<InputStream> response;
43+
private final int statusCode;
4544

4645
private final HttpHeaders headers;
4746

4847
private final InputStream body;
4948

5049

51-
public JdkClientHttpResponse(HttpResponse<InputStream> response) {
52-
this.response = response;
53-
this.headers = adaptHeaders(response);
54-
InputStream inputStream = response.body();
55-
this.body = (inputStream != null ? inputStream : InputStream.nullInputStream());
50+
public JdkClientHttpResponse(int statusCode, java.net.http.HttpHeaders headers, InputStream body) {
51+
this.statusCode = statusCode;
52+
this.headers = adaptHeaders(headers);
53+
this.body = body != null ? body : InputStream.nullInputStream();
5654
}
5755

58-
private static HttpHeaders adaptHeaders(HttpResponse<?> response) {
59-
Map<String, List<String>> rawHeaders = response.headers().map();
56+
private static HttpHeaders adaptHeaders(java.net.http.HttpHeaders headers) {
57+
Map<String, List<String>> rawHeaders = headers.map();
6058
Map<String, List<String>> map = new LinkedCaseInsensitiveMap<>(rawHeaders.size(), Locale.ENGLISH);
6159
MultiValueMap<String, String> multiValueMap = CollectionUtils.toMultiValueMap(map);
6260
multiValueMap.putAll(rawHeaders);
@@ -66,7 +64,7 @@ private static HttpHeaders adaptHeaders(HttpResponse<?> response) {
6664

6765
@Override
6866
public HttpStatusCode getStatusCode() {
69-
return HttpStatusCode.valueOf(this.response.statusCode());
67+
return HttpStatusCode.valueOf(statusCode);
7068
}
7169

7270
@Override

0 commit comments

Comments
 (0)