Skip to content

Fix throws only ResourceAccessException on timeout #34721

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.util.concurrent.Executor;
import java.util.concurrent.Flow;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;

import org.jspecify.annotations.Nullable;

Expand Down Expand Up @@ -96,12 +97,13 @@ public URI getURI() {
@Override
protected ClientHttpResponse executeInternal(HttpHeaders headers, @Nullable Body body) throws IOException {
CompletableFuture<HttpResponse<InputStream>> responseFuture = null;
TimeoutHandler timeoutHandler = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be no purpose to this change that I can see. Is this intended?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change was to make the TimeoutHandler readable outside the try

try {
HttpRequest request = buildRequest(headers, body);
responseFuture = this.httpClient.sendAsync(request, HttpResponse.BodyHandlers.ofInputStream());

if (this.timeout != null) {
TimeoutHandler timeoutHandler = new TimeoutHandler(responseFuture, this.timeout);
timeoutHandler = new TimeoutHandler(responseFuture, this.timeout);
HttpResponse<InputStream> response = responseFuture.get();
InputStream inputStream = timeoutHandler.wrapInputStream(response);
return new JdkClientHttpResponse(response, inputStream);
Expand All @@ -120,7 +122,10 @@ protected ClientHttpResponse executeInternal(HttpHeaders headers, @Nullable Body
Throwable cause = ex.getCause();

if (cause instanceof CancellationException) {
throw new HttpTimeoutException("Request timed out");
if (timeoutHandler != null && timeoutHandler.isTimeout()) {
throw new HttpTimeoutException("Request timed out");
}
throw new IOException("Request was cancelled");
}
if (cause instanceof UncheckedIOException uioEx) {
throw uioEx.getCause();
Expand All @@ -136,6 +141,12 @@ else if (cause instanceof IOException ioEx) {
throw (message == null ? new IOException(cause) : new IOException(message, cause));
}
}
catch (CancellationException ex) {
if (timeoutHandler != null && timeoutHandler.isTimeout()) {
throw new HttpTimeoutException("Request timed out");
}
throw new IOException("Request was cancelled");
}
}

private HttpRequest buildRequest(HttpHeaders headers, @Nullable Body body) {
Expand Down Expand Up @@ -224,6 +235,7 @@ public ByteBuffer map(byte[] b, int off, int len) {
private static final class TimeoutHandler {

private final CompletableFuture<Void> timeoutFuture;
private final AtomicBoolean isTimeout = new AtomicBoolean(false);

private TimeoutHandler(CompletableFuture<HttpResponse<InputStream>> future, Duration timeout) {

Expand All @@ -232,6 +244,7 @@ private TimeoutHandler(CompletableFuture<HttpResponse<InputStream>> future, Dura

this.timeoutFuture.thenRun(() -> {
if (future.cancel(true) || future.isCompletedExceptionally() || !future.isDone()) {
isTimeout.set(true);
return;
}
try {
Expand All @@ -258,6 +271,10 @@ public void close() throws IOException {
}
};
}

public boolean isTimeout() {
return isTimeout.get();
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
package org.springframework.http.client;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.Mockito.*;

import java.io.IOException;
import java.io.InputStream;
import java.net.URI;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
import java.net.http.HttpTimeoutException;
import java.time.Duration;
import java.util.concurrent.*;

import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpMethod;

class JdkClientHttpRequestTest {

private HttpClient mockHttpClient;
private URI uri = URI.create("http://example.com");
private HttpMethod method = HttpMethod.GET;

private ExecutorService executor;

@BeforeEach
void setup() {
mockHttpClient = mock(HttpClient.class);
executor = Executors.newSingleThreadExecutor();
}

@AfterEach
void tearDown() {
executor.shutdownNow();
}

@Test
void executeInternal_withTimeout_shouldThrowHttpTimeoutException() throws Exception {
Duration timeout = Duration.ofMillis(10);

JdkClientHttpRequest request = new JdkClientHttpRequest(mockHttpClient, uri, method, executor, timeout);

CompletableFuture<HttpResponse<InputStream>> future = new CompletableFuture<>();

when(mockHttpClient.sendAsync(any(HttpRequest.class), any(HttpResponse.BodyHandler.class)))
.thenReturn(future);

HttpHeaders headers = new HttpHeaders();

CountDownLatch startLatch = new CountDownLatch(1);

// Cancellation thread waits for startLatch, then cancels the future after a delay
Thread canceller = new Thread(() -> {
try {
startLatch.await();
Thread.sleep(500);
future.cancel(true);
} catch (InterruptedException ignored) {
}
});
canceller.start();

IOException ex = assertThrows(IOException.class, () -> {
startLatch.countDown();
request.executeInternal(headers, null);
});

assertThat(ex)
.isInstanceOf(HttpTimeoutException.class)
.hasMessage("Request timed out");

canceller.join();
}

@Test
void executeInternal_withTimeout_shouldThrowIOException() throws Exception {
Duration timeout = Duration.ofMillis(500);

JdkClientHttpRequest request = new JdkClientHttpRequest(mockHttpClient, uri, method, executor, timeout);

CompletableFuture<HttpResponse<InputStream>> future = new CompletableFuture<>();

when(mockHttpClient.sendAsync(any(HttpRequest.class), any(HttpResponse.BodyHandler.class)))
.thenReturn(future);

HttpHeaders headers = new HttpHeaders();

CountDownLatch startLatch = new CountDownLatch(1);

Thread canceller = new Thread(() -> {
try {
startLatch.await();
Thread.sleep(10);
future.cancel(true);
} catch (InterruptedException ignored) {
}
});
canceller.start();

IOException ex = assertThrows(IOException.class, () -> {
startLatch.countDown();
request.executeInternal(headers, null);
});

assertThat(ex)
.isInstanceOf(IOException.class)
.hasMessage("Request was cancelled");

canceller.join();
}

}