From 7d1e97085ef7f6ada7db446015556011b175562f Mon Sep 17 00:00:00 2001 From: Andreas Fankhauser <23085769+hiddenalpha@users.noreply.github.com> Date: Tue, 21 May 2024 17:00:16 +0200 Subject: [PATCH] [SDCISA-15833, https://github.com/swisspost/vertx-redisques/issues/170] Introduce ExceptionFactory --- .../exception/GateleenExceptionFactory.java | 40 +++++++++++++++++++ .../GateleenNoStackReplyException.java | 30 ++++++++++++++ .../GateleenNoStacktraceException.java | 28 +++++++++++++ .../GateleenThriftyExceptionFactory.java | 28 +++++++++++++ .../GateleenWastefulExceptionFactory.java | 28 +++++++++++++ .../core/lock/impl/RedisBasedLock.java | 31 +++++++++----- .../core/lock/impl/RedisBasedLockTest.java | 4 +- .../swisspush/gateleen/playground/Server.java | 4 +- .../gateleen/runconfig/RunConfig.java | 1 - .../org/swisspush/gateleen/AbstractTest.java | 5 ++- 10 files changed, 185 insertions(+), 14 deletions(-) create mode 100644 gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenExceptionFactory.java create mode 100644 gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenNoStackReplyException.java create mode 100644 gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenNoStacktraceException.java create mode 100644 gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenThriftyExceptionFactory.java create mode 100644 gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenWastefulExceptionFactory.java diff --git a/gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenExceptionFactory.java b/gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenExceptionFactory.java new file mode 100644 index 000000000..200941311 --- /dev/null +++ b/gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenExceptionFactory.java @@ -0,0 +1,40 @@ +package org.swisspush.gateleen.core.exception; + +import io.vertx.core.eventbus.ReplyException; +import io.vertx.core.eventbus.ReplyFailure; + + +/** + * Applies dependency inversion for exception instantiation. + * + * This class did arise because we had different use cases in different + * applications. One of them has the need to perform fine-grained error + * reporting. Whereas in the other application this led to performance issues. + * So now through this abstraction, both applications can choose the behavior + * they need. + * + * If dependency-injection gets applied properly, an app can even provide its + * custom implementation to fine-tune the exact behavior even further. + */ +public interface GateleenExceptionFactory { + + public Exception newException(String message, Throwable cause); + + public ReplyException newReplyException(ReplyFailure failureType, int failureCode, String message); + + + /** + * See {@link GateleenThriftyExceptionFactory}. + */ + public static GateleenExceptionFactory newGateleenThriftyExceptionFactory() { + return new GateleenThriftyExceptionFactory(); + } + + /** + * See {@link GateleenWastefulExceptionFactory}. + */ + public static GateleenExceptionFactory newGateleenWastefulExceptionFactory() { + return new GateleenWastefulExceptionFactory(); + } + +} diff --git a/gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenNoStackReplyException.java b/gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenNoStackReplyException.java new file mode 100644 index 000000000..91a384ff0 --- /dev/null +++ b/gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenNoStackReplyException.java @@ -0,0 +1,30 @@ +package org.swisspush.gateleen.core.exception; + +import io.vertx.core.eventbus.ReplyFailure; + +/** + * There was once a fix in vertx for this (https://github.com/eclipse-vertx/vert.x/issues/4840) + * but for whatever reason in our case we still see stack-trace recordings. Passing + * this subclass to {@link io.vertx.core.eventbus.Message#reply(Object)} seems to + * do the trick. + */ +public class GateleenNoStackReplyException extends io.vertx.core.eventbus.ReplyException { + + public GateleenNoStackReplyException(ReplyFailure failureType, int failureCode, String message) { + super(failureType, failureCode, message); + } + + public GateleenNoStackReplyException(ReplyFailure failureType, String message) { + this(failureType, -1, message); + } + + public GateleenNoStackReplyException(ReplyFailure failureType) { + this(failureType, -1, null); + } + + @Override + public Throwable fillInStackTrace() { + return this; + } + +} diff --git a/gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenNoStacktraceException.java b/gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenNoStacktraceException.java new file mode 100644 index 000000000..36b32c45c --- /dev/null +++ b/gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenNoStacktraceException.java @@ -0,0 +1,28 @@ +package org.swisspush.gateleen.core.exception; + +/** + * Basically same as in vertx, But adding the forgotten contructors. + */ +public class GateleenNoStacktraceException extends RuntimeException { + + public GateleenNoStacktraceException() { + } + + public GateleenNoStacktraceException(String message) { + super(message); + } + + public GateleenNoStacktraceException(String message, Throwable cause) { + super(message, cause); + } + + public GateleenNoStacktraceException(Throwable cause) { + super(cause); + } + + @Override + public Throwable fillInStackTrace() { + return this; + } + +} diff --git a/gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenThriftyExceptionFactory.java b/gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenThriftyExceptionFactory.java new file mode 100644 index 000000000..c6607ca94 --- /dev/null +++ b/gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenThriftyExceptionFactory.java @@ -0,0 +1,28 @@ +package org.swisspush.gateleen.core.exception; + +import io.vertx.core.eventbus.ReplyException; +import io.vertx.core.eventbus.ReplyFailure; + +/** + * Trades maintainability for speed. For example prefers lightweight + * exceptions without stacktrace recording. It may even decide to drop 'cause' + * or other details. If an app needs more error details it should use + * {@link GateleenWastefulExceptionFactory}. If none of those fits the apps needs, it + * can provide its own implementation. + */ +class GateleenThriftyExceptionFactory implements GateleenExceptionFactory { + + GateleenThriftyExceptionFactory() { + } + + public Exception newException(String message, Throwable cause) { + if (cause instanceof Exception) return (Exception) cause; + return new GateleenNoStacktraceException(message, cause); + } + + @Override + public ReplyException newReplyException(ReplyFailure failureType, int failureCode, String message) { + return new GateleenNoStackReplyException(failureType, failureCode, message); + } + +} diff --git a/gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenWastefulExceptionFactory.java b/gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenWastefulExceptionFactory.java new file mode 100644 index 000000000..aa89d844a --- /dev/null +++ b/gateleen-core/src/main/java/org/swisspush/gateleen/core/exception/GateleenWastefulExceptionFactory.java @@ -0,0 +1,28 @@ +package org.swisspush.gateleen.core.exception; + +import io.vertx.core.eventbus.ReplyException; +import io.vertx.core.eventbus.ReplyFailure; + +/** + * Trades speed for maintainability. For example invests more resources like + * recording stack traces (which likely provocates more logs) to get easier + * to debug error messages and better hints of what is happening. It also + * keeps details like 'causes' and 'suppressed' exceptions. If an app needs + * more error details it should use {@link GateleenWastefulExceptionFactory}. If none + * of those fits the apps needs, it can provide its own implementation. + */ +class GateleenWastefulExceptionFactory implements GateleenExceptionFactory { + + GateleenWastefulExceptionFactory() { + } + + public Exception newException(String message, Throwable cause) { + return new Exception(message, cause); + } + + @Override + public ReplyException newReplyException(ReplyFailure failureType, int failureCode, String message) { + return new ReplyException(failureType, failureCode, message); + } + +} diff --git a/gateleen-core/src/main/java/org/swisspush/gateleen/core/lock/impl/RedisBasedLock.java b/gateleen-core/src/main/java/org/swisspush/gateleen/core/lock/impl/RedisBasedLock.java index 841973864..7f359df32 100644 --- a/gateleen-core/src/main/java/org/swisspush/gateleen/core/lock/impl/RedisBasedLock.java +++ b/gateleen-core/src/main/java/org/swisspush/gateleen/core/lock/impl/RedisBasedLock.java @@ -9,6 +9,7 @@ import io.vertx.redis.client.Response; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.swisspush.gateleen.core.exception.GateleenExceptionFactory; import org.swisspush.gateleen.core.lock.Lock; import org.swisspush.gateleen.core.lock.lua.LockLuaScripts; import org.swisspush.gateleen.core.lock.lua.ReleaseLockRedisCommand; @@ -28,14 +29,17 @@ public class RedisBasedLock implements Lock { private static final Logger log = LoggerFactory.getLogger(RedisBasedLock.class); + private static final String[] EMPTY_STRING_ARRAY = new String[0]; public static final String STORAGE_PREFIX = "gateleen.core-lock:"; private final LuaScriptState releaseLockLuaScriptState; private final RedisProvider redisProvider; + private final GateleenExceptionFactory exceptionFactory; - public RedisBasedLock(RedisProvider redisProvider) { + public RedisBasedLock(RedisProvider redisProvider, GateleenExceptionFactory exceptionFactory) { this.redisProvider = redisProvider; + this.exceptionFactory = exceptionFactory; this.releaseLockLuaScriptState = new LuaScriptState(LockLuaScripts.LOCK_RELEASE, redisProvider, false); } @@ -47,22 +51,28 @@ private void redisSetWithOptions(String key, String value, boolean nx, long px, } redisProvider.redis().onComplete( redisEv -> { if( redisEv.failed() ){ - handler.handle(new FailedAsyncResult<>(redisEv.cause())); + Throwable ex = exceptionFactory.newException("redisProvider.redis() failed", redisEv.cause()); + handler.handle(new FailedAsyncResult<>(ex)); return; } var redisAPI = redisEv.result(); - redisAPI.send(Command.SET, RedisUtils.toPayload(key, value, options).toArray(new String[0])) - .onComplete( ev -> { - if( ev.failed() && log.isInfoEnabled() ) log.info("stacktrace", new Exception("stacktrace", ev.cause())); - handler.handle(ev); - }); + String[] payload = RedisUtils.toPayload(key, value, options).toArray(EMPTY_STRING_ARRAY); + redisAPI.send(Command.SET, payload).onComplete(ev -> { + if (ev.failed()) { + Throwable ex = exceptionFactory.newException("redisAPI.send() failed", ev.cause()); + handler.handle(new FailedAsyncResult<>(ex)); + return; + } + handler.handle(ev); + }); }); } @Override public Future acquireLock(String lock, String token, long lockExpiryMs) { Promise promise = Promise.promise(); - redisSetWithOptions(buildLockKey(lock), token, true, lockExpiryMs, event -> { + String lockKey = buildLockKey(lock); + redisSetWithOptions(lockKey, token, true, lockExpiryMs, event -> { if (event.succeeded()) { if (event.result() != null) { promise.complete("OK".equalsIgnoreCase(event.result().toString())); @@ -70,8 +80,9 @@ public Future acquireLock(String lock, String token, long lockExpiryMs) promise.complete(false); } } else { - if( log.isInfoEnabled() ) log.info("stacktrace", new Exception("stacktrace", event.cause())); - promise.fail(event.cause().getMessage()); + Throwable ex = exceptionFactory.newException( + "redisSetWithOptions(lockKey=\"" + lockKey + "\") failed", event.cause()); + promise.fail(ex); } }); return promise.future(); diff --git a/gateleen-core/src/test/java/org/swisspush/gateleen/core/lock/impl/RedisBasedLockTest.java b/gateleen-core/src/test/java/org/swisspush/gateleen/core/lock/impl/RedisBasedLockTest.java index d0f0e2a32..29f0b02ff 100644 --- a/gateleen-core/src/test/java/org/swisspush/gateleen/core/lock/impl/RedisBasedLockTest.java +++ b/gateleen-core/src/test/java/org/swisspush/gateleen/core/lock/impl/RedisBasedLockTest.java @@ -17,6 +17,7 @@ import org.junit.BeforeClass; import org.junit.Test; import org.junit.runner.RunWith; +import org.swisspush.gateleen.core.exception.GateleenExceptionFactory; import redis.clients.jedis.Jedis; import redis.clients.jedis.exceptions.JedisConnectionException; @@ -24,6 +25,7 @@ import static org.awaitility.Awaitility.await; import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static org.swisspush.gateleen.core.exception.GateleenExceptionFactory.newGateleenWastefulExceptionFactory; /** * Tests for the {@link RedisBasedLock} class @@ -48,7 +50,7 @@ public static void setupLock(){ vertx = Vertx.vertx(); RedisAPI redisAPI = RedisAPI.api(new RedisClient(vertx, new NetClientOptions(), new PoolOptions(), new RedisStandaloneConnectOptions(), TracingPolicy.IGNORE)); - redisBasedLock = new RedisBasedLock(() -> Future.succeededFuture(redisAPI)); + redisBasedLock = new RedisBasedLock(() -> Future.succeededFuture(redisAPI), newGateleenWastefulExceptionFactory()); } @Before diff --git a/gateleen-playground/src/main/java/org/swisspush/gateleen/playground/Server.java b/gateleen-playground/src/main/java/org/swisspush/gateleen/playground/Server.java index 48bbeb033..212dd8661 100755 --- a/gateleen-playground/src/main/java/org/swisspush/gateleen/playground/Server.java +++ b/gateleen-playground/src/main/java/org/swisspush/gateleen/playground/Server.java @@ -86,6 +86,8 @@ import java.util.Arrays; import java.util.Map; +import static org.swisspush.gateleen.core.exception.GateleenExceptionFactory.newGateleenWastefulExceptionFactory; + /** * Playground server to try Gateleen at home. * @@ -214,7 +216,7 @@ public void start() { copyResourceHandler = new CopyResourceHandler(selfClient, SERVER_ROOT + "/v1/copy"); monitoringHandler = new MonitoringHandler(vertx, storage, PREFIX, SERVER_ROOT + "/monitoring/rpr"); - Lock lock = new RedisBasedLock(redisProvider); + Lock lock = new RedisBasedLock(redisProvider, newGateleenWastefulExceptionFactory()); cacheStorage = new RedisCacheStorage(vertx, lock, redisProvider, 20 * 1000); cacheDataFetcher = new DefaultCacheDataFetcher(new ClientRequestCreator(selfClient)); diff --git a/gateleen-runconfig/src/main/java/org/swisspush/gateleen/runconfig/RunConfig.java b/gateleen-runconfig/src/main/java/org/swisspush/gateleen/runconfig/RunConfig.java index 824add3af..2d9043015 100755 --- a/gateleen-runconfig/src/main/java/org/swisspush/gateleen/runconfig/RunConfig.java +++ b/gateleen-runconfig/src/main/java/org/swisspush/gateleen/runconfig/RunConfig.java @@ -38,7 +38,6 @@ import org.swisspush.gateleen.queue.queuing.QueueBrowser; import org.swisspush.gateleen.queue.queuing.QueuingHandler; import org.swisspush.gateleen.queue.queuing.circuitbreaker.configuration.QueueCircuitBreakerConfigurationResourceManager; -import org.swisspush.gateleen.queue.queuing.splitter.NoOpQueueSplitter; import org.swisspush.gateleen.queue.queuing.splitter.QueueSplitter; import org.swisspush.gateleen.routing.CustomHttpResponseHandler; import org.swisspush.gateleen.routing.Router; diff --git a/gateleen-test/src/test/java/org/swisspush/gateleen/AbstractTest.java b/gateleen-test/src/test/java/org/swisspush/gateleen/AbstractTest.java index c06a011f4..c99436661 100755 --- a/gateleen-test/src/test/java/org/swisspush/gateleen/AbstractTest.java +++ b/gateleen-test/src/test/java/org/swisspush/gateleen/AbstractTest.java @@ -73,6 +73,7 @@ import org.swisspush.gateleen.scheduler.SchedulerResourceManager; import org.swisspush.gateleen.user.RoleProfileHandler; import org.swisspush.gateleen.user.UserProfileHandler; +import org.swisspush.reststorage.exception.RestStorageExceptionFactory; import redis.clients.jedis.Jedis; import javax.management.*; @@ -82,6 +83,8 @@ import java.util.Map; import java.util.Set; +import static org.swisspush.reststorage.exception.RestStorageExceptionFactory.newRestStorageWastefulExceptionFactory; + /** * TestVerticle all Gateleen tests.
* @@ -170,7 +173,7 @@ public static void setupBeforeClass(TestContext context) { RoleProfileHandler roleProfileHandler = new RoleProfileHandler(vertx, storage, SERVER_ROOT + "/roles/v1/([^/]+)/profile"); qosHandler = new QoSHandler(vertx, storage, SERVER_ROOT + "/admin/v1/qos", props, PREFIX); - Lock lock = new RedisBasedLock(redisProvider); + Lock lock = new RedisBasedLock(redisProvider, newRestStorageWastefulExceptionFactory()); QueueClient queueClient = new QueueClient(vertx, monitoringHandler); ReducedPropagationManager reducedPropagationManager = new ReducedPropagationManager(vertx,