From c775b4eb10884c16e24e169fec5766f4d4f8b5ee Mon Sep 17 00:00:00 2001 From: Michele Rastelli Date: Mon, 7 Nov 2022 20:28:17 +0100 Subject: [PATCH 1/2] CodeQL fixes --- .github/workflows/codeql.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index a50a410d9..14392e1ac 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -28,6 +28,11 @@ jobs: analyze: name: Analyze runs-on: ubuntu-latest + + defaults: + run: + working-directory: driver + permissions: actions: read contents: read From 8555fb150ee52ca3da0dd1175859cdb69962e3a1 Mon Sep 17 00:00:00 2001 From: Michele Rastelli Date: Tue, 8 Nov 2022 12:04:10 +0100 Subject: [PATCH 2/2] added http retries for safe methods --- .../internal/http/HttpCommunication.java | 16 +- .../internal/http/HttpConnection.java | 7 +- .../java/resilience/SingleServerTest.java | 22 +- .../resilience/connection/ConnectionTest.java | 14 +- .../reconnection/ReconnectionTest.java | 119 ----------- .../test/java/resilience/retry/RetryTest.java | 197 ++++++++++++++++++ .../src/test/resources/logback-test.xml | 2 +- 7 files changed, 238 insertions(+), 139 deletions(-) delete mode 100644 resilience-tests/src/test/java/resilience/reconnection/ReconnectionTest.java create mode 100644 resilience-tests/src/test/java/resilience/retry/RetryTest.java diff --git a/driver/src/main/java/com/arangodb/internal/http/HttpCommunication.java b/driver/src/main/java/com/arangodb/internal/http/HttpCommunication.java index 82229676c..0ce212159 100644 --- a/driver/src/main/java/com/arangodb/internal/http/HttpCommunication.java +++ b/driver/src/main/java/com/arangodb/internal/http/HttpCommunication.java @@ -22,6 +22,7 @@ import com.arangodb.ArangoDBException; import com.arangodb.Request; +import com.arangodb.RequestType; import com.arangodb.Response; import com.arangodb.internal.net.*; import com.arangodb.internal.serde.InternalSerde; @@ -84,8 +85,6 @@ private Response execute(final Request request, final HostHandle hostHandle, fin return response; } catch (final SocketTimeoutException e) { // SocketTimeoutException exceptions are wrapped and rethrown. - // Differently from other IOException exceptions they must not be retried, - // since the requests could not be idempotent. TimeoutException te = new TimeoutException(e.getMessage()); te.initCause(e); throw new ArangoDBException(te, reqId); @@ -96,7 +95,7 @@ private Response execute(final Request request, final HostHandle hostHandle, fin } final Host failedHost = host; host = hostHandler.get(hostHandle, accessType); - if (host != null) { + if (host != null && isSafe(request)) { LOGGER.warn("Could not connect to {} while executing request [id={}]", failedHost.getDescription(), reqId, e); LOGGER.debug("Try connecting to {}", host.getDescription()); @@ -106,9 +105,9 @@ private Response execute(final Request request, final HostHandle hostHandle, fin } } } - } catch (final ArangoDBException e) { - if (e instanceof ArangoDBRedirectException && attemptCount < 3) { - final String location = ((ArangoDBRedirectException) e).getLocation(); + } catch (final ArangoDBRedirectException e) { + if (attemptCount < 3) { + final String location = e.getLocation(); final HostDescription redirectHost = HostUtils.createFromLocation(location); hostHandler.failIfNotMatch(redirectHost, e); return execute(request, new HostHandle().setHost(redirectHost), attemptCount + 1); @@ -118,6 +117,11 @@ private Response execute(final Request request, final HostHandle hostHandle, fin } } + private boolean isSafe(final Request request) { + RequestType type = request.getRequestType(); + return type == RequestType.GET || type == RequestType.HEAD || type == RequestType.OPTIONS; + } + public static class Builder { private HostHandler hostHandler; private InternalSerde serde; diff --git a/driver/src/main/java/com/arangodb/internal/http/HttpConnection.java b/driver/src/main/java/com/arangodb/internal/http/HttpConnection.java index 3bd43813a..65fef09af 100644 --- a/driver/src/main/java/com/arangodb/internal/http/HttpConnection.java +++ b/driver/src/main/java/com/arangodb/internal/http/HttpConnection.java @@ -61,6 +61,7 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; @@ -227,10 +228,12 @@ public Response execute(final Request request) throws IOException { throw ArangoDBException.wrap(e); } catch (ExecutionException e) { Throwable cause = e.getCause(); - if (cause instanceof IOException) { + if (cause instanceof TimeoutException) { + throw ArangoDBException.wrap(cause); + } else if (cause instanceof IOException) { throw (IOException) cause; } else { - throw ArangoDBException.wrap(e.getCause()); + throw new IOException(cause); } } checkError(resp); diff --git a/resilience-tests/src/test/java/resilience/SingleServerTest.java b/resilience-tests/src/test/java/resilience/SingleServerTest.java index 6ba01612e..7e9bc32b6 100644 --- a/resilience-tests/src/test/java/resilience/SingleServerTest.java +++ b/resilience-tests/src/test/java/resilience/SingleServerTest.java @@ -37,8 +37,8 @@ static void afterAll() throws IOException { } @BeforeEach - void beforeEach() throws IOException { - endpoint.getProxy().enable(); + void beforeEach() { + enableEndpoint(); } protected static Endpoint getEndpoint() { @@ -57,4 +57,22 @@ protected static ArangoDBAsync.Builder dbBuilderAsync() { .password(PASSWORD); } + protected void enableEndpoint(){ + try { + getEndpoint().getProxy().enable(); + Thread.sleep(100); + } catch (IOException | InterruptedException e) { + throw new RuntimeException(e); + } + } + + protected void disableEndpoint(){ + try { + getEndpoint().getProxy().disable(); + Thread.sleep(100); + } catch (IOException | InterruptedException e) { + throw new RuntimeException(e); + } + } + } diff --git a/resilience-tests/src/test/java/resilience/connection/ConnectionTest.java b/resilience-tests/src/test/java/resilience/connection/ConnectionTest.java index 6c2453acf..24e484282 100644 --- a/resilience-tests/src/test/java/resilience/connection/ConnectionTest.java +++ b/resilience-tests/src/test/java/resilience/connection/ConnectionTest.java @@ -8,7 +8,6 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; -import java.io.IOException; import java.net.ConnectException; import java.net.UnknownHostException; import java.util.stream.Stream; @@ -59,21 +58,18 @@ void nameResolutionFailTest(Protocol protocol) { @ParameterizedTest @MethodSource("arangoProvider") - void connectionFailTest(ArangoDB arangoDB) throws IOException, InterruptedException { - getEndpoint().getProxy().disable(); - Thread.sleep(100); + void connectionFailTest(ArangoDB arangoDB) { + disableEndpoint(); Throwable thrown = catchThrowable(arangoDB::getVersion); assertThat(thrown).isInstanceOf(ArangoDBException.class); assertThat(thrown.getMessage()).contains("Cannot contact any host"); assertThat(thrown.getCause()).isNotNull(); assertThat(thrown.getCause()).isInstanceOf(ArangoDBMultipleException.class); - ((ArangoDBMultipleException) thrown.getCause()).getExceptions().forEach(e -> { - assertThat(e).isInstanceOf(ConnectException.class); - }); + ((ArangoDBMultipleException) thrown.getCause()).getExceptions().forEach(e -> + assertThat(e).isInstanceOf(ConnectException.class)); arangoDB.shutdown(); - getEndpoint().getProxy().enable(); - Thread.sleep(100); + enableEndpoint(); } } diff --git a/resilience-tests/src/test/java/resilience/reconnection/ReconnectionTest.java b/resilience-tests/src/test/java/resilience/reconnection/ReconnectionTest.java deleted file mode 100644 index 276e5236e..000000000 --- a/resilience-tests/src/test/java/resilience/reconnection/ReconnectionTest.java +++ /dev/null @@ -1,119 +0,0 @@ -package resilience.reconnection; - -import ch.qos.logback.classic.Level; -import com.arangodb.ArangoDB; -import com.arangodb.ArangoDBException; -import com.arangodb.ArangoDBMultipleException; -import com.arangodb.Protocol; -import org.junit.jupiter.params.provider.EnumSource; -import resilience.SingleServerTest; -import eu.rekawek.toxiproxy.model.ToxicDirection; -import eu.rekawek.toxiproxy.model.toxic.Latency; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.MethodSource; - -import java.io.IOException; -import java.net.ConnectException; -import java.util.concurrent.TimeoutException; -import java.util.stream.Stream; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.catchThrowable; -import static org.junit.jupiter.api.Assumptions.assumeTrue; - -/** - * @author Michele Rastelli - */ -class ReconnectionTest extends SingleServerTest { - - static Stream arangoProvider() { - return Stream.of( - dbBuilder().timeout(1_000).useProtocol(Protocol.VST).build(), - dbBuilder().timeout(1_000).useProtocol(Protocol.HTTP_VPACK).build(), - dbBuilder().timeout(1_000).useProtocol(Protocol.HTTP2_VPACK).build() - ); - } - - /** - * on reconnection failure: - * - 3x logs WARN Could not connect to host[addr=127.0.0.1,port=8529] - * - ArangoDBException("Cannot contact any host") - *

- * once the proxy is re-enabled: - * - the subsequent requests should be successful - */ - @ParameterizedTest - @MethodSource("arangoProvider") - void unreachableHost(ArangoDB arangoDB) throws IOException, InterruptedException { - arangoDB.getVersion(); - - // close the driver connection - getEndpoint().getProxy().disable(); - Thread.sleep(100); - - for (int i = 0; i < 10; i++) { - Throwable thrown = catchThrowable(arangoDB::getVersion); - assertThat(thrown).isInstanceOf(ArangoDBException.class); - assertThat(thrown.getMessage()).contains("Cannot contact any host"); - assertThat(thrown.getCause()).isNotNull(); - assertThat(thrown.getCause()).isInstanceOf(ArangoDBMultipleException.class); - ((ArangoDBMultipleException) thrown.getCause()).getExceptions().forEach(e -> { - assertThat(e).isInstanceOf(ConnectException.class); - }); - } - - long warnsCount = logs.getLoggedEvents().stream() - .filter(e -> e.getLevel().equals(Level.WARN)) - .filter(e -> e.getMessage().contains("Could not connect to host[addr=127.0.0.1,port=18529]")) - .count(); - assertThat(warnsCount).isGreaterThanOrEqualTo(3); - - getEndpoint().getProxy().enable(); - Thread.sleep(100); - - arangoDB.getVersion(); - arangoDB.shutdown(); - } - - /** - * on reconnection failure: - * - 3x logs WARN Could not connect to host[addr=127.0.0.1,port=8529] - * - ArangoDBException("Cannot contact any host") - *

- * once the proxy is re-enabled: - * - the subsequent requests should be successful - */ - @ParameterizedTest - @EnumSource(Protocol.class) - void connectionTimeout(Protocol protocol) throws IOException, InterruptedException { - // https://github.com/vert-x3/vertx-web/issues/2296 - // WebClient: HTTP/2 request timeout does not throw TimeoutException - assumeTrue(protocol != Protocol.HTTP2_VPACK); - assumeTrue(protocol != Protocol.HTTP2_JSON); - - ArangoDB arangoDB = dbBuilder() - .timeout(1_000) - .useProtocol(protocol) - .build(); - - arangoDB.getVersion(); - - // slow down the driver connection - Latency toxic = getEndpoint().getProxy().toxics().latency("latency", ToxicDirection.DOWNSTREAM, 10_000); - Thread.sleep(100); - - Throwable thrown = catchThrowable(arangoDB::getVersion); - thrown.printStackTrace(); - assertThat(thrown) - .isInstanceOf(ArangoDBException.class) - .extracting(Throwable::getCause) - .isInstanceOf(TimeoutException.class); - - toxic.remove(); - Thread.sleep(100); - - arangoDB.getVersion(); - arangoDB.shutdown(); - } - -} diff --git a/resilience-tests/src/test/java/resilience/retry/RetryTest.java b/resilience-tests/src/test/java/resilience/retry/RetryTest.java new file mode 100644 index 000000000..6eb074928 --- /dev/null +++ b/resilience-tests/src/test/java/resilience/retry/RetryTest.java @@ -0,0 +1,197 @@ +package resilience.retry; + +import ch.qos.logback.classic.Level; +import com.arangodb.ArangoDB; +import com.arangodb.ArangoDBException; +import com.arangodb.ArangoDBMultipleException; +import com.arangodb.Protocol; +import io.vertx.core.http.HttpClosedException; +import org.junit.jupiter.params.provider.EnumSource; +import resilience.SingleServerTest; +import eu.rekawek.toxiproxy.model.ToxicDirection; +import eu.rekawek.toxiproxy.model.toxic.Latency; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; + +import java.io.IOException; +import java.net.ConnectException; +import java.util.List; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.stream.Stream; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.catchThrowable; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +/** + * @author Michele Rastelli + */ +class RetryTest extends SingleServerTest { + + static Stream arangoProvider() { + return Stream.of( + dbBuilder().timeout(1_000).useProtocol(Protocol.VST).build(), + dbBuilder().timeout(1_000).useProtocol(Protocol.HTTP_VPACK).build(), + dbBuilder().timeout(1_000).useProtocol(Protocol.HTTP2_VPACK).build() + ); + } + + /** + * on reconnection failure: - 3x logs WARN Could not connect to host[addr=127.0.0.1,port=8529] - + * ArangoDBException("Cannot contact any host") + *

+ * once the proxy is re-enabled: - the subsequent requests should be successful + */ + @ParameterizedTest + @MethodSource("arangoProvider") + void unreachableHost(ArangoDB arangoDB) { + arangoDB.getVersion(); + disableEndpoint(); + + for (int i = 0; i < 10; i++) { + Throwable thrown = catchThrowable(arangoDB::getVersion); + assertThat(thrown).isInstanceOf(ArangoDBException.class); + assertThat(thrown.getMessage()).contains("Cannot contact any host"); + assertThat(thrown.getCause()).isNotNull(); + assertThat(thrown.getCause()).isInstanceOf(ArangoDBMultipleException.class); + ((ArangoDBMultipleException) thrown.getCause()).getExceptions().forEach(e -> { + assertThat(e).isInstanceOf(ConnectException.class); + }); + } + + long warnsCount = logs.getLoggedEvents().stream() + .filter(e -> e.getLevel().equals(Level.WARN)) + .filter(e -> e.getMessage().contains("Could not connect to host[addr=127.0.0.1,port=18529]")) + .count(); + assertThat(warnsCount).isGreaterThanOrEqualTo(3); + + enableEndpoint(); + arangoDB.getVersion(); + arangoDB.shutdown(); + } + + /** + * on delayed response: + * - ArangoDBException with cause TimeoutException + *

+ * once the delay is removed: + * - the subsequent requests should be successful + */ + @ParameterizedTest + @EnumSource(Protocol.class) + void connectionTimeout(Protocol protocol) throws IOException, InterruptedException { + // https://github.com/vert-x3/vertx-web/issues/2296 + // WebClient: HTTP/2 request timeout does not throw TimeoutException + assumeTrue(protocol != Protocol.HTTP2_VPACK); + assumeTrue(protocol != Protocol.HTTP2_JSON); + + ArangoDB arangoDB = dbBuilder() + .timeout(1_000) + .useProtocol(protocol) + .build(); + + arangoDB.getVersion(); + + // slow down the driver connection + Latency toxic = getEndpoint().getProxy().toxics().latency("latency", ToxicDirection.DOWNSTREAM, 10_000); + Thread.sleep(100); + + Throwable thrown = catchThrowable(arangoDB::getVersion); + thrown.printStackTrace(); + assertThat(thrown) + .isInstanceOf(ArangoDBException.class) + .extracting(Throwable::getCause) + .isInstanceOf(TimeoutException.class); + + toxic.remove(); + Thread.sleep(100); + + arangoDB.getVersion(); + arangoDB.shutdown(); + } + + + /** + * on closed pending requests of safe HTTP methods: - retry 3 times - ArangoDBMultipleException with 3 exceptions + *

+ * once restored: - the subsequent requests should be successful + */ + @ParameterizedTest + @EnumSource(Protocol.class) + void retryGetOnClosedConnection(Protocol protocol) throws IOException, InterruptedException { + assumeTrue(protocol != Protocol.VST); + ArangoDB arangoDB = dbBuilder() + .useProtocol(protocol) + .build(); + + arangoDB.getVersion(); + + // slow down the driver connection + Latency toxic = getEndpoint().getProxy().toxics().latency("latency", ToxicDirection.DOWNSTREAM, 10_000); + Thread.sleep(100); + + ScheduledExecutorService es = Executors.newSingleThreadScheduledExecutor(); + es.schedule(this::disableEndpoint, 300, TimeUnit.MILLISECONDS); + + Throwable thrown = catchThrowable(arangoDB::getVersion); + thrown.printStackTrace(); + assertThat(thrown).isInstanceOf(ArangoDBException.class); + assertThat(thrown.getCause()).isInstanceOf(ArangoDBMultipleException.class); + List exceptions = ((ArangoDBMultipleException) thrown.getCause()).getExceptions(); + assertThat(exceptions).hasSize(3); + assertThat(exceptions.get(0)).isInstanceOf(IOException.class); + assertThat(exceptions.get(0).getCause()).isInstanceOf(HttpClosedException.class); + assertThat(exceptions.get(1)).isInstanceOf(ConnectException.class); + assertThat(exceptions.get(2)).isInstanceOf(ConnectException.class); + + toxic.remove(); + Thread.sleep(100); + enableEndpoint(); + + arangoDB.getVersion(); + arangoDB.shutdown(); + es.shutdown(); + } + + + /** + * on closed pending requests of unsafe HTTP methods: - no retry should happen + *

+ * once restored: - the subsequent requests should be successful + */ + @ParameterizedTest + @EnumSource(Protocol.class) + void notRetryPostOnClosedConnection(Protocol protocol) throws IOException, InterruptedException { + assumeTrue(protocol != Protocol.VST); + ArangoDB arangoDB = dbBuilder() + .useProtocol(protocol) + .build(); + + arangoDB.db().query("return null", Void.class); + + // slow down the driver connection + Latency toxic = getEndpoint().getProxy().toxics().latency("latency", ToxicDirection.DOWNSTREAM, 10_000); + Thread.sleep(100); + + ScheduledExecutorService es = Executors.newSingleThreadScheduledExecutor(); + es.schedule(this::disableEndpoint, 300, TimeUnit.MILLISECONDS); + + Throwable thrown = catchThrowable(() -> arangoDB.db().query("return null", Void.class)); + thrown.printStackTrace(); + assertThat(thrown).isInstanceOf(ArangoDBException.class); + assertThat(thrown.getCause()).isInstanceOf(IOException.class); + assertThat(thrown.getCause().getCause()).isInstanceOf(HttpClosedException.class); + + toxic.remove(); + Thread.sleep(100); + enableEndpoint(); + + arangoDB.db().query("return null", Void.class); + arangoDB.shutdown(); + es.shutdown(); + } + +} diff --git a/resilience-tests/src/test/resources/logback-test.xml b/resilience-tests/src/test/resources/logback-test.xml index 840d133d7..343f4dfdd 100644 --- a/resilience-tests/src/test/resources/logback-test.xml +++ b/resilience-tests/src/test/resources/logback-test.xml @@ -6,7 +6,7 @@ - +