From 487826e13d2ea45be29f6f301e843f08251f2c67 Mon Sep 17 00:00:00 2001 From: Sam Barker Date: Thu, 8 Aug 2024 07:32:17 +0200 Subject: [PATCH] Fix jetty completion handling to forward failures (6197) Fix jetty completion handling to forward failures --- Implement onFailure to complete the asyncResponse future. --- Add changelog entry --- Test client failure handling --- remove redundant semi-colons --- Disable `expectFailure` on windows as the exceptions forwarded by the clients are completely different. --- Resolve the exception type based on the current operating system for OkHttp and thus enable the test on Windows. (cherry picked from commit 6074795d984878e5e7e1eacf2932dbcc64530c33) Signed-off-by: Marc Nuri --- CHANGELOG.md | 1 + .../client/jdkhttp/JdkHttpClientPostTest.java | 7 +++ .../client/jdkhttp/JdkHttpClientPutTest.java | 7 +++ .../jetty/JettyAsyncResponseListener.java | 11 +++- .../client/jetty/JettyHttpPostTest.java | 7 +++ .../client/jetty/JettyHttpPutTest.java | 7 +++ .../client/okhttp/OkHttpPostTest.java | 13 +++++ .../client/okhttp/OkHttpPutTest.java | 13 +++++ .../client/vertx/VertxHttpClientPostTest.java | 7 +++ .../client/vertx/VertxHttpClientPutTest.java | 7 +++ .../internal/MockDispatcher.java | 6 ++ .../client/http/AbstractHttpPostTest.java | 58 ++++++++++++++++--- .../client/http/AbstractHttpPutTest.java | 38 ++++++++++++ 13 files changed, 173 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 134080327bd..1797f3d2868 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ #### Bugs * Fix #6066: Added support for missing `v1.APIVersions` in KubernetesClient * Fix #6110: VolumeSource (and other file mode fields) in Octal are correctly interpreted +* Fix #6197: JettyHttp client error handling improvements. * Fix #6215: Suppressing rejected execution exception for port forwarder * Fix #6212: Improved reliability of file upload to Pod diff --git a/httpclient-jdk/src/test/java/io/fabric8/kubernetes/client/jdkhttp/JdkHttpClientPostTest.java b/httpclient-jdk/src/test/java/io/fabric8/kubernetes/client/jdkhttp/JdkHttpClientPostTest.java index b58ffce17dc..67fd3d56330 100644 --- a/httpclient-jdk/src/test/java/io/fabric8/kubernetes/client/jdkhttp/JdkHttpClientPostTest.java +++ b/httpclient-jdk/src/test/java/io/fabric8/kubernetes/client/jdkhttp/JdkHttpClientPostTest.java @@ -18,6 +18,8 @@ import io.fabric8.kubernetes.client.http.AbstractHttpPostTest; import io.fabric8.kubernetes.client.http.HttpClient; +import java.io.IOException; + @SuppressWarnings("java:S2187") public class JdkHttpClientPostTest extends AbstractHttpPostTest { @Override @@ -31,4 +33,9 @@ public void expectContinue() { // Apparently the JDK sets the Expect header to 100-Continue which is not according to spec. // We should consider overriding the header manually instead. } + + @Override + protected Class getConnectionFailedExceptionType() { + return IOException.class; + } } diff --git a/httpclient-jdk/src/test/java/io/fabric8/kubernetes/client/jdkhttp/JdkHttpClientPutTest.java b/httpclient-jdk/src/test/java/io/fabric8/kubernetes/client/jdkhttp/JdkHttpClientPutTest.java index e7e4054ed5a..a888cab4f13 100644 --- a/httpclient-jdk/src/test/java/io/fabric8/kubernetes/client/jdkhttp/JdkHttpClientPutTest.java +++ b/httpclient-jdk/src/test/java/io/fabric8/kubernetes/client/jdkhttp/JdkHttpClientPutTest.java @@ -18,10 +18,17 @@ import io.fabric8.kubernetes.client.http.AbstractHttpPutTest; import io.fabric8.kubernetes.client.http.HttpClient; +import java.io.IOException; + @SuppressWarnings("java:S2187") public class JdkHttpClientPutTest extends AbstractHttpPutTest { @Override protected HttpClient.Factory getHttpClientFactory() { return new JdkHttpClientFactory(); } + + @Override + protected Class getConnectionFailedExceptionType() { + return IOException.class; + } } diff --git a/httpclient-jetty/src/main/java/io/fabric8/kubernetes/client/jetty/JettyAsyncResponseListener.java b/httpclient-jetty/src/main/java/io/fabric8/kubernetes/client/jetty/JettyAsyncResponseListener.java index c2e88b2ca2d..1783b96fde1 100644 --- a/httpclient-jetty/src/main/java/io/fabric8/kubernetes/client/jetty/JettyAsyncResponseListener.java +++ b/httpclient-jetty/src/main/java/io/fabric8/kubernetes/client/jetty/JettyAsyncResponseListener.java @@ -72,7 +72,16 @@ public void onHeaders(Response response) { @Override public void onComplete(Result result) { - asyncBodyDone.complete(null); + if (result.isSucceeded()) { + asyncBodyDone.complete(null); + } else { + asyncBodyDone.completeExceptionally(result.getRequestFailure()); + } + } + + @Override + public void onFailure(Response response, Throwable failure) { + asyncResponse.completeExceptionally(failure); } public CompletableFuture> listen(Request request) { diff --git a/httpclient-jetty/src/test/java/io/fabric8/kubernetes/client/jetty/JettyHttpPostTest.java b/httpclient-jetty/src/test/java/io/fabric8/kubernetes/client/jetty/JettyHttpPostTest.java index 33fbe1d6ad9..024631d591a 100644 --- a/httpclient-jetty/src/test/java/io/fabric8/kubernetes/client/jetty/JettyHttpPostTest.java +++ b/httpclient-jetty/src/test/java/io/fabric8/kubernetes/client/jetty/JettyHttpPostTest.java @@ -18,10 +18,17 @@ import io.fabric8.kubernetes.client.http.AbstractHttpPostTest; import io.fabric8.kubernetes.client.http.HttpClient; +import java.net.ConnectException; + @SuppressWarnings("java:S2187") public class JettyHttpPostTest extends AbstractHttpPostTest { @Override protected HttpClient.Factory getHttpClientFactory() { return new JettyHttpClientFactory(); } + + @Override + protected Class getConnectionFailedExceptionType() { + return ConnectException.class; + } } diff --git a/httpclient-jetty/src/test/java/io/fabric8/kubernetes/client/jetty/JettyHttpPutTest.java b/httpclient-jetty/src/test/java/io/fabric8/kubernetes/client/jetty/JettyHttpPutTest.java index 59283a0c938..aaeae528058 100644 --- a/httpclient-jetty/src/test/java/io/fabric8/kubernetes/client/jetty/JettyHttpPutTest.java +++ b/httpclient-jetty/src/test/java/io/fabric8/kubernetes/client/jetty/JettyHttpPutTest.java @@ -18,10 +18,17 @@ import io.fabric8.kubernetes.client.http.AbstractHttpPutTest; import io.fabric8.kubernetes.client.http.HttpClient; +import java.net.ConnectException; + @SuppressWarnings("java:S2187") public class JettyHttpPutTest extends AbstractHttpPutTest { @Override protected HttpClient.Factory getHttpClientFactory() { return new JettyHttpClientFactory(); } + + @Override + protected Class getConnectionFailedExceptionType() { + return ConnectException.class; + } } diff --git a/httpclient-okhttp/src/test/java/io/fabric8/kubernetes/client/okhttp/OkHttpPostTest.java b/httpclient-okhttp/src/test/java/io/fabric8/kubernetes/client/okhttp/OkHttpPostTest.java index b5e8abf0b8a..f3f33d07dbc 100644 --- a/httpclient-okhttp/src/test/java/io/fabric8/kubernetes/client/okhttp/OkHttpPostTest.java +++ b/httpclient-okhttp/src/test/java/io/fabric8/kubernetes/client/okhttp/OkHttpPostTest.java @@ -17,6 +17,10 @@ import io.fabric8.kubernetes.client.http.AbstractHttpPostTest; import io.fabric8.kubernetes.client.http.HttpClient; +import org.junit.jupiter.api.condition.OS; + +import java.io.InterruptedIOException; +import java.net.ConnectException; @SuppressWarnings("java:S2187") public class OkHttpPostTest extends AbstractHttpPostTest { @@ -24,4 +28,13 @@ public class OkHttpPostTest extends AbstractHttpPostTest { protected HttpClient.Factory getHttpClientFactory() { return new OkHttpClientFactory(); } + + @Override + protected Class getConnectionFailedExceptionType() { + if (OS.WINDOWS.equals(OS.current())) { + return InterruptedIOException.class; + } else { + return ConnectException.class; + } + } } diff --git a/httpclient-okhttp/src/test/java/io/fabric8/kubernetes/client/okhttp/OkHttpPutTest.java b/httpclient-okhttp/src/test/java/io/fabric8/kubernetes/client/okhttp/OkHttpPutTest.java index 4f77fa9e05f..71bbab0c5b3 100644 --- a/httpclient-okhttp/src/test/java/io/fabric8/kubernetes/client/okhttp/OkHttpPutTest.java +++ b/httpclient-okhttp/src/test/java/io/fabric8/kubernetes/client/okhttp/OkHttpPutTest.java @@ -17,6 +17,10 @@ import io.fabric8.kubernetes.client.http.AbstractHttpPutTest; import io.fabric8.kubernetes.client.http.HttpClient; +import org.junit.jupiter.api.condition.OS; + +import java.io.InterruptedIOException; +import java.net.ConnectException; @SuppressWarnings("java:S2187") public class OkHttpPutTest extends AbstractHttpPutTest { @@ -24,4 +28,13 @@ public class OkHttpPutTest extends AbstractHttpPutTest { protected HttpClient.Factory getHttpClientFactory() { return new OkHttpClientFactory(); } + + @Override + protected Class getConnectionFailedExceptionType() { + if (OS.WINDOWS.equals(OS.current())) { + return InterruptedIOException.class; + } else { + return ConnectException.class; + } + } } diff --git a/httpclient-vertx/src/test/java/io/fabric8/kubernetes/client/vertx/VertxHttpClientPostTest.java b/httpclient-vertx/src/test/java/io/fabric8/kubernetes/client/vertx/VertxHttpClientPostTest.java index ef4095a54bd..c243ecc564f 100644 --- a/httpclient-vertx/src/test/java/io/fabric8/kubernetes/client/vertx/VertxHttpClientPostTest.java +++ b/httpclient-vertx/src/test/java/io/fabric8/kubernetes/client/vertx/VertxHttpClientPostTest.java @@ -18,6 +18,8 @@ import io.fabric8.kubernetes.client.http.AbstractHttpPostTest; import io.fabric8.kubernetes.client.http.HttpClient; +import java.net.ConnectException; + @SuppressWarnings("java:S2187") public class VertxHttpClientPostTest extends AbstractHttpPostTest { @Override @@ -25,4 +27,9 @@ protected HttpClient.Factory getHttpClientFactory() { return new VertxHttpClientFactory(); } + @Override + protected Class getConnectionFailedExceptionType() { + return ConnectException.class; + } + } diff --git a/httpclient-vertx/src/test/java/io/fabric8/kubernetes/client/vertx/VertxHttpClientPutTest.java b/httpclient-vertx/src/test/java/io/fabric8/kubernetes/client/vertx/VertxHttpClientPutTest.java index 5ede5806444..cc5141efe14 100644 --- a/httpclient-vertx/src/test/java/io/fabric8/kubernetes/client/vertx/VertxHttpClientPutTest.java +++ b/httpclient-vertx/src/test/java/io/fabric8/kubernetes/client/vertx/VertxHttpClientPutTest.java @@ -18,10 +18,17 @@ import io.fabric8.kubernetes.client.http.AbstractHttpPutTest; import io.fabric8.kubernetes.client.http.HttpClient; +import java.net.ConnectException; + @SuppressWarnings("java:S2187") public class VertxHttpClientPutTest extends AbstractHttpPutTest { @Override protected HttpClient.Factory getHttpClientFactory() { return new VertxHttpClientFactory(); } + + @Override + protected Class getConnectionFailedExceptionType() { + return ConnectException.class; + } } diff --git a/junit/mockwebserver/src/main/java/io/fabric8/mockwebserver/internal/MockDispatcher.java b/junit/mockwebserver/src/main/java/io/fabric8/mockwebserver/internal/MockDispatcher.java index 570b577ed31..32b818dd14c 100644 --- a/junit/mockwebserver/src/main/java/io/fabric8/mockwebserver/internal/MockDispatcher.java +++ b/junit/mockwebserver/src/main/java/io/fabric8/mockwebserver/internal/MockDispatcher.java @@ -21,6 +21,7 @@ import okhttp3.mockwebserver.Dispatcher; import okhttp3.mockwebserver.MockResponse; import okhttp3.mockwebserver.RecordedRequest; +import okhttp3.mockwebserver.SocketPolicy; import java.util.Collection; import java.util.Map; @@ -36,6 +37,11 @@ public MockDispatcher(Map> responses) { this.responses = responses; } + @Override + public MockResponse peek() { + return new MockResponse().setSocketPolicy(SocketPolicy.EXPECT_CONTINUE); + } + @Override public MockResponse dispatch(RecordedRequest request) { for (WebSocketSession webSocketSession : webSocketSessions) { diff --git a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractHttpPostTest.java b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractHttpPostTest.java index dbc9f732434..47a406f0314 100644 --- a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractHttpPostTest.java +++ b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractHttpPostTest.java @@ -23,8 +23,15 @@ import org.junit.jupiter.api.Test; import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.net.InetAddress; +import java.net.ServerSocket; +import java.net.URI; +import java.net.URISyntaxException; import java.nio.charset.StandardCharsets; import java.util.Collections; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import static org.assertj.core.api.Assertions.assertThat; @@ -46,6 +53,8 @@ static void afterAll() { protected abstract HttpClient.Factory getHttpClientFactory(); + protected abstract Class getConnectionFailedExceptionType(); + @Test @DisplayName("String body, should send a POST request with body") public void postStringBody() throws Exception { @@ -128,20 +137,53 @@ public void postFormDataBody() throws Exception { @Test public void expectContinue() throws Exception { + server.expect().post().withPath("/post-expect-continue").andReturn(200, "").always(); + // When try (HttpClient client = getHttpClientFactory().newBuilder().build()) { - client + final CompletableFuture> response = client .sendAsync(client.newHttpRequestBuilder() .post(Collections.emptyMap()) .uri(server.url("/post-expect-continue")) .expectContinue() - .build(), String.class) - .get(10L, TimeUnit.SECONDS); + .build(), String.class); + + // Then + assertThat(response).succeedsWithin(10, TimeUnit.SECONDS); + assertThat(server.getLastRequest()) + .returns("POST", RecordedRequest::getMethod) + .extracting(rr -> rr.getHeader("Expect")).asString() + .isEqualTo("100-continue"); } - // Then - assertThat(server.getLastRequest()) - .returns("POST", RecordedRequest::getMethod) - .extracting(rr -> rr.getHeader("Expect")).asString() - .isEqualTo("100-continue"); } + + @Test + public void expectFailure() throws IOException, URISyntaxException { + try (final ServerSocket serverSocket = new ServerSocket(0)) { + + try (HttpClient client = getHttpClientFactory().newBuilder().build()) { + final URI uri = uriForPath(serverSocket, "/post-failing"); + serverSocket.close(); + + // When + final CompletableFuture> response = client + .sendAsync(client.newHttpRequestBuilder() + .post("text/plain", new ByteArrayInputStream("A string body".getBytes(StandardCharsets.UTF_8)), -1) + .uri(uri) + .timeout(250, TimeUnit.MILLISECONDS) + .build(), String.class); + + // Then + assertThat(response).failsWithin(30, TimeUnit.SECONDS) + .withThrowableOfType(ExecutionException.class) + .withCauseInstanceOf(getConnectionFailedExceptionType()); + } + } + } + + private static URI uriForPath(ServerSocket socket, String path) throws URISyntaxException { + final InetAddress httpServerAddress = socket.getInetAddress(); + return new URI(String.format("http://%s:%s%s", httpServerAddress.getHostName(), socket.getLocalPort(), path)); + } + } diff --git a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractHttpPutTest.java b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractHttpPutTest.java index 31aa8e0ccb3..e24dc11a4ca 100644 --- a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractHttpPutTest.java +++ b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractHttpPutTest.java @@ -23,7 +23,14 @@ import org.junit.jupiter.api.Test; import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.net.InetAddress; +import java.net.ServerSocket; +import java.net.URI; +import java.net.URISyntaxException; import java.nio.charset.StandardCharsets; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import static org.assertj.core.api.Assertions.assertThat; @@ -44,6 +51,8 @@ static void afterAll() { protected abstract HttpClient.Factory getHttpClientFactory(); + protected abstract Class getConnectionFailedExceptionType(); + @Test @DisplayName("String body, should send a PUT request with body") public void putStringBody() throws Exception { @@ -83,4 +92,33 @@ public void putInputStreamBody() throws Exception { .extracting(rr -> rr.getHeader("Content-Type")).asString() .startsWith("text/plain"); } + + @Test + public void expectFailure() throws IOException, URISyntaxException { + try (final ServerSocket serverSocket = new ServerSocket(0)) { + + try (HttpClient client = getHttpClientFactory().newBuilder().build()) { + final URI uri = uriForPath(serverSocket, "/put-failing"); + serverSocket.close(); + + // When + final CompletableFuture> response = client + .sendAsync(client.newHttpRequestBuilder() + .put("text/plain", new ByteArrayInputStream("A string body".getBytes(StandardCharsets.UTF_8)), -1) + .uri(uri) + .timeout(250, TimeUnit.MILLISECONDS) + .build(), String.class); + + // Then + assertThat(response).failsWithin(30, TimeUnit.SECONDS) + .withThrowableOfType(ExecutionException.class) + .withCauseInstanceOf(getConnectionFailedExceptionType()); + } + } + } + + private static URI uriForPath(ServerSocket socket, String path) throws URISyntaxException { + final InetAddress httpServerAddress = socket.getInetAddress(); + return new URI(String.format("http://%s:%s%s", httpServerAddress.getHostName(), socket.getLocalPort(), path)); + } }