From 8dca231bd09795ace9d2faecd4f57a9d3e9a15f3 Mon Sep 17 00:00:00 2001 From: Andreas Fankhauser <23085769+hiddenalpha@users.noreply.github.com> Date: Tue, 21 May 2024 12:05:09 +0200 Subject: [PATCH 1/8] [SDCISA-15833, https://github.com/swisspost/vertx-redisques/issues/170] Reduce IO (logging) spam by making stacks configurable. --- .../swisspush/reststorage/RestStorageMod.java | 17 ++++++-- .../exception/ExceptionFactory.java | 39 +++++++++++++++++++ .../exception/NoStacktraceException.java | 28 +++++++++++++ .../exception/ThriftyExceptionFactory.java | 28 +++++++++++++ .../exception/WastefulExceptionFactory.java | 29 ++++++++++++++ .../reststorage/lock/impl/RedisBasedLock.java | 10 ++++- .../reststorage/redis/RedisStorage.java | 10 ++++- 7 files changed, 154 insertions(+), 7 deletions(-) create mode 100644 src/main/java/org/swisspush/reststorage/exception/ExceptionFactory.java create mode 100644 src/main/java/org/swisspush/reststorage/exception/NoStacktraceException.java create mode 100644 src/main/java/org/swisspush/reststorage/exception/ThriftyExceptionFactory.java create mode 100644 src/main/java/org/swisspush/reststorage/exception/WastefulExceptionFactory.java diff --git a/src/main/java/org/swisspush/reststorage/RestStorageMod.java b/src/main/java/org/swisspush/reststorage/RestStorageMod.java index 8ebc136..6930c6f 100644 --- a/src/main/java/org/swisspush/reststorage/RestStorageMod.java +++ b/src/main/java/org/swisspush/reststorage/RestStorageMod.java @@ -5,20 +5,31 @@ import io.vertx.core.http.HttpServerRequest; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.swisspush.reststorage.exception.ExceptionFactory; import org.swisspush.reststorage.redis.RedisProvider; import org.swisspush.reststorage.redis.RedisStorage; import org.swisspush.reststorage.util.ModuleConfiguration; +import static org.swisspush.reststorage.exception.ExceptionFactory.newThriftyExceptionFactory; + public class RestStorageMod extends AbstractVerticle { private final Logger log = LoggerFactory.getLogger(RestStorageMod.class); private RedisProvider redisProvider; + private final ExceptionFactory exceptionFactory; - public RestStorageMod() {} + public RestStorageMod() { + this.exceptionFactory = newThriftyExceptionFactory(); + } - public RestStorageMod(RedisProvider redisProvider) { + public RestStorageMod( + RedisProvider redisProvider, + ExceptionFactory exceptionFactory + ) { + assert exceptionFactory != null; this.redisProvider = redisProvider; + this.exceptionFactory = exceptionFactory; } @Override @@ -84,7 +95,7 @@ private Future createRedisStorage(Vertx vertx, ModuleConfiguration redisProvider.redis().onComplete(event -> { if(event.succeeded()) { - initPromise.complete(new RedisStorage(vertx, moduleConfiguration, redisProvider)); + initPromise.complete(new RedisStorage(vertx, moduleConfiguration, redisProvider, exceptionFactory)); } else { initPromise.fail(new Exception(event.cause())); } diff --git a/src/main/java/org/swisspush/reststorage/exception/ExceptionFactory.java b/src/main/java/org/swisspush/reststorage/exception/ExceptionFactory.java new file mode 100644 index 0000000..3b55174 --- /dev/null +++ b/src/main/java/org/swisspush/reststorage/exception/ExceptionFactory.java @@ -0,0 +1,39 @@ +package org.swisspush.reststorage.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 ExceptionFactory { + + public Exception newException(String message, Throwable cause); + + public ReplyException newReplyException(ReplyFailure failureType, int failureCode, String message); + + + /** + * See {@link ThriftyExceptionFactory}. + */ + public static ExceptionFactory newThriftyExceptionFactory() { + return new ThriftyExceptionFactory(); + } + + /** + * See {@link WastefulExceptionFactory}. + */ + public static ExceptionFactory newWastefulExceptionFactory() { + return new WastefulExceptionFactory(); + } + +} diff --git a/src/main/java/org/swisspush/reststorage/exception/NoStacktraceException.java b/src/main/java/org/swisspush/reststorage/exception/NoStacktraceException.java new file mode 100644 index 0000000..bfa9955 --- /dev/null +++ b/src/main/java/org/swisspush/reststorage/exception/NoStacktraceException.java @@ -0,0 +1,28 @@ +package org.swisspush.reststorage.exception; + +/** + * Basically same as in vertx, But adding the forgotten contructors. + */ +public class NoStacktraceException extends RuntimeException { + + public NoStacktraceException() { + } + + public NoStacktraceException(String message) { + super(message); + } + + public NoStacktraceException(String message, Throwable cause) { + super(message, cause); + } + + public NoStacktraceException(Throwable cause) { + super(cause); + } + + @Override + public Throwable fillInStackTrace() { + return this; + } + +} diff --git a/src/main/java/org/swisspush/reststorage/exception/ThriftyExceptionFactory.java b/src/main/java/org/swisspush/reststorage/exception/ThriftyExceptionFactory.java new file mode 100644 index 0000000..2e61582 --- /dev/null +++ b/src/main/java/org/swisspush/reststorage/exception/ThriftyExceptionFactory.java @@ -0,0 +1,28 @@ +package org.swisspush.reststorage.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' + * and 'suppressed' exceptions. If an app needs more error details it should use + * {@link WastefulExceptionFactory}. If none of those fits the apps needs, it + * can provide its own implementation. + */ +class ThriftyExceptionFactory implements ExceptionFactory { + + ThriftyExceptionFactory() { + } + + @Override + public Exception newException(String message, Throwable cause) { + return new NoStacktraceException(message, cause); + } + + @Override + public ReplyException newReplyException(ReplyFailure failureType, int failureCode, String message) { + return new NoStackReplyException(failureType, failureCode, message); + } + +} diff --git a/src/main/java/org/swisspush/reststorage/exception/WastefulExceptionFactory.java b/src/main/java/org/swisspush/reststorage/exception/WastefulExceptionFactory.java new file mode 100644 index 0000000..0dd3de3 --- /dev/null +++ b/src/main/java/org/swisspush/reststorage/exception/WastefulExceptionFactory.java @@ -0,0 +1,29 @@ +package org.swisspush.reststorage.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 WastefulExceptionFactory}. If none + * of those fits the apps needs, it can provide its own implementation. + */ +class WastefulExceptionFactory implements ExceptionFactory { + + WastefulExceptionFactory() { + } + + @Override + 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/src/main/java/org/swisspush/reststorage/lock/impl/RedisBasedLock.java b/src/main/java/org/swisspush/reststorage/lock/impl/RedisBasedLock.java index 1f721e5..02a00b3 100644 --- a/src/main/java/org/swisspush/reststorage/lock/impl/RedisBasedLock.java +++ b/src/main/java/org/swisspush/reststorage/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.reststorage.exception.ExceptionFactory; import org.swisspush.reststorage.redis.RedisProvider; import org.swisspush.reststorage.lock.Lock; import org.swisspush.reststorage.lock.lua.LockLuaScripts; @@ -34,9 +35,14 @@ public class RedisBasedLock implements Lock { private final LuaScriptState releaseLockLuaScriptState; private final RedisProvider redisProvider; + private final ExceptionFactory exceptionFactory; - public RedisBasedLock(RedisProvider redisProvider) { + public RedisBasedLock( + RedisProvider redisProvider, + ExceptionFactory exceptionFactory + ) { this.redisProvider = redisProvider; + this.exceptionFactory = exceptionFactory; this.releaseLockLuaScriptState = new LuaScriptState(LockLuaScripts.LOCK_RELEASE, redisProvider, false); } @@ -74,7 +80,7 @@ public Future acquireLock(String lock, String token, long lockExpiryMs) promise.complete(false); } } else { - promise.fail(new Exception("stacktrace", event.cause())); + promise.fail(exceptionFactory.newException("redisSetWithOptions() failed", event.cause())); } }); return promise.future(); diff --git a/src/main/java/org/swisspush/reststorage/redis/RedisStorage.java b/src/main/java/org/swisspush/reststorage/redis/RedisStorage.java index e623fc4..a5aa41c 100644 --- a/src/main/java/org/swisspush/reststorage/redis/RedisStorage.java +++ b/src/main/java/org/swisspush/reststorage/redis/RedisStorage.java @@ -23,6 +23,7 @@ import org.swisspush.reststorage.DocumentResource; import org.swisspush.reststorage.Resource; import org.swisspush.reststorage.Storage; +import org.swisspush.reststorage.exception.ExceptionFactory; import org.swisspush.reststorage.lock.Lock; import org.swisspush.reststorage.lock.impl.RedisBasedLock; import org.swisspush.reststorage.util.GZIPUtil; @@ -89,7 +90,12 @@ public class RedisStorage implements Storage { private final String ID; private final String hostAndPort; - public RedisStorage(Vertx vertx, ModuleConfiguration config, RedisProvider redisProvider) { + public RedisStorage( + Vertx vertx, + ModuleConfiguration config, + RedisProvider redisProvider, + ExceptionFactory exceptionFactory + ) { this.expirableSet = config.getExpirablePrefix(); this.redisResourcesPrefix = config.getResourcesPrefix(); this.redisCollectionsPrefix = config.getCollectionsPrefix(); @@ -106,7 +112,7 @@ public RedisStorage(Vertx vertx, ModuleConfiguration config, RedisProvider redis this.ID = UUID.randomUUID().toString(); this.hostAndPort = config.getRedisHost() + ":" + config.getPort(); - this.lock = new RedisBasedLock(redisProvider); + this.lock = new RedisBasedLock(redisProvider, exceptionFactory); // load all the lua scripts LuaScriptState luaGetScriptState = new LuaScriptState(LuaScript.GET, false); From e3ed8202461239a0f838aff7ed06e24591c01d8b Mon Sep 17 00:00:00 2001 From: Andreas Fankhauser <23085769+hiddenalpha@users.noreply.github.com> Date: Tue, 21 May 2024 12:20:09 +0200 Subject: [PATCH 2/8] [SDCISA-15833, https://github.com/swisspost/vertx-redisques/issues/170] Add missing class. --- .../exception/NoStackReplyException.java | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 src/main/java/org/swisspush/reststorage/exception/NoStackReplyException.java diff --git a/src/main/java/org/swisspush/reststorage/exception/NoStackReplyException.java b/src/main/java/org/swisspush/reststorage/exception/NoStackReplyException.java new file mode 100644 index 0000000..4eb9cd3 --- /dev/null +++ b/src/main/java/org/swisspush/reststorage/exception/NoStackReplyException.java @@ -0,0 +1,30 @@ +package org.swisspush.reststorage.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 NoStackReplyException extends io.vertx.core.eventbus.ReplyException { + + public NoStackReplyException(ReplyFailure failureType, int failureCode, String message) { + super(failureType, failureCode, message); + } + + public NoStackReplyException(ReplyFailure failureType, String message) { + this(failureType, -1, message); + } + + public NoStackReplyException(ReplyFailure failureType) { + this(failureType, -1, null); + } + + @Override + public Throwable fillInStackTrace() { + return this; + } + +} From a9542b1ce9e8e037b9324794ea16b2be31cdc919 Mon Sep 17 00:00:00 2001 From: Andreas Fankhauser <23085769+hiddenalpha@users.noreply.github.com> Date: Tue, 21 May 2024 12:53:19 +0200 Subject: [PATCH 3/8] [SDCISA-15833, https://github.com/swisspost/vertx-redisques/issues/170] Reduce risk of name collisions. --- .../swisspush/reststorage/RestStorageMod.java | 10 +++---- .../exception/NoStacktraceException.java | 28 ------------------- ....java => RestStorageExceptionFactory.java} | 14 +++++----- ... => RestStorageNoStackReplyException.java} | 8 +++--- .../RestStorageNoStacktraceException.java | 28 +++++++++++++++++++ ...> RestStorageThriftyExceptionFactory.java} | 10 +++---- ... RestStorageWastefulExceptionFactory.java} | 6 ++-- .../reststorage/lock/impl/RedisBasedLock.java | 6 ++-- .../reststorage/redis/RedisStorage.java | 7 ++--- 9 files changed, 57 insertions(+), 60 deletions(-) delete mode 100644 src/main/java/org/swisspush/reststorage/exception/NoStacktraceException.java rename src/main/java/org/swisspush/reststorage/exception/{ExceptionFactory.java => RestStorageExceptionFactory.java} (66%) rename src/main/java/org/swisspush/reststorage/exception/{NoStackReplyException.java => RestStorageNoStackReplyException.java} (63%) create mode 100644 src/main/java/org/swisspush/reststorage/exception/RestStorageNoStacktraceException.java rename src/main/java/org/swisspush/reststorage/exception/{ThriftyExceptionFactory.java => RestStorageThriftyExceptionFactory.java} (63%) rename src/main/java/org/swisspush/reststorage/exception/{WastefulExceptionFactory.java => RestStorageWastefulExceptionFactory.java} (79%) diff --git a/src/main/java/org/swisspush/reststorage/RestStorageMod.java b/src/main/java/org/swisspush/reststorage/RestStorageMod.java index 6930c6f..04887f1 100644 --- a/src/main/java/org/swisspush/reststorage/RestStorageMod.java +++ b/src/main/java/org/swisspush/reststorage/RestStorageMod.java @@ -5,27 +5,27 @@ import io.vertx.core.http.HttpServerRequest; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.swisspush.reststorage.exception.ExceptionFactory; +import org.swisspush.reststorage.exception.RestStorageExceptionFactory; import org.swisspush.reststorage.redis.RedisProvider; import org.swisspush.reststorage.redis.RedisStorage; import org.swisspush.reststorage.util.ModuleConfiguration; -import static org.swisspush.reststorage.exception.ExceptionFactory.newThriftyExceptionFactory; +import static org.swisspush.reststorage.exception.RestStorageExceptionFactory.newRestStorageThriftyExceptionFactory; public class RestStorageMod extends AbstractVerticle { private final Logger log = LoggerFactory.getLogger(RestStorageMod.class); private RedisProvider redisProvider; - private final ExceptionFactory exceptionFactory; + private final RestStorageExceptionFactory exceptionFactory; public RestStorageMod() { - this.exceptionFactory = newThriftyExceptionFactory(); + this.exceptionFactory = newRestStorageThriftyExceptionFactory(); } public RestStorageMod( RedisProvider redisProvider, - ExceptionFactory exceptionFactory + RestStorageExceptionFactory exceptionFactory ) { assert exceptionFactory != null; this.redisProvider = redisProvider; diff --git a/src/main/java/org/swisspush/reststorage/exception/NoStacktraceException.java b/src/main/java/org/swisspush/reststorage/exception/NoStacktraceException.java deleted file mode 100644 index bfa9955..0000000 --- a/src/main/java/org/swisspush/reststorage/exception/NoStacktraceException.java +++ /dev/null @@ -1,28 +0,0 @@ -package org.swisspush.reststorage.exception; - -/** - * Basically same as in vertx, But adding the forgotten contructors. - */ -public class NoStacktraceException extends RuntimeException { - - public NoStacktraceException() { - } - - public NoStacktraceException(String message) { - super(message); - } - - public NoStacktraceException(String message, Throwable cause) { - super(message, cause); - } - - public NoStacktraceException(Throwable cause) { - super(cause); - } - - @Override - public Throwable fillInStackTrace() { - return this; - } - -} diff --git a/src/main/java/org/swisspush/reststorage/exception/ExceptionFactory.java b/src/main/java/org/swisspush/reststorage/exception/RestStorageExceptionFactory.java similarity index 66% rename from src/main/java/org/swisspush/reststorage/exception/ExceptionFactory.java rename to src/main/java/org/swisspush/reststorage/exception/RestStorageExceptionFactory.java index 3b55174..7cff9cc 100644 --- a/src/main/java/org/swisspush/reststorage/exception/ExceptionFactory.java +++ b/src/main/java/org/swisspush/reststorage/exception/RestStorageExceptionFactory.java @@ -15,7 +15,7 @@ * If dependency-injection gets applied properly, an app can even provide its * custom implementation to fine-tune the exact behavior even further. */ -public interface ExceptionFactory { +public interface RestStorageExceptionFactory { public Exception newException(String message, Throwable cause); @@ -23,17 +23,17 @@ public interface ExceptionFactory { /** - * See {@link ThriftyExceptionFactory}. + * See {@link RestStorageThriftyExceptionFactory}. */ - public static ExceptionFactory newThriftyExceptionFactory() { - return new ThriftyExceptionFactory(); + public static RestStorageExceptionFactory newRestStorageThriftyExceptionFactory() { + return new RestStorageThriftyExceptionFactory(); } /** - * See {@link WastefulExceptionFactory}. + * See {@link RestStorageWastefulExceptionFactory}. */ - public static ExceptionFactory newWastefulExceptionFactory() { - return new WastefulExceptionFactory(); + public static RestStorageExceptionFactory newRestStorageWastefulExceptionFactory() { + return new RestStorageWastefulExceptionFactory(); } } diff --git a/src/main/java/org/swisspush/reststorage/exception/NoStackReplyException.java b/src/main/java/org/swisspush/reststorage/exception/RestStorageNoStackReplyException.java similarity index 63% rename from src/main/java/org/swisspush/reststorage/exception/NoStackReplyException.java rename to src/main/java/org/swisspush/reststorage/exception/RestStorageNoStackReplyException.java index 4eb9cd3..15b6c79 100644 --- a/src/main/java/org/swisspush/reststorage/exception/NoStackReplyException.java +++ b/src/main/java/org/swisspush/reststorage/exception/RestStorageNoStackReplyException.java @@ -8,17 +8,17 @@ * this subclass to {@link io.vertx.core.eventbus.Message#reply(Object)} seems to * do the trick. */ -public class NoStackReplyException extends io.vertx.core.eventbus.ReplyException { +public class RestStorageNoStackReplyException extends io.vertx.core.eventbus.ReplyException { - public NoStackReplyException(ReplyFailure failureType, int failureCode, String message) { + public RestStorageNoStackReplyException(ReplyFailure failureType, int failureCode, String message) { super(failureType, failureCode, message); } - public NoStackReplyException(ReplyFailure failureType, String message) { + public RestStorageNoStackReplyException(ReplyFailure failureType, String message) { this(failureType, -1, message); } - public NoStackReplyException(ReplyFailure failureType) { + public RestStorageNoStackReplyException(ReplyFailure failureType) { this(failureType, -1, null); } diff --git a/src/main/java/org/swisspush/reststorage/exception/RestStorageNoStacktraceException.java b/src/main/java/org/swisspush/reststorage/exception/RestStorageNoStacktraceException.java new file mode 100644 index 0000000..468aa15 --- /dev/null +++ b/src/main/java/org/swisspush/reststorage/exception/RestStorageNoStacktraceException.java @@ -0,0 +1,28 @@ +package org.swisspush.reststorage.exception; + +/** + * Basically same as in vertx, But adding the forgotten contructors. + */ +public class RestStorageNoStacktraceException extends RuntimeException { + + public RestStorageNoStacktraceException() { + } + + public RestStorageNoStacktraceException(String message) { + super(message); + } + + public RestStorageNoStacktraceException(String message, Throwable cause) { + super(message, cause); + } + + public RestStorageNoStacktraceException(Throwable cause) { + super(cause); + } + + @Override + public Throwable fillInStackTrace() { + return this; + } + +} diff --git a/src/main/java/org/swisspush/reststorage/exception/ThriftyExceptionFactory.java b/src/main/java/org/swisspush/reststorage/exception/RestStorageThriftyExceptionFactory.java similarity index 63% rename from src/main/java/org/swisspush/reststorage/exception/ThriftyExceptionFactory.java rename to src/main/java/org/swisspush/reststorage/exception/RestStorageThriftyExceptionFactory.java index 2e61582..6e955b6 100644 --- a/src/main/java/org/swisspush/reststorage/exception/ThriftyExceptionFactory.java +++ b/src/main/java/org/swisspush/reststorage/exception/RestStorageThriftyExceptionFactory.java @@ -7,22 +7,22 @@ * Trades maintainability for speed. For example prefers lightweight * exceptions without stacktrace recording. It may even decide to drop 'cause' * and 'suppressed' exceptions. If an app needs more error details it should use - * {@link WastefulExceptionFactory}. If none of those fits the apps needs, it + * {@link RestStorageWastefulExceptionFactory}. If none of those fits the apps needs, it * can provide its own implementation. */ -class ThriftyExceptionFactory implements ExceptionFactory { +class RestStorageThriftyExceptionFactory implements RestStorageExceptionFactory { - ThriftyExceptionFactory() { + RestStorageThriftyExceptionFactory() { } @Override public Exception newException(String message, Throwable cause) { - return new NoStacktraceException(message, cause); + return new RestStorageNoStacktraceException(message, cause); } @Override public ReplyException newReplyException(ReplyFailure failureType, int failureCode, String message) { - return new NoStackReplyException(failureType, failureCode, message); + return new RestStorageNoStackReplyException(failureType, failureCode, message); } } diff --git a/src/main/java/org/swisspush/reststorage/exception/WastefulExceptionFactory.java b/src/main/java/org/swisspush/reststorage/exception/RestStorageWastefulExceptionFactory.java similarity index 79% rename from src/main/java/org/swisspush/reststorage/exception/WastefulExceptionFactory.java rename to src/main/java/org/swisspush/reststorage/exception/RestStorageWastefulExceptionFactory.java index 0dd3de3..c31a3df 100644 --- a/src/main/java/org/swisspush/reststorage/exception/WastefulExceptionFactory.java +++ b/src/main/java/org/swisspush/reststorage/exception/RestStorageWastefulExceptionFactory.java @@ -8,12 +8,12 @@ * 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 WastefulExceptionFactory}. If none + * more error details it should use {@link RestStorageWastefulExceptionFactory}. If none * of those fits the apps needs, it can provide its own implementation. */ -class WastefulExceptionFactory implements ExceptionFactory { +class RestStorageWastefulExceptionFactory implements RestStorageExceptionFactory { - WastefulExceptionFactory() { + RestStorageWastefulExceptionFactory() { } @Override diff --git a/src/main/java/org/swisspush/reststorage/lock/impl/RedisBasedLock.java b/src/main/java/org/swisspush/reststorage/lock/impl/RedisBasedLock.java index 02a00b3..e30bc52 100644 --- a/src/main/java/org/swisspush/reststorage/lock/impl/RedisBasedLock.java +++ b/src/main/java/org/swisspush/reststorage/lock/impl/RedisBasedLock.java @@ -9,7 +9,7 @@ import io.vertx.redis.client.Response; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.swisspush.reststorage.exception.ExceptionFactory; +import org.swisspush.reststorage.exception.RestStorageExceptionFactory; import org.swisspush.reststorage.redis.RedisProvider; import org.swisspush.reststorage.lock.Lock; import org.swisspush.reststorage.lock.lua.LockLuaScripts; @@ -35,11 +35,11 @@ public class RedisBasedLock implements Lock { private final LuaScriptState releaseLockLuaScriptState; private final RedisProvider redisProvider; - private final ExceptionFactory exceptionFactory; + private final RestStorageExceptionFactory exceptionFactory; public RedisBasedLock( RedisProvider redisProvider, - ExceptionFactory exceptionFactory + RestStorageExceptionFactory exceptionFactory ) { this.redisProvider = redisProvider; this.exceptionFactory = exceptionFactory; diff --git a/src/main/java/org/swisspush/reststorage/redis/RedisStorage.java b/src/main/java/org/swisspush/reststorage/redis/RedisStorage.java index a5aa41c..d82c140 100644 --- a/src/main/java/org/swisspush/reststorage/redis/RedisStorage.java +++ b/src/main/java/org/swisspush/reststorage/redis/RedisStorage.java @@ -23,7 +23,7 @@ import org.swisspush.reststorage.DocumentResource; import org.swisspush.reststorage.Resource; import org.swisspush.reststorage.Storage; -import org.swisspush.reststorage.exception.ExceptionFactory; +import org.swisspush.reststorage.exception.RestStorageExceptionFactory; import org.swisspush.reststorage.lock.Lock; import org.swisspush.reststorage.lock.impl.RedisBasedLock; import org.swisspush.reststorage.util.GZIPUtil; @@ -40,17 +40,14 @@ import java.text.DecimalFormat; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.UUID; -import java.util.stream.Stream; import static org.swisspush.reststorage.redis.RedisUtils.toPayload; @@ -94,7 +91,7 @@ public RedisStorage( Vertx vertx, ModuleConfiguration config, RedisProvider redisProvider, - ExceptionFactory exceptionFactory + RestStorageExceptionFactory exceptionFactory ) { this.expirableSet = config.getExpirablePrefix(); this.redisResourcesPrefix = config.getResourcesPrefix(); From dc5352a2d4cbfe4b7e5039c9340805de1d2188ac Mon Sep 17 00:00:00 2001 From: Andreas Fankhauser <23085769+hiddenalpha@users.noreply.github.com> Date: Tue, 21 May 2024 12:58:07 +0200 Subject: [PATCH 4/8] [SDCISA-15833, https://github.com/swisspost/vertx-redisques/issues/170] Fix typo. --- .../exception/RestStorageWastefulExceptionFactory.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/swisspush/reststorage/exception/RestStorageWastefulExceptionFactory.java b/src/main/java/org/swisspush/reststorage/exception/RestStorageWastefulExceptionFactory.java index c31a3df..7171124 100644 --- a/src/main/java/org/swisspush/reststorage/exception/RestStorageWastefulExceptionFactory.java +++ b/src/main/java/org/swisspush/reststorage/exception/RestStorageWastefulExceptionFactory.java @@ -8,8 +8,9 @@ * 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 RestStorageWastefulExceptionFactory}. If none - * of those fits the apps needs, it can provide its own implementation. + * more error details it should use {@link RestStorageThriftyExceptionFactory} + * instead. If none of those impls fit the apps needs, it can provide its own + * implementation. */ class RestStorageWastefulExceptionFactory implements RestStorageExceptionFactory { From 36e08c97d0eb6e7e265d68b4610ebb78a1509b19 Mon Sep 17 00:00:00 2001 From: Andreas Fankhauser <23085769+hiddenalpha@users.noreply.github.com> Date: Tue, 21 May 2024 14:10:19 +0200 Subject: [PATCH 5/8] [SDCISA-15833, https://github.com/swisspost/vertx-redisques/issues/170] Fix tests. --- .../swisspush/reststorage/lock/impl/RedisBasedLockTest.java | 4 +++- .../org/swisspush/reststorage/redis/RedisStorageTest.java | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/swisspush/reststorage/lock/impl/RedisBasedLockTest.java b/src/test/java/org/swisspush/reststorage/lock/impl/RedisBasedLockTest.java index 44c2b63..85f6f8b 100644 --- a/src/test/java/org/swisspush/reststorage/lock/impl/RedisBasedLockTest.java +++ b/src/test/java/org/swisspush/reststorage/lock/impl/RedisBasedLockTest.java @@ -15,6 +15,7 @@ import org.junit.BeforeClass; import org.junit.Test; import org.junit.runner.RunWith; +import org.swisspush.reststorage.exception.RestStorageExceptionFactory; import redis.clients.jedis.Jedis; import redis.clients.jedis.exceptions.JedisConnectionException; @@ -22,6 +23,7 @@ import static java.util.concurrent.TimeUnit.MILLISECONDS; import static org.awaitility.Awaitility.await; +import static org.swisspush.reststorage.exception.RestStorageExceptionFactory.newRestStorageWastefulExceptionFactory; /** * Tests for the {@link RedisBasedLock} class @@ -45,7 +47,7 @@ public class RedisBasedLockTest { 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), newRestStorageWastefulExceptionFactory()); } @Before diff --git a/src/test/java/org/swisspush/reststorage/redis/RedisStorageTest.java b/src/test/java/org/swisspush/reststorage/redis/RedisStorageTest.java index 152e429..e703cee 100644 --- a/src/test/java/org/swisspush/reststorage/redis/RedisStorageTest.java +++ b/src/test/java/org/swisspush/reststorage/redis/RedisStorageTest.java @@ -27,6 +27,7 @@ import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import static org.swisspush.reststorage.exception.RestStorageExceptionFactory.newRestStorageWastefulExceptionFactory; /** * Tests for the {@link RedisStorage} class @@ -45,8 +46,9 @@ public void setUp(TestContext context) { redisAPI = Mockito.mock(RedisAPI.class); redisProvider = Mockito.mock(RedisProvider.class); when(redisProvider.redis()).thenReturn(Future.succeededFuture(redisAPI)); + var exceptionFactory = newRestStorageWastefulExceptionFactory(); - storage = new RedisStorage(mock(Vertx.class), new ModuleConfiguration(), redisProvider); + storage = new RedisStorage(mock(Vertx.class), new ModuleConfiguration(), redisProvider, exceptionFactory); } From d825c5889c075eec84161e72ed6df3cd2a523158 Mon Sep 17 00:00:00 2001 From: Andreas Fankhauser <23085769+hiddenalpha@users.noreply.github.com> Date: Tue, 21 May 2024 17:19:57 +0200 Subject: [PATCH 6/8] Skip instance creation if possible. Related: SDCISA-15833, https://github.com/swisspost/vertx-redisques/issues/170, https://github.com/swisspost/vertx-redisques/pull/177, https://github.com/swisspost/vertx-rest-storage/pull/186, https://github.com/swisspost/gateleen/pull/577 --- .../exception/RestStorageThriftyExceptionFactory.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/swisspush/reststorage/exception/RestStorageThriftyExceptionFactory.java b/src/main/java/org/swisspush/reststorage/exception/RestStorageThriftyExceptionFactory.java index 6e955b6..712ad64 100644 --- a/src/main/java/org/swisspush/reststorage/exception/RestStorageThriftyExceptionFactory.java +++ b/src/main/java/org/swisspush/reststorage/exception/RestStorageThriftyExceptionFactory.java @@ -6,9 +6,10 @@ /** * Trades maintainability for speed. For example prefers lightweight * exceptions without stacktrace recording. It may even decide to drop 'cause' - * and 'suppressed' exceptions. If an app needs more error details it should use - * {@link RestStorageWastefulExceptionFactory}. If none of those fits the apps needs, it - * can provide its own implementation. + * and 'suppressed' exceptions. Or to make other optimizations towards + * performance. If an app needs more error details it should use + * {@link RestStorageWastefulExceptionFactory}. If none of those fits the apps + * needs, it can provide its own implementation. */ class RestStorageThriftyExceptionFactory implements RestStorageExceptionFactory { @@ -17,6 +18,7 @@ class RestStorageThriftyExceptionFactory implements RestStorageExceptionFactory @Override public Exception newException(String message, Throwable cause) { + if (cause instanceof Exception) return (Exception) cause; return new RestStorageNoStacktraceException(message, cause); } From 701dce2da77c42bef040dfca17e8e755b92b9442 Mon Sep 17 00:00:00 2001 From: Andreas Fankhauser <23085769+hiddenalpha@users.noreply.github.com> Date: Thu, 23 May 2024 14:44:55 +0200 Subject: [PATCH 7/8] Fix parts for 'https://github.com/swisspost/vertx-rest-storage/pull/186#pullrequestreview-2070250247' Related: SDCISA-15833, https://github.com/swisspost/vertx-redisques/issues/170, https://github.com/swisspost/vertx-redisques/pull/177, https://github.com/swisspost/vertx-rest-storage/pull/186, https://github.com/swisspost/gateleen/pull/577 --- .../reststorage/DefaultRedisProvider.java | 15 ++-- .../reststorage/EventBusAdapter.java | 28 ++++++-- .../reststorage/FileSystemStorage.java | 15 ++-- .../reststorage/RestStorageHandler.java | 28 ++++++-- .../swisspush/reststorage/RestStorageMod.java | 20 +++--- .../RestStorageExceptionFactory.java | 14 ++++ .../RestStorageThriftyExceptionFactory.java | 12 ++++ .../RestStorageWastefulExceptionFactory.java | 5 ++ .../reststorage/lock/impl/RedisBasedLock.java | 11 +-- .../reststorage/lock/lua/LuaScriptState.java | 25 ++++--- .../lock/lua/ReleaseLockRedisCommand.java | 18 +++-- .../reststorage/redis/RedisStorage.java | 72 ++++++++++++------- .../reststorage/FilesystemStorageTest.java | 9 +-- .../reststorage/RestStorageHandlerTest.java | 25 +++---- 14 files changed, 208 insertions(+), 89 deletions(-) diff --git a/src/main/java/org/swisspush/reststorage/DefaultRedisProvider.java b/src/main/java/org/swisspush/reststorage/DefaultRedisProvider.java index c384593..f8ca4d6 100644 --- a/src/main/java/org/swisspush/reststorage/DefaultRedisProvider.java +++ b/src/main/java/org/swisspush/reststorage/DefaultRedisProvider.java @@ -10,6 +10,7 @@ import io.vertx.redis.client.RedisOptions; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.swisspush.reststorage.exception.RestStorageExceptionFactory; import org.swisspush.reststorage.redis.RedisProvider; import org.swisspush.reststorage.util.ModuleConfiguration; @@ -29,6 +30,7 @@ public class DefaultRedisProvider implements RedisProvider { private final Vertx vertx; private final ModuleConfiguration configuration; + private final RestStorageExceptionFactory exceptionFactory; private RedisAPI redisAPI; private Redis redis; @@ -37,9 +39,14 @@ public class DefaultRedisProvider implements RedisProvider { private final AtomicReference> connectPromiseRef = new AtomicReference<>(); - public DefaultRedisProvider(Vertx vertx, ModuleConfiguration configuration) { + public DefaultRedisProvider( + Vertx vertx, + ModuleConfiguration configuration, + RestStorageExceptionFactory exceptionFactory + ) { this.vertx = vertx; this.configuration = configuration; + this.exceptionFactory = exceptionFactory; } @Override @@ -102,9 +109,9 @@ private Future connectToRedis() { createConnectStrings().forEach(redisOptions::addConnectionString); redis = Redis.createClient(vertx, redisOptions); - redis.connect().onComplete( ev -> { - if( ev.failed() ) { - promise.fail(new Exception("redis.connect()", ev.cause())); + redis.connect().onComplete(ev -> { + if (ev.failed()) { + promise.fail(exceptionFactory.newException("redis.connect() failed", ev.cause())); connecting.set(false); return; } diff --git a/src/main/java/org/swisspush/reststorage/EventBusAdapter.java b/src/main/java/org/swisspush/reststorage/EventBusAdapter.java index 92196e4..e6a35bd 100644 --- a/src/main/java/org/swisspush/reststorage/EventBusAdapter.java +++ b/src/main/java/org/swisspush/reststorage/EventBusAdapter.java @@ -14,6 +14,7 @@ import io.vertx.core.net.NetSocket; import io.vertx.core.net.SocketAddress; import org.slf4j.Logger; +import org.swisspush.reststorage.exception.RestStorageExceptionFactory; import javax.net.ssl.SSLSession; import javax.security.cert.X509Certificate; @@ -34,9 +35,18 @@ public class EventBusAdapter { private static final Logger log = getLogger(EventBusAdapter.class); + private final RestStorageExceptionFactory exceptionFactory; + + public EventBusAdapter( + RestStorageExceptionFactory exceptionFactory + ) { + this.exceptionFactory = exceptionFactory; + } public void init(final Vertx vertx, String address, final Handler requestHandler) { - vertx.eventBus().consumer(address, (Handler>) message -> requestHandler.handle(new MappedHttpServerRequest(vertx, message))); + vertx.eventBus().consumer(address, (Handler>) message -> { + requestHandler.handle(new MappedHttpServerRequest(vertx, message, exceptionFactory)); + }); } private static class MappedHttpServerRequest extends HttpServerRequestInternal { @@ -46,6 +56,7 @@ private static class MappedHttpServerRequest extends HttpServerRequestInternal { private final String uri; private final MultiMap requestHeaders; private final Message message; + private final RestStorageExceptionFactory exceptionFactory; private String path; private String query; private MultiMap params; @@ -54,9 +65,14 @@ private static class MappedHttpServerRequest extends HttpServerRequestInternal { private Handler endHandler; private HttpServerResponse response; - private MappedHttpServerRequest(Vertx vertx, Message message) { + private MappedHttpServerRequest( + Vertx vertx, + Message message, + RestStorageExceptionFactory exceptionFactory + ) { this.vertx = vertx; this.message = message; + this.exceptionFactory = exceptionFactory; Buffer buffer = message.body(); int headerLength = buffer.getInt(0); JsonObject header = new JsonObject(buffer.getString(4, headerLength + 4)); @@ -487,15 +503,15 @@ public boolean writeQueueFull() { @Override public HttpServerResponse drainHandler(Handler voidHandler) { - log.warn("I wish you a happy timeout as this method ignores drainHandler anyway.", - new Exception("may this stacktrace help you")); + log.warn("stacktrace", exceptionFactory.newException( + "I wish you a happy timeout as this method ignores drainHandler anyway.")); return this; } @Override public HttpServerResponse exceptionHandler(Handler throwableHandler) { - log.warn("I wish you a happy debugging session as this method ignores exceptionHandler anyway.", - new Exception("may this stacktrace help you")); + log.warn("stacktrace", exceptionFactory.newException( + "I wish you a happy debugging session as this method ignores exceptionHandler anyway.")); return this; } }; diff --git a/src/main/java/org/swisspush/reststorage/FileSystemStorage.java b/src/main/java/org/swisspush/reststorage/FileSystemStorage.java index 145483b..6bf06be 100644 --- a/src/main/java/org/swisspush/reststorage/FileSystemStorage.java +++ b/src/main/java/org/swisspush/reststorage/FileSystemStorage.java @@ -8,6 +8,7 @@ import io.vertx.core.file.OpenOptions; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.swisspush.reststorage.exception.RestStorageExceptionFactory; import org.swisspush.reststorage.util.LockMode; import java.io.File; @@ -24,13 +25,15 @@ public class FileSystemStorage implements Storage { private final String root; private final Vertx vertx; + private final RestStorageExceptionFactory exceptionFactory; private final int rootLen; private final FileSystemDirLister fileSystemDirLister; private final Logger log = LoggerFactory.getLogger(FileSystemStorage.class); - public FileSystemStorage(Vertx vertx, String root) { + public FileSystemStorage(Vertx vertx, RestStorageExceptionFactory exceptionFactory, String root) { this.vertx = vertx; + this.exceptionFactory = exceptionFactory; this.fileSystemDirLister = new FileSystemDirLister(vertx, root); // Unify format for simpler work. String tmpRoot; @@ -63,8 +66,9 @@ public void get(String path, String etag, final int offset, final int count, fin fileSystem().exists(fullPath, booleanAsyncResult -> { if( booleanAsyncResult.failed() ){ String msg = "vertx.fileSystem().exists()"; - if( log.isWarnEnabled() ){ - log.warn(msg, new Exception(fullPath, booleanAsyncResult.cause())); + if (log.isWarnEnabled()) { + log.warn(msg, exceptionFactory.newException("fileSystem().exists(" + fullPath + ") failed", + booleanAsyncResult.cause())); } Resource r = new Resource(); r.error = true; @@ -83,8 +87,9 @@ public void get(String path, String etag, final int offset, final int count, fin fileSystem().props(fullPath, filePropsAsyncResult -> { if( filePropsAsyncResult.failed() ){ String msg = "vertx.fileSystem().props()"; - if( log.isWarnEnabled() ){ - log.warn(msg, new Exception(fullPath, filePropsAsyncResult.cause())); + if (log.isWarnEnabled()) { + log.warn(msg, exceptionFactory.newException("fileSystem().props(" + fullPath + ")", + filePropsAsyncResult.cause())); } Resource r = new Resource(); r.error = true; diff --git a/src/main/java/org/swisspush/reststorage/RestStorageHandler.java b/src/main/java/org/swisspush/reststorage/RestStorageHandler.java index da6fd6c..e217494 100644 --- a/src/main/java/org/swisspush/reststorage/RestStorageHandler.java +++ b/src/main/java/org/swisspush/reststorage/RestStorageHandler.java @@ -8,12 +8,14 @@ import io.vertx.core.http.HttpServerResponse; import io.vertx.core.json.JsonArray; import io.vertx.core.json.JsonObject; +import io.vertx.core.spi.ExecutorServiceFactory; import io.vertx.core.streams.Pump; import io.vertx.ext.auth.authentication.AuthenticationProvider; import io.vertx.ext.web.Router; import io.vertx.ext.web.RoutingContext; import io.vertx.ext.web.handler.BasicAuthHandler; import org.slf4j.Logger; +import org.swisspush.reststorage.exception.RestStorageExceptionFactory; import org.swisspush.reststorage.util.*; import java.text.DecimalFormat; @@ -29,6 +31,7 @@ public class RestStorageHandler implements Handler { private final Logger log; private final Router router; private final Storage storage; + private final RestStorageExceptionFactory exceptionFactory; private final MimeTypeResolver mimeTypeResolver = new MimeTypeResolver("application/json; charset=utf-8"); @@ -43,10 +46,17 @@ public class RestStorageHandler implements Handler { private final DecimalFormat decimalFormat; private final Integer maxStorageExpandSubresources; - public RestStorageHandler(Vertx vertx, final Logger log, final Storage storage, final ModuleConfiguration config) { + public RestStorageHandler( + Vertx vertx, + Logger log, + Storage storage, + RestStorageExceptionFactory exceptionFactory, + ModuleConfiguration config + ) { this.router = Router.router(vertx); this.log = log; this.storage = storage; + this.exceptionFactory = exceptionFactory; this.prefix = config.getPrefix(); this.confirmCollectionDelete = config.isConfirmCollectionDelete(); this.rejectStorageWriteOnLowMemory = config.isRejectStorageWriteOnLowMemory(); @@ -65,7 +75,8 @@ public RestStorageHandler(Vertx vertx, final Logger log, final Storage storage, Result result = checkHttpAuthenticationConfiguration(config); if(result.isErr()) { router.route().handler(ctx -> { - log.warn("router.route()", new Exception(result.getErr())); + log.warn("stacktrace", exceptionFactory.newException( + "router.route() failed: " + result.getErr())); respondWith(ctx.response(), StatusCode.INTERNAL_SERVER_ERROR, result.getErr()); }); } else if (result.getOk()) { @@ -278,8 +289,10 @@ public void handle(Resource resource) { documentResource.closeHandler.handle(null); rsp.end(); }); - documentResource.addErrorHandler(ex -> log.error("TODO error handling", new Exception(ex))); - documentResource.readStream.exceptionHandler(ex -> log.error("TODO error handling", new Exception(ex))); + documentResource.addErrorHandler((Throwable ex) -> log.error("stacktrace", + exceptionFactory.newException("TODO error handling", ex))); + documentResource.readStream.exceptionHandler((Throwable ex) -> log.error("stacktrace", + exceptionFactory.newException("TODO error handling", ex))); pump.start(); } } @@ -487,8 +500,9 @@ private void putResource_storeContentsOfDocumentResource(RoutingContext ctx, Doc // Caller is responsible to do any 'error', 'exists', 'rejected' checks on the // resource. Therefore we simply go forward and store its content. final HttpServerRequest request = ctx.request(); - resource.addErrorHandler(ex -> { - if( log.isDebugEnabled() ) log.debug("Happy stacktrace just for you", new Exception(ex)); + resource.addErrorHandler((Throwable ex) -> { + if (log.isDebugEnabled()) log.debug("stacktrace", + exceptionFactory.newException("DocumentResource reports error", ex)); respondWith(response, StatusCode.INTERNAL_SERVER_ERROR, ex.getMessage()); }); // Complete response when resource written. @@ -504,7 +518,7 @@ private void putResource_storeContentsOfDocumentResource(RoutingContext ctx, Doc resource.errorMessage = exc.getMessage(); final Handler resourceErrorHandler = resource.errorHandler; if (resourceErrorHandler != null) { - resourceErrorHandler.handle(new Exception(exc)); + resourceErrorHandler.handle(exceptionFactory.newException(exc)); } }); final Pump pump = Pump.pump(request, resource.writeStream); diff --git a/src/main/java/org/swisspush/reststorage/RestStorageMod.java b/src/main/java/org/swisspush/reststorage/RestStorageMod.java index 04887f1..baa6713 100644 --- a/src/main/java/org/swisspush/reststorage/RestStorageMod.java +++ b/src/main/java/org/swisspush/reststorage/RestStorageMod.java @@ -41,7 +41,8 @@ public void start(Promise promise) { if (event.failed()) { promise.fail(event.cause()); } else { - Handler handler = new RestStorageHandler(vertx, log, event.result(), modConfig); + Handler handler = new RestStorageHandler( + vertx, log, event.result(), exceptionFactory, modConfig); if(modConfig.isHttpRequestHandlerEnabled()) { // in Vert.x 2x 100-continues was activated per default, in vert.x 3x it is off per default. @@ -49,14 +50,15 @@ public void start(Promise promise) { vertx.createHttpServer(options).requestHandler(handler).listen(modConfig.getPort(), result -> { if (result.succeeded()) { - new EventBusAdapter().init(vertx, modConfig.getStorageAddress(), handler); + new EventBusAdapter(exceptionFactory).init(vertx, modConfig.getStorageAddress(), handler); promise.complete(); } else { - promise.fail(new Exception(result.cause())); + promise.fail(exceptionFactory.newException( + "vertx.HttpServer.listen(" + modConfig.getPort() + ") failed", result.cause())); } }); } else { - new EventBusAdapter().init(vertx, modConfig.getStorageAddress(), handler); + new EventBusAdapter(exceptionFactory).init(vertx, modConfig.getStorageAddress(), handler); promise.complete(); } } @@ -68,19 +70,19 @@ private Future createStorage(ModuleConfiguration moduleConfiguration) { switch (moduleConfiguration.getStorageType()) { case filesystem: - promise.complete(new FileSystemStorage(vertx, moduleConfiguration.getRoot())); + promise.complete(new FileSystemStorage(vertx, exceptionFactory, moduleConfiguration.getRoot())); break; case redis: createRedisStorage(vertx, moduleConfiguration).onComplete(event -> { if(event.succeeded()){ promise.complete(event.result()); } else { - promise.fail(new Exception(event.cause())); + promise.fail(exceptionFactory.newException("createRedisStorage() failed", event.cause())); } }); break; default: - promise.fail(new RuntimeException("Storage not supported: " + moduleConfiguration.getStorageType())); + promise.fail(exceptionFactory.newException("Storage not supported: " + moduleConfiguration.getStorageType())); } return promise.future(); @@ -90,14 +92,14 @@ private Future createRedisStorage(Vertx vertx, ModuleConfiguration Promise initPromise = Promise.promise(); if(redisProvider == null) { - redisProvider = new DefaultRedisProvider(vertx, moduleConfiguration); + redisProvider = new DefaultRedisProvider(vertx, moduleConfiguration, exceptionFactory); } redisProvider.redis().onComplete(event -> { if(event.succeeded()) { initPromise.complete(new RedisStorage(vertx, moduleConfiguration, redisProvider, exceptionFactory)); } else { - initPromise.fail(new Exception(event.cause())); + initPromise.fail(exceptionFactory.newException("redisProvider.redis() failed", event.cause())); } }); diff --git a/src/main/java/org/swisspush/reststorage/exception/RestStorageExceptionFactory.java b/src/main/java/org/swisspush/reststorage/exception/RestStorageExceptionFactory.java index 7cff9cc..a523daa 100644 --- a/src/main/java/org/swisspush/reststorage/exception/RestStorageExceptionFactory.java +++ b/src/main/java/org/swisspush/reststorage/exception/RestStorageExceptionFactory.java @@ -17,8 +17,22 @@ */ public interface RestStorageExceptionFactory { + /** Convenience overload for {@link #newException(String, Throwable)}. */ + public default Exception newException(String msg){ return newException(msg, null); } + + /** Convenience overload for {@link #newException(String, Throwable)}. */ + public default Exception newException(Throwable cause){ return newException(null, cause); } + public Exception newException(String message, Throwable cause); + /** Convenience overload for {@link #newRuntimeException(String, Throwable)}. */ + public default RuntimeException newRuntimeException(String msg){ return newRuntimeException(msg, null); } + + /** Convenience overload for {@link #newRuntimeException(String, Throwable)}. */ + public default RuntimeException newRuntimeException(Throwable cause){ return newRuntimeException(null, cause); } + + public RuntimeException newRuntimeException(String message, Throwable cause); + public ReplyException newReplyException(ReplyFailure failureType, int failureCode, String message); diff --git a/src/main/java/org/swisspush/reststorage/exception/RestStorageThriftyExceptionFactory.java b/src/main/java/org/swisspush/reststorage/exception/RestStorageThriftyExceptionFactory.java index 712ad64..0578265 100644 --- a/src/main/java/org/swisspush/reststorage/exception/RestStorageThriftyExceptionFactory.java +++ b/src/main/java/org/swisspush/reststorage/exception/RestStorageThriftyExceptionFactory.java @@ -18,10 +18,22 @@ class RestStorageThriftyExceptionFactory implements RestStorageExceptionFactory @Override public Exception newException(String message, Throwable cause) { + // This implementation cares about performance. So if 'cause' is already + // of the correct type, we just re-use it directly. There's no need to + // produce yet another exception instance. if (cause instanceof Exception) return (Exception) cause; return new RestStorageNoStacktraceException(message, cause); } + @Override + public RuntimeException newRuntimeException(String msg, Throwable cause) { + // This implementation cares about performance. So if 'cause' is already + // of the correct type, we just re-use it directly. There's no need to + // produce yet another exception instance. + if (cause instanceof RuntimeException) return (RuntimeException) cause; + return new RestStorageNoStacktraceException(msg, cause); + } + @Override public ReplyException newReplyException(ReplyFailure failureType, int failureCode, String message) { return new RestStorageNoStackReplyException(failureType, failureCode, message); diff --git a/src/main/java/org/swisspush/reststorage/exception/RestStorageWastefulExceptionFactory.java b/src/main/java/org/swisspush/reststorage/exception/RestStorageWastefulExceptionFactory.java index 7171124..914e0f0 100644 --- a/src/main/java/org/swisspush/reststorage/exception/RestStorageWastefulExceptionFactory.java +++ b/src/main/java/org/swisspush/reststorage/exception/RestStorageWastefulExceptionFactory.java @@ -22,6 +22,11 @@ public Exception newException(String message, Throwable cause) { return new Exception(message, cause); } + @Override + public RuntimeException newRuntimeException(String msg, Throwable cause) { + return new RuntimeException(msg, cause); + } + @Override public ReplyException newReplyException(ReplyFailure failureType, int failureCode, String message) { return new ReplyException(failureType, failureCode, message); diff --git a/src/main/java/org/swisspush/reststorage/lock/impl/RedisBasedLock.java b/src/main/java/org/swisspush/reststorage/lock/impl/RedisBasedLock.java index e30bc52..dc4c4d5 100644 --- a/src/main/java/org/swisspush/reststorage/lock/impl/RedisBasedLock.java +++ b/src/main/java/org/swisspush/reststorage/lock/impl/RedisBasedLock.java @@ -43,7 +43,8 @@ public RedisBasedLock( ) { this.redisProvider = redisProvider; this.exceptionFactory = exceptionFactory; - this.releaseLockLuaScriptState = new LuaScriptState(LockLuaScripts.LOCK_RELEASE, redisProvider, false); + this.releaseLockLuaScriptState = new LuaScriptState( + LockLuaScripts.LOCK_RELEASE, redisProvider, exceptionFactory, false); } private void redisSetWithOptions(String key, String value, boolean nx, long px, Handler> handler) { @@ -54,14 +55,16 @@ private void redisSetWithOptions(String key, String value, boolean nx, long px, } redisProvider.redis().onComplete( redisEv -> { if( redisEv.failed() ){ - handler.handle(new FailedAsyncResult<>(new Exception("redisProvider.redis()", redisEv.cause()))); + Throwable ex = exceptionFactory.newException("redisProvider.redis() failed", redisEv.cause()); + handler.handle(new FailedAsyncResult<>(ex)); return; } var redisAPI = redisEv.result(); String[] payload = RedisUtils.toPayload(key, value, options).toArray(EMPTY_STRING_ARRAY); redisAPI.send(Command.SET, payload).onComplete( ev -> { if( ev.failed() ){ - handler.handle(new FailedAsyncResult<>(new Exception("redisAPI.send(SET, ...)", ev.cause()))); + Throwable ex = exceptionFactory.newException("redisAPI.send(SET, ...) failed", ev.cause()); + handler.handle(new FailedAsyncResult<>(ex)); }else{ handler.handle(ev); } @@ -92,7 +95,7 @@ public Future releaseLock(String lock, String token) { List keys = Collections.singletonList(buildLockKey(lock)); List arguments = Collections.singletonList(token); ReleaseLockRedisCommand cmd = new ReleaseLockRedisCommand(releaseLockLuaScriptState, - keys, arguments, redisProvider, log, promise); + keys, arguments, redisProvider, exceptionFactory, log, promise); cmd.exec(0); return promise.future(); } diff --git a/src/main/java/org/swisspush/reststorage/lock/lua/LuaScriptState.java b/src/main/java/org/swisspush/reststorage/lock/lua/LuaScriptState.java index 4904b48..1bae66d 100644 --- a/src/main/java/org/swisspush/reststorage/lock/lua/LuaScriptState.java +++ b/src/main/java/org/swisspush/reststorage/lock/lua/LuaScriptState.java @@ -3,6 +3,7 @@ import org.apache.commons.codec.digest.DigestUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.swisspush.reststorage.exception.RestStorageExceptionFactory; import org.swisspush.reststorage.redis.RedisProvider; import java.io.BufferedReader; @@ -29,12 +30,19 @@ public class LuaScriptState { private String sha; private final RedisProvider redisProvider; + private final RestStorageExceptionFactory exceptionFactory; private final Logger log = LoggerFactory.getLogger(LuaScriptState.class); - public LuaScriptState(LuaScript luaScriptType, RedisProvider redisProvider, boolean logoutput) { + public LuaScriptState( + LuaScript luaScriptType, + RedisProvider redisProvider, + RestStorageExceptionFactory exceptionFactory, + boolean logoutput + ) { this.luaScriptType = luaScriptType; this.redisProvider = redisProvider; + this.exceptionFactory = exceptionFactory; this.logoutput = logoutput; this.composeLuaScript(luaScriptType); this.loadLuaScript(new RedisCommandDoNothing(), 0); @@ -87,15 +95,15 @@ public void loadLuaScript(final RedisCommand redisCommand, int executionCounter) final int executionCounterIncr = ++executionCounter; // check first if the lua script already exists in the store - redisProvider.redis().onComplete( ev -> { - if( ev.failed() ){ - log.error("redisProvider.redis()", new Exception("stacktrace", ev.cause())); + redisProvider.redis().onComplete(ev -> { + if (ev.failed()) { + log.error("stacktrace", exceptionFactory.newException("redisProvider.redis() failed", ev.cause())); return; } var redisAPI = ev.result(); redisAPI.script(Arrays.asList("exists", sha), existsEv -> { - if( existsEv.failed() ) { - log.error("Error checking whether lua script exists", new Exception("stacktrace", existsEv.cause())); + if (existsEv.failed()) { + log.error("stacktrace", exceptionFactory.newException("Error checking whether lua script exists", existsEv.cause())); return; } Long exists = existsEv.result().get(0).toLong(); @@ -105,8 +113,9 @@ public void loadLuaScript(final RedisCommand redisCommand, int executionCounter) } else { log.info("load lua script for script type: {} logutput: {}", luaScriptType, logoutput); redisAPI.script(Arrays.asList("load", script), loadEv -> { - if( loadEv.failed() ) { - log.error("stacktrace", new Exception("stacktrace", loadEv.cause())); + if (loadEv.failed()) { + log.error("stacktrace", exceptionFactory.newException( + "redisAPI.script(['load', script) failed", loadEv.cause())); return; } String newSha = loadEv.result().toString(); diff --git a/src/main/java/org/swisspush/reststorage/lock/lua/ReleaseLockRedisCommand.java b/src/main/java/org/swisspush/reststorage/lock/lua/ReleaseLockRedisCommand.java index 7b6fe1a..7be2ce7 100644 --- a/src/main/java/org/swisspush/reststorage/lock/lua/ReleaseLockRedisCommand.java +++ b/src/main/java/org/swisspush/reststorage/lock/lua/ReleaseLockRedisCommand.java @@ -2,6 +2,7 @@ import io.vertx.core.Promise; import org.slf4j.Logger; +import org.swisspush.reststorage.exception.RestStorageExceptionFactory; import org.swisspush.reststorage.redis.RedisProvider; import java.util.ArrayList; @@ -17,14 +18,23 @@ public class ReleaseLockRedisCommand implements RedisCommand { private final List arguments; private final Promise promise; private final RedisProvider redisProvider; + private final RestStorageExceptionFactory exceptionFactory; private final Logger log; - public ReleaseLockRedisCommand(LuaScriptState luaScriptState, List keys, List arguments, - RedisProvider redisProvider, Logger log, final Promise promise) { + public ReleaseLockRedisCommand( + LuaScriptState luaScriptState, + List keys, + List arguments, + RedisProvider redisProvider, + RestStorageExceptionFactory exceptionFactory, + Logger log, + final Promise promise + ) { this.luaScriptState = luaScriptState; this.keys = keys; this.arguments = arguments; this.redisProvider = redisProvider; + this.exceptionFactory = exceptionFactory; this.log = log; this.promise = promise; } @@ -39,7 +49,7 @@ public void exec(int executionCounter) { redisProvider.redis().onComplete( redisEv -> { if( redisEv.failed() ){ - promise.fail(new Exception("redisProvider.redis()", redisEv.cause())); + promise.fail(exceptionFactory.newException("redisProvider.redis() failed", redisEv.cause())); return; } var redisAPI = redisEv.result(); @@ -54,7 +64,7 @@ public void exec(int executionCounter) { promise.fail("amount the script got loaded is higher than 10, we abort"); } else { luaScriptState.loadLuaScript(new ReleaseLockRedisCommand(luaScriptState, keys, - arguments, redisProvider, log, promise), executionCounter); + arguments, redisProvider, exceptionFactory, log, promise), executionCounter); } } else { promise.fail(new Exception("ReleaseLockRedisCommand request failed", ex)); diff --git a/src/main/java/org/swisspush/reststorage/redis/RedisStorage.java b/src/main/java/org/swisspush/reststorage/redis/RedisStorage.java index d82c140..4124bf8 100644 --- a/src/main/java/org/swisspush/reststorage/redis/RedisStorage.java +++ b/src/main/java/org/swisspush/reststorage/redis/RedisStorage.java @@ -77,6 +77,7 @@ public class RedisStorage implements Storage { private final Lock lock; private final RedisProvider redisProvider; + private final RestStorageExceptionFactory exceptionFactory; private RedisMonitor redisMonitor; private final Map luaScripts = new HashMap<>(); @@ -104,6 +105,7 @@ public RedisStorage( this.vertx = vertx; this.redisProvider = redisProvider; + this.exceptionFactory = exceptionFactory; this.decimalFormat = new DecimalFormat(); this.decimalFormat.setMaximumFractionDigits(1); @@ -207,14 +209,14 @@ public Future> calculateCurrentMemoryUsage() { redisProvider.redis().onComplete( ev -> { if( ev.failed() ){ log.error("Unable to get memory information from redis", - new Exception("A happy stacktrace just for you", ev.cause())); + exceptionFactory.newException("redisProvider.redis() failed", ev.cause())); promise.complete(Optional.empty()); } var redisAPI = ev.result(); redisAPI.info(Collections.singletonList("memory"), memoryInfo -> { if (memoryInfo.failed()) { log.error("Unable to get memory information from redis", - new Exception("stacktrace", memoryInfo.cause())); + exceptionFactory.newException("redisAPI.info([\"memory\"]) failed", memoryInfo.cause())); promise.complete(Optional.empty()); return; } @@ -389,13 +391,17 @@ public void recomposeLuaScript() { public void loadLuaScript(final RedisCommand redisCommand, int executionCounter) { final int executionCounterIncr = ++executionCounter; - redisProvider.redis().onComplete( ev -> { - if( ev.failed() ) throw new RuntimeException("RedisProvider.redis()", ev.cause()); + redisProvider.redis().onComplete(ev -> { + if (ev.failed()) { + throw exceptionFactory.newRuntimeException("RedisProvider.redis()", ev.cause()); + } var redisAPI = ev.result(); // check first if the lua script already exists in the store redisAPI.script(Arrays.asList("exists", sha), existsEv -> { - if( existsEv.failed() ) - throw new RuntimeException("Error checking whether lua script exists", existsEv.cause()); + if (existsEv.failed()) { + throw exceptionFactory.newRuntimeException( + "redisAPI.script(['exists', '" + sha + "') failed", existsEv.cause()); + } Long exists = existsEv.result().get(0).toLong(); // if script already if (Long.valueOf(1).equals(exists)) { @@ -405,8 +411,8 @@ public void loadLuaScript(final RedisCommand redisCommand, int executionCounter) log.info("load lua script for script type: {} logoutput: {}", luaScriptType, logoutput); redisAPI.script(Arrays.asList("load", script), loadEv -> { if (loadEv.failed()) { - log.error("Loading of lua script {} failed", - luaScriptType, new Exception("stacktrace", loadEv.cause())); + log.error("Loading of lua script {} failed", luaScriptType, + exceptionFactory.newException("redisAPI.script(['load', ...)", loadEv.cause())); return; } String newSha = loadEv.result().toString(); @@ -606,7 +612,9 @@ public Get(List keys, List arguments, final Handler ha public void exec(final int executionCounter) { List args = toPayload(luaScripts.get(LuaScript.GET).getSha(), keys.size(), keys, arguments); redisProvider.redis().onComplete( ev -> { - if( ev.failed() ) throw new RuntimeException("redisProvider.redis()", ev.cause()); + if (ev.failed()) { + throw exceptionFactory.newRuntimeException("redisProvider.redis() failed", ev.cause()); + } var redisAPI = ev.result(); redisAPI.evalsha(args, evalShaEv -> { if (evalShaEv.succeeded()) { @@ -633,7 +641,7 @@ public void exec(final int executionCounter) { luaScripts.get(LuaScript.GET).loadLuaScript(new Get(keys, arguments, handler), executionCounter); } } else { - log.error("GET request failed", new Exception("Happy stacktrace", ex)); + log.error("GET request failed", exceptionFactory.newException("redisAPI.evalsha() failed", ex)); } } }); @@ -680,14 +688,17 @@ public void exec(final int executionCounter) { List args = toPayload(luaScripts.get(LuaScript.STORAGE_EXPAND).getSha(), keys.size(), keys, arguments); redisProvider.redis().onComplete( redisEv -> { - if( redisEv.failed() ) throw new RuntimeException("redisProvider.redis()", redisEv.cause()); + if (redisEv.failed()) { + throw exceptionFactory.newRuntimeException("redisProvider.redis() failed", redisEv.cause()); + } var redisAPI = redisEv.result(); redisAPI.evalsha(args, evalShaEv -> { if( evalShaEv.failed() ){ Throwable ex = evalShaEv.cause(); String message = ex.getMessage(); if (message != null && message.startsWith("NOSCRIPT")) { - log.warn("storageExpand script couldn't be found, reload it", new Exception(ex)); + log.warn("storageExpand script couldn't be found, reload it", + exceptionFactory.newException("redisAPI.evalsha() failed", ex)); log.warn("amount the script got loaded: {}", executionCounter); if (executionCounter > 10) { log.error("amount the script got loaded is higher than 10, we abort"); @@ -696,7 +707,8 @@ public void exec(final int executionCounter) { new StorageExpand(keys, arguments, handler, etag), executionCounter); } } else { - log.error("StorageExpand request failed with message", new Exception("stacktrace", ex)); + log.error("StorageExpand request failed with message", + exceptionFactory.newException("redisAPI.evalsha() failed", ex)); } return; } @@ -787,8 +799,10 @@ private void handleJsonArrayValues(Response values, Handler handler, b }; handler.handle(r); } else { - if (log.isInfoEnabled()) - log.info("stacktrace just for you", new Exception(decompressedResult.cause())); + if (log.isInfoEnabled()) { + log.info("stacktrace, because handler cannot receive it", exceptionFactory.newException( + "GZIPUtil.decompressResource() failed", decompressedResult.cause())); + } error(handler, "Error during decompression of resource: " + decompressedResult.cause().getMessage()); } }); @@ -956,7 +970,8 @@ public void put(String path, String etag, boolean merge, long expire, String loc ); reloadScriptIfLoglevelChangedAndExecuteRedisCommand(LuaScript.PUT, new Put(d, keys, arg, handler), 0); } else { - log.info("stacktrace", new Exception("stacktrace", compressResourceResult.cause())); + if (log.isInfoEnabled()) log.info("stacktrace", exceptionFactory.newException( + "GZIPUtil.compressResource(stream.getBytes()) failed", compressResourceResult.cause())); error(handler, "Error during compression of resource: "+ compressResourceResult.cause().getMessage()); } }); @@ -1004,8 +1019,10 @@ public Put(DocumentResource d, List keys, List arguments, Handle public void exec(final int executionCounter) { List args = toPayload(luaScripts.get(LuaScript.PUT).getSha(), keys.size(), keys, arguments); - redisProvider.redis().onComplete( redisEv -> { - if( redisEv.failed() ) throw new RuntimeException("redisProvider.redis()", redisEv.cause()); + redisProvider.redis().onComplete(redisEv -> { + if (redisEv.failed()) { + throw exceptionFactory.newRuntimeException("redisProvider.redis() failed", redisEv.cause()); + } var redisAPI = redisEv.result(); redisAPI.evalsha(args, evalShaEv -> { if (evalShaEv.succeeded()) { @@ -1037,10 +1054,11 @@ public void exec(final int executionCounter) { luaScripts.get(LuaScript.PUT).loadLuaScript(new Put(d, keys, arguments, handler), executionCounter); } } else if ( d.errorHandler != null ) { - if( log.isDebugEnabled() ) log.debug("PUT request failed", new Exception("stacktrace", ex)); + if (log.isDebugEnabled()) log.debug("PUT request failed", + exceptionFactory.newException("redisAPI.evalsha() failed", ex)); d.errorHandler.handle(ex); }else{ - log.error("PUT request failed", new Exception("stacktrace", ex)); + log.error("PUT request failed", exceptionFactory.newException("redisAPI.evalsha() failed", ex)); } } }); @@ -1095,8 +1113,9 @@ public void exec(final int executionCounter) { List args = toPayload(luaScripts.get(LuaScript.DELETE).getSha(), keys.size(), keys, arguments); redisProvider.redis().onComplete( ev -> { - if( ev.failed() ){ - log.error("redisProvider.redis()", new Exception(ev.cause())); + if (ev.failed()) { + log.error("redisProvider.redis()", exceptionFactory.newException( + "redisProvider.redis() failed", ev.cause())); return; } RedisAPI redisAPI = ev.result(); @@ -1164,9 +1183,10 @@ public void cleanupRecursive(final Handler handler, final long ); List args = toPayload(luaScripts.get(LuaScript.CLEANUP).getSha(), 0, Collections.emptyList(), arguments); - redisProvider.redis().onComplete( ev -> { - if( ev.failed() ){ - log.error("Redis: cleanupRecursive failed", new Exception("redisProvider.redis()", ev.cause())); + redisProvider.redis().onComplete(ev -> { + if (ev.failed()) { + log.error("Redis: cleanupRecursive failed", exceptionFactory.newException( + "redisProvider.redis() failed", ev.cause())); return; } var redisAPI = ev.result(); @@ -1177,7 +1197,7 @@ public void cleanupRecursive(final Handler handler, final long log.warn("the cleanup script is not loaded. Load it and exit. The Cleanup will success the next time", ex); luaScripts.get(LuaScript.CLEANUP).loadLuaScript(new RedisCommandDoNothing(), 0); }else { - if( log.isInfoEnabled() ) log.info("stacktrace", new Exception("stacktrace", ex)); + if (log.isInfoEnabled()) log.info("stacktrace", exceptionFactory.newException("redisApi.evalsha() failed", ex)); DocumentResource r = new DocumentResource(); r.invalid = r.rejected = r.error = true; r.errorMessage = ex.getMessage(); diff --git a/src/test/java/org/swisspush/reststorage/FilesystemStorageTest.java b/src/test/java/org/swisspush/reststorage/FilesystemStorageTest.java index 491b01f..89d8104 100644 --- a/src/test/java/org/swisspush/reststorage/FilesystemStorageTest.java +++ b/src/test/java/org/swisspush/reststorage/FilesystemStorageTest.java @@ -25,6 +25,7 @@ import java.util.UUID; import static org.mockito.Mockito.mock; +import static org.swisspush.reststorage.exception.RestStorageExceptionFactory.newRestStorageWastefulExceptionFactory; /** * Tests for the {@link FileSystemStorage} class @@ -38,7 +39,7 @@ public class FilesystemStorageTest { @Test(expected=UnsupportedOperationException.class) public void testGetMemoryUsageNotYetImplemented(TestContext testContext){ - FileSystemStorage storage = new FileSystemStorage(mock(Vertx.class), "/root"); + FileSystemStorage storage = new FileSystemStorage(mock(Vertx.class), newRestStorageWastefulExceptionFactory(), "/root"); storage.getCurrentMemoryUsage(); } @@ -115,7 +116,7 @@ public FileSystem delete(String s, Handler> handler) { return fileSystem; } }; - victim = new FileSystemStorage(mockedVertx, root); + victim = new FileSystemStorage(mockedVertx, newRestStorageWastefulExceptionFactory(), root); } // Challenge victim @@ -225,7 +226,7 @@ public void cleanupEmptyParentDirsOnResourceDeletion(final TestContext testConte // Use pseudo root (could be anything because our test will never access real filesystem). final String root = createPseudoFileStorageRoot(); // Wire up victim instance. - victim = new FileSystemStorage(mockedVertx, root); + victim = new FileSystemStorage(mockedVertx, newRestStorageWastefulExceptionFactory(), root); } // Challenge victim @@ -289,7 +290,7 @@ public long setTimer(long delayMillis, Handler handler) { return realVertx.setTimer(delayMillis, handler); } }; - victim = new FileSystemStorage(mockedVertx, root); + victim = new FileSystemStorage(mockedVertx, newRestStorageWastefulExceptionFactory(), root); } // Trigger work diff --git a/src/test/java/org/swisspush/reststorage/RestStorageHandlerTest.java b/src/test/java/org/swisspush/reststorage/RestStorageHandlerTest.java index e45c1c4..e10a2bb 100644 --- a/src/test/java/org/swisspush/reststorage/RestStorageHandlerTest.java +++ b/src/test/java/org/swisspush/reststorage/RestStorageHandlerTest.java @@ -30,6 +30,7 @@ import static org.mockito.Matchers.eq; import static org.mockito.Mockito.*; +import static org.swisspush.reststorage.exception.RestStorageExceptionFactory.newRestStorageWastefulExceptionFactory; /** * Tests for the {@link RestStorageHandler} class @@ -76,7 +77,7 @@ public void testInvalidAuthenticationConfiguration(TestContext testContext) { .httpRequestHandlerAuthenticationEnabled(true) .httpRequestHandlerUsername("foo"); - restStorageHandler = new RestStorageHandler(vertx, log, storage, config); + restStorageHandler = new RestStorageHandler(vertx, log, storage, newRestStorageWastefulExceptionFactory(), config); // ARRANGE when(request.headers()).thenReturn(new HeadersMultiMap()); @@ -96,7 +97,7 @@ public void testInvalidAuthenticationConfiguration(TestContext testContext) { .httpRequestHandlerAuthenticationEnabled(true) .httpRequestHandlerPassword("bar"); - restStorageHandler = new RestStorageHandler(vertx, log, storage, config); + restStorageHandler = new RestStorageHandler(vertx, log, storage, newRestStorageWastefulExceptionFactory(), config); // ARRANGE when(request.headers()).thenReturn(new HeadersMultiMap()); @@ -119,7 +120,7 @@ public void testMissingAuthenticationHeader(TestContext testContext) { .httpRequestHandlerUsername("foo") .httpRequestHandlerPassword("bar"); - restStorageHandler = new RestStorageHandler(vertx, log, storage, config); + restStorageHandler = new RestStorageHandler(vertx, log, storage, newRestStorageWastefulExceptionFactory(), config); // ARRANGE when(request.headers()).thenReturn(new HeadersMultiMap()); @@ -140,7 +141,7 @@ public void testCorrectAuthenticationHeader(TestContext testContext) { .httpRequestHandlerUsername("foo") .httpRequestHandlerPassword("bar"); - restStorageHandler = new RestStorageHandler(vertx, log, storage, config); + restStorageHandler = new RestStorageHandler(vertx, log, storage, newRestStorageWastefulExceptionFactory(), config); // ARRANGE when(request.method()).thenReturn(HttpMethod.GET); @@ -172,7 +173,7 @@ public void testWrongAuthenticationHeader(TestContext testContext) { .httpRequestHandlerUsername("foo") .httpRequestHandlerPassword("bar"); - restStorageHandler = new RestStorageHandler(vertx, log, storage, config); + restStorageHandler = new RestStorageHandler(vertx, log, storage, newRestStorageWastefulExceptionFactory(), config); // ARRANGE when(request.method()).thenReturn(HttpMethod.GET); @@ -198,7 +199,7 @@ public void testWrongAuthenticationHeader(TestContext testContext) { @Test public void testPUTWithInvalidImportanceLevelHeader(TestContext testContext) { ModuleConfiguration config = new ModuleConfiguration().prefix("/").rejectStorageWriteOnLowMemory(true); - restStorageHandler = new RestStorageHandler(vertx, log, storage, config); + restStorageHandler = new RestStorageHandler(vertx, log, storage, newRestStorageWastefulExceptionFactory(), config); // ARRANGE when(request.headers()).thenReturn(new HeadersMultiMap().add(HttpRequestHeader.IMPORTANCE_LEVEL_HEADER.getName(), "not_a_number")); @@ -220,7 +221,7 @@ public void testPUTWithInvalidImportanceLevelHeader(TestContext testContext) { @Test public void testPUTWithEnabledRejectStorageWriteOnLowMemoryButNoHeaders(TestContext testContext) { ModuleConfiguration config = new ModuleConfiguration().prefix("/").rejectStorageWriteOnLowMemory(true); - restStorageHandler = new RestStorageHandler(vertx, log, storage, config); + restStorageHandler = new RestStorageHandler(vertx, log, storage, newRestStorageWastefulExceptionFactory(), config); // ACT restStorageHandler.handle(request); @@ -235,7 +236,7 @@ public void testPUTWithEnabledRejectStorageWriteOnLowMemoryButNoHeaders(TestCont @Test public void testPUTWithDisabledRejectStorageWriteOnLowMemoryButHeaders(TestContext testContext) { ModuleConfiguration config = new ModuleConfiguration().prefix("/").rejectStorageWriteOnLowMemory(false); - restStorageHandler = new RestStorageHandler(vertx, log, storage, config); + restStorageHandler = new RestStorageHandler(vertx, log, storage, newRestStorageWastefulExceptionFactory(), config); // ARRANGE when(request.headers()).thenReturn(new HeadersMultiMap().add(HttpRequestHeader.IMPORTANCE_LEVEL_HEADER.getName(), "50")); @@ -253,7 +254,7 @@ public void testPUTWithDisabledRejectStorageWriteOnLowMemoryButHeaders(TestConte @Test public void testPUTWithNoMemoryUsageAvailable(TestContext testContext) { ModuleConfiguration config = new ModuleConfiguration().prefix("/").rejectStorageWriteOnLowMemory(true); - restStorageHandler = new RestStorageHandler(vertx, log, storage, config); + restStorageHandler = new RestStorageHandler(vertx, log, storage, newRestStorageWastefulExceptionFactory(), config); // ARRANGE when(request.headers()).thenReturn(new HeadersMultiMap().add(HttpRequestHeader.IMPORTANCE_LEVEL_HEADER.getName(), "50")); @@ -270,7 +271,7 @@ public void testPUTWithNoMemoryUsageAvailable(TestContext testContext) { @Test public void testRejectPUTRequestWhenMemoryUsageHigherThanImportanceLevel(TestContext testContext) { ModuleConfiguration config = new ModuleConfiguration().prefix("/").rejectStorageWriteOnLowMemory(true); - restStorageHandler = new RestStorageHandler(vertx, log, storage, config); + restStorageHandler = new RestStorageHandler(vertx, log, storage, newRestStorageWastefulExceptionFactory(), config); // ARRANGE when(request.headers()).thenReturn(new HeadersMultiMap().add(HttpRequestHeader.IMPORTANCE_LEVEL_HEADER.getName(), "50")); @@ -335,7 +336,7 @@ public boolean writeQueueFull() { } }; final ModuleConfiguration config = new ModuleConfiguration(); - victim = new RestStorageHandler(mockedVertx, log, mockedStorage, config); + victim = new RestStorageHandler(mockedVertx, log, mockedStorage, newRestStorageWastefulExceptionFactory(), config); } // Mock request @@ -553,7 +554,7 @@ public String query() { final RestStorageHandler victim; { final ModuleConfiguration myConfig = new ModuleConfiguration().prefix("/").rejectStorageWriteOnLowMemory(true); - victim = new RestStorageHandler(null, log, storage, myConfig); + victim = new RestStorageHandler(null, log, storage, newRestStorageWastefulExceptionFactory(), myConfig); } // Trigger our funny request. From 25cdec32224608b9136ed8ad332afb753b3dc2da Mon Sep 17 00:00:00 2001 From: Andreas Fankhauser <23085769+hiddenalpha@users.noreply.github.com> Date: Thu, 23 May 2024 14:59:24 +0200 Subject: [PATCH 8/8] Fix more for 'https://github.com/swisspost/vertx-rest-storage/pull/186#pullrequestreview-2070250247' Related: SDCISA-15833, https://github.com/swisspost/vertx-redisques/issues/170, https://github.com/swisspost/vertx-redisques/pull/177, https://github.com/swisspost/vertx-rest-storage/pull/186, https://github.com/swisspost/gateleen/pull/577 --- .../swisspush/reststorage/lock/lua/ReleaseLockRedisCommand.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/swisspush/reststorage/lock/lua/ReleaseLockRedisCommand.java b/src/main/java/org/swisspush/reststorage/lock/lua/ReleaseLockRedisCommand.java index 7be2ce7..bddd0f7 100644 --- a/src/main/java/org/swisspush/reststorage/lock/lua/ReleaseLockRedisCommand.java +++ b/src/main/java/org/swisspush/reststorage/lock/lua/ReleaseLockRedisCommand.java @@ -67,7 +67,7 @@ public void exec(int executionCounter) { arguments, redisProvider, exceptionFactory, log, promise), executionCounter); } } else { - promise.fail(new Exception("ReleaseLockRedisCommand request failed", ex)); + promise.fail(exceptionFactory.newException("redisAPI.evalsha() failed", ex)); } return; }