From 0d2c3e43089bcdb1aa775ed755cf5ebe079833a4 Mon Sep 17 00:00:00 2001 From: giampaolo Date: Sat, 5 Apr 2025 14:19:06 +0200 Subject: [PATCH] CancellationExceptions are thrown instead of the expected ResourceAccessException during timeout scenarios. Handle the CancellationExceptions in order to throw an ResourceAccessException when timeout occurred Closes gh-33973 Signed-off-by: giampaolo fix: use timeoutHandler with a flag isTimeout Closes gh-33973 Signed-off-by: giampaolo --- .../http/client/JdkClientHttpRequest.java | 21 +++- .../http/client/JdkClientHttpRequestTest.java | 117 ++++++++++++++++++ 2 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 spring-web/src/test/java/org/springframework/http/client/JdkClientHttpRequestTest.java diff --git a/spring-web/src/main/java/org/springframework/http/client/JdkClientHttpRequest.java b/spring-web/src/main/java/org/springframework/http/client/JdkClientHttpRequest.java index 060bef8dde28..b20cbbda569c 100644 --- a/spring-web/src/main/java/org/springframework/http/client/JdkClientHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/client/JdkClientHttpRequest.java @@ -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; @@ -96,12 +97,13 @@ public URI getURI() { @Override protected ClientHttpResponse executeInternal(HttpHeaders headers, @Nullable Body body) throws IOException { CompletableFuture> responseFuture = null; + TimeoutHandler timeoutHandler = null; 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 response = responseFuture.get(); InputStream inputStream = timeoutHandler.wrapInputStream(response); return new JdkClientHttpResponse(response, inputStream); @@ -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(); @@ -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) { @@ -224,6 +235,7 @@ public ByteBuffer map(byte[] b, int off, int len) { private static final class TimeoutHandler { private final CompletableFuture timeoutFuture; + private final AtomicBoolean isTimeout = new AtomicBoolean(false); private TimeoutHandler(CompletableFuture> future, Duration timeout) { @@ -232,6 +244,7 @@ private TimeoutHandler(CompletableFuture> future, Dura this.timeoutFuture.thenRun(() -> { if (future.cancel(true) || future.isCompletedExceptionally() || !future.isDone()) { + isTimeout.set(true); return; } try { @@ -258,6 +271,10 @@ public void close() throws IOException { } }; } + + public boolean isTimeout() { + return isTimeout.get(); + } } } diff --git a/spring-web/src/test/java/org/springframework/http/client/JdkClientHttpRequestTest.java b/spring-web/src/test/java/org/springframework/http/client/JdkClientHttpRequestTest.java new file mode 100644 index 000000000000..86630e1fd37c --- /dev/null +++ b/spring-web/src/test/java/org/springframework/http/client/JdkClientHttpRequestTest.java @@ -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> 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> 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(); + } + +}