From 770fbac67094ee0d570a17ba73243cfad6892df5 Mon Sep 17 00:00:00 2001 From: John Blum Date: Mon, 12 Jun 2023 12:34:35 -0700 Subject: [PATCH] Review and polish for confiuring a cache lock TTL. Closes #2300 Pull request: #2597. --- .../redis/cache/DefaultRedisCacheWriter.java | 38 ++- ...ion.java => FixedDurationTtlFunction.java} | 8 +- .../data/redis/cache/RedisCache.java | 67 +++-- .../redis/cache/RedisCacheConfiguration.java | 92 +++--- .../data/redis/cache/RedisCacheWriter.java | 30 +- .../data/redis/core/types/Expiration.java | 275 +++++++++--------- .../cache/DefaultRedisCacheWriterTests.java | 45 +-- .../cache/RedisCacheManagerUnitTests.java | 6 +- 8 files changed, 310 insertions(+), 251 deletions(-) rename src/main/java/org/springframework/data/redis/cache/{SingletonTtlFunction.java => FixedDurationTtlFunction.java} (72%) diff --git a/src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java b/src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java index c4656a164..226184842 100644 --- a/src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java +++ b/src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java @@ -31,12 +31,13 @@ import org.springframework.util.Assert; /** * {@link RedisCacheWriter} implementation capable of reading/writing binary data from/to Redis in {@literal standalone} - * and {@literal cluster} environments. Works upon a given {@link RedisConnectionFactory} to obtain the actual - * {@link RedisConnection}.
+ * and {@literal cluster} environments, and uses a given {@link RedisConnectionFactory} to obtain the actual + * {@link RedisConnection}. + *

* {@link DefaultRedisCacheWriter} can be used in - * {@link RedisCacheWriter#lockingRedisCacheWriter(RedisConnectionFactory) locking} or - * {@link RedisCacheWriter#nonLockingRedisCacheWriter(RedisConnectionFactory) non-locking} mode. While - * {@literal non-locking} aims for maximum performance it may result in overlapping, non atomic, command execution for + * {@link RedisCacheWriter#lockingRedisCacheWriter(RedisConnectionFactory) locking} + * or {@link RedisCacheWriter#nonLockingRedisCacheWriter(RedisConnectionFactory) non-locking} mode. While + * {@literal non-locking} aims for maximum performance it may result in overlapping, non-atomic, command execution for * operations spanning multiple Redis interactions like {@code putIfAbsent}. The {@literal locking} counterpart prevents * command overlap by setting an explicit lock key and checking against presence of this key which leads to additional * requests and potential command wait times. @@ -44,16 +45,20 @@ import org.springframework.util.Assert; * @author Christoph Strobl * @author Mark Paluch * @author André Prata + * @author John Blum * @since 2.0 */ class DefaultRedisCacheWriter implements RedisCacheWriter { - private final RedisConnectionFactory connectionFactory; + private final BatchStrategy batchStrategy; + + private final CacheStatisticsCollector statistics; + private final Duration sleepTime; + private final RedisConnectionFactory connectionFactory; + private final TtlFunction lockTtl; - private final CacheStatisticsCollector statistics; - private final BatchStrategy batchStrategy; /** * @param connectionFactory must not be {@literal null}. @@ -166,8 +171,8 @@ class DefaultRedisCacheWriter implements RedisCacheWriter { } return connection.get(key); - } finally { + } finally { if (isLockingCacheWriter()) { doUnlock(name, connection); } @@ -196,21 +201,21 @@ class DefaultRedisCacheWriter implements RedisCacheWriter { boolean wasLocked = false; try { - if (isLockingCacheWriter()) { doLock(name, name, pattern, connection); wasLocked = true; } long deleteCount = batchStrategy.cleanCache(connection, name, pattern); + while (deleteCount > Integer.MAX_VALUE) { statistics.incDeletesBy(name, Integer.MAX_VALUE); deleteCount -= Integer.MAX_VALUE; } + statistics.incDeletesBy(name, (int) deleteCount); } finally { - if (wasLocked && isLockingCacheWriter()) { doUnlock(name, connection); } @@ -280,8 +285,8 @@ class DefaultRedisCacheWriter implements RedisCacheWriter { private T execute(String name, Function callback) { RedisConnection connection = connectionFactory.getConnection(); - try { + try { checkAndPotentiallyWaitUntilUnlocked(name, connection); return callback.apply(connection); } finally { @@ -307,18 +312,19 @@ class DefaultRedisCacheWriter implements RedisCacheWriter { } long lockWaitTimeNs = System.nanoTime(); - try { + try { while (doCheckLock(name, connection)) { Thread.sleep(sleepTime.toMillis()); } - } catch (InterruptedException ex) { + } catch (InterruptedException cause) { // Re-interrupt current thread, to allow other participants to react. Thread.currentThread().interrupt(); - throw new PessimisticLockingFailureException(String.format("Interrupted while waiting to unlock cache %s", name), - ex); + String message = String.format("Interrupted while waiting to unlock cache %s", name); + + throw new PessimisticLockingFailureException(message, cause); } finally { statistics.incLockTime(name, System.nanoTime() - lockWaitTimeNs); } diff --git a/src/main/java/org/springframework/data/redis/cache/SingletonTtlFunction.java b/src/main/java/org/springframework/data/redis/cache/FixedDurationTtlFunction.java similarity index 72% rename from src/main/java/org/springframework/data/redis/cache/SingletonTtlFunction.java rename to src/main/java/org/springframework/data/redis/cache/FixedDurationTtlFunction.java index 267023e83..04c636094 100644 --- a/src/main/java/org/springframework/data/redis/cache/SingletonTtlFunction.java +++ b/src/main/java/org/springframework/data/redis/cache/FixedDurationTtlFunction.java @@ -21,12 +21,16 @@ import org.springframework.data.redis.cache.RedisCacheWriter.TtlFunction; import org.springframework.lang.Nullable; /** - * Singleton implementation of {@link TtlFunction}. + * {@link TtlFunction} implementation returning the given, predetermined {@link Duration} used for per cache entry + * {@literal time-to-live (TTL) expiration}. * * @author Mark Paluch + * @author John Blum + * @see java.time.Duration + * @see org.springframework.data.redis.cache.RedisCacheWriter.TtlFunction * @since 3.2 */ -public record SingletonTtlFunction(Duration duration) implements TtlFunction { +public record FixedDurationTtlFunction(Duration duration) implements TtlFunction { @Override public Duration getTimeToLive(Object key, @Nullable Object value) { diff --git a/src/main/java/org/springframework/data/redis/cache/RedisCache.java b/src/main/java/org/springframework/data/redis/cache/RedisCache.java index fb6e349d7..41982b67b 100644 --- a/src/main/java/org/springframework/data/redis/cache/RedisCache.java +++ b/src/main/java/org/springframework/data/redis/cache/RedisCache.java @@ -67,10 +67,10 @@ public class RedisCache extends AbstractValueAdaptingCache { * Create a new {@link RedisCache}. * * @param name {@link String name} for this {@link Cache}; must not be {@literal null}. - * @param cacheWriter {@link RedisCacheWriter} used to perform {@link RedisCache} operations by executing appropriate - * Redis commands; must not be {@literal null}. - * @param cacheConfiguration {@link RedisCacheConfiguration} applied to this {@link RedisCache on creation; must not - * be {@literal null}. + * @param cacheWriter {@link RedisCacheWriter} used to perform {@link RedisCache} operations by executing + * the necessary Redis commands; must not be {@literal null}. + * @param cacheConfiguration {@link RedisCacheConfiguration} applied to this {@link RedisCache} on creation; + * must not be {@literal null}. * @throws IllegalArgumentException if either the given {@link RedisCacheWriter} or {@link RedisCacheConfiguration} * are {@literal null} or the given {@link String} name for this {@link RedisCache} is {@literal null}. */ @@ -88,18 +88,33 @@ public class RedisCache extends AbstractValueAdaptingCache { } /** - * Get {@link RedisCacheConfiguration} used. + * Get the {@link RedisCacheConfiguration} used to configure this {@link RedisCache} on initialization. * - * @return immutable {@link RedisCacheConfiguration}. Never {@literal null}. + * @return an immutable {@link RedisCacheConfiguration} used to configure this {@link RedisCache} on initialization; + * never {@literal null}. */ public RedisCacheConfiguration getCacheConfiguration() { return this.cacheConfiguration; } + /** + * Gets the configured {@link RedisCacheWriter} used to modify Redis for cache operations. + * + * @return the configured {@link RedisCacheWriter} used to modify Redis for cache operations. + */ protected RedisCacheWriter getCacheWriter() { return this.cacheWriter; } + /** + * Gets the configured {@link ConversionService} used to convert {@link Object cache keys} to a {@link String} + * when accessing entries in the cache. + * + * @return the configured {@link ConversionService} used to convert {@link Object cache keys} to a {@link String} + * when accessing entries in the cache. + * @see RedisCacheConfiguration#getConversionService() + * @see #getCacheConfiguration() + */ protected ConversionService getConversionService() { return getCacheConfiguration().getConversionService(); } @@ -111,12 +126,13 @@ public class RedisCache extends AbstractValueAdaptingCache { @Override public RedisCacheWriter getNativeCache() { - return this.cacheWriter; + return getCacheWriter(); } /** - * Return the {@link CacheStatistics} snapshot for this cache instance. Statistics are accumulated per cache instance - * and not from the backing Redis data store. + * Return the {@link CacheStatistics} snapshot for this cache instance. + *

+ * Statistics are accumulated per cache instance and not from the backing Redis data store. * * @return statistics object for this {@link RedisCache}. * @since 2.4 @@ -134,8 +150,8 @@ public class RedisCache extends AbstractValueAdaptingCache { return result != null ? (T) result.get() : getSynchronized(key, valueLoader); } - @SuppressWarnings("unchecked") @Nullable + @SuppressWarnings("unchecked") private synchronized T getSynchronized(Object key, Callable valueLoader) { ValueWrapper result = get(key); @@ -143,6 +159,16 @@ public class RedisCache extends AbstractValueAdaptingCache { return result != null ? (T) result.get() : loadCacheValue(key, valueLoader); } + /** + * Loads the {@link Object} using the given {@link Callable valueLoader} and {@link #put(Object, Object) puts} + * the {@link Object loaded value} in the cache. + * + * @param {@link Class type} of the loaded {@link Object cache value}. + * @param key {@link Object key} mapped to the loaded {@link Object cache value}. + * @param valueLoader {@link Callable} object used to load the {@link Object value} + * for the given {@link Object key}. + * @return the loaded {@link Object value}. + */ protected T loadCacheValue(Object key, Callable valueLoader) { T value; @@ -172,7 +198,7 @@ public class RedisCache extends AbstractValueAdaptingCache { Object cacheValue = preProcessCacheValue(value); - if (!isAllowNullValues() && cacheValue == null) { + if (nullCacheValueIsNotAllowed(cacheValue)) { String message = String.format("Cache '%s' does not allow 'null' values; Avoid storing null" + " via '@Cacheable(unless=\"#result == null\")' or configure RedisCache to allow 'null'" @@ -182,7 +208,7 @@ public class RedisCache extends AbstractValueAdaptingCache { } getCacheWriter().put(getName(), createAndConvertCacheKey(key), serializeCacheValue(cacheValue), - getCacheConfiguration().getTtl()); + getCacheConfiguration().getTtlFunction().getTimeToLive(key, value)); } @Override @@ -190,12 +216,12 @@ public class RedisCache extends AbstractValueAdaptingCache { Object cacheValue = preProcessCacheValue(value); - if (!isAllowNullValues() && cacheValue == null) { + if (nullCacheValueIsNotAllowed(cacheValue)) { return get(key); } byte[] result = getCacheWriter().putIfAbsent(getName(), createAndConvertCacheKey(key), - serializeCacheValue(cacheValue), getCacheConfiguration().getTtl()); + serializeCacheValue(cacheValue), getCacheConfiguration().getTtlFunction().getTimeToLive(key, value)); return result != null ? new SimpleValueWrapper(fromStoreValue(deserializeCacheValue(result))) : null; } @@ -206,7 +232,7 @@ public class RedisCache extends AbstractValueAdaptingCache { } /** - * Clear keys that match the provided {@link String keyPattern}. + * Clear keys that match the given {@link String keyPattern}. *

* Useful when cache keys are formatted in a style where Redis patterns can be used for matching these. * @@ -303,7 +329,7 @@ public class RedisCache extends AbstractValueAdaptingCache { } /** - * Convert {@code key} to a {@link String} representation used for cache key creation. + * Convert {@code key} to a {@link String} used in cache key creation. * * @param key will never be {@literal null}. * @return never {@literal null}. @@ -338,10 +364,9 @@ public class RedisCache extends AbstractValueAdaptingCache { return key.toString(); } - String message = String.format( - "Cannot convert cache key %s to String; Please register a suitable Converter" + String message = String.format("Cannot convert cache key %s to String; Please register a suitable Converter" + " via 'RedisCacheConfiguration.configureKeyConverters(...)' or override '%s.toString()'", - source, key.getClass().getName()); + source, key.getClass().getName()); throw new IllegalStateException(message); } @@ -403,4 +428,8 @@ public class RedisCache extends AbstractValueAdaptingCache { // allow contextual cache names by computing the key prefix on every call. return getCacheConfiguration().getKeyPrefixFor(getName()) + key; } + + private boolean nullCacheValueIsNotAllowed(@Nullable Object cacheValue) { + return cacheValue == null && !isAllowNullValues(); + } } diff --git a/src/main/java/org/springframework/data/redis/cache/RedisCacheConfiguration.java b/src/main/java/org/springframework/data/redis/cache/RedisCacheConfiguration.java index 5c732f30f..ccacdb566 100644 --- a/src/main/java/org/springframework/data/redis/cache/RedisCacheConfiguration.java +++ b/src/main/java/org/springframework/data/redis/cache/RedisCacheConfiguration.java @@ -32,11 +32,12 @@ import org.springframework.lang.Nullable; import org.springframework.util.Assert; /** - * Immutable {@link RedisCacheConfiguration} used to customize {@link RedisCache} behaviour, such as caching + * Immutable {@link RedisCacheConfiguration} used to customize {@link RedisCache} behavior, such as caching * {@literal null} values, computing cache key prefixes and handling binary serialization. *

- * Start with {@link RedisCacheConfiguration#defaultCacheConfig()} and customize {@link RedisCache} behaviour - * from that point on. + * Start with {@link RedisCacheConfiguration#defaultCacheConfig()} and customize {@link RedisCache} behavior + * using the builder methods, such as {@link #entryTtl(Duration)}, {@link #serializeKeysWith(SerializationPair)} + * and {@link #serializeValuesWith(SerializationPair)}. * * @author Christoph Strobl * @author Mark Paluch @@ -107,10 +108,11 @@ public class RedisCacheConfiguration { registerDefaultConverters(conversionService); - return new RedisCacheConfiguration(Duration.ZERO, DEFAULT_CACHE_NULL_VALUES, DEFAULT_USE_PREFIX, + return new RedisCacheConfiguration(TtlFunction.persistent(), DEFAULT_CACHE_NULL_VALUES, DEFAULT_USE_PREFIX, CacheKeyPrefix.simple(), SerializationPair.fromSerializer(RedisSerializer.string()), - SerializationPair.fromSerializer(RedisSerializer.java(classLoader)), conversionService); + SerializationPair.fromSerializer(RedisSerializer.java(classLoader)), + conversionService); } private final boolean cacheNullValues; @@ -120,25 +122,17 @@ public class RedisCacheConfiguration { private final ConversionService conversionService; - private final TtlFunction ttl; - private final SerializationPair keySerializationPair; private final SerializationPair valueSerializationPair; - private RedisCacheConfiguration(Duration ttl, Boolean cacheNullValues, Boolean usePrefix, CacheKeyPrefix keyPrefix, - SerializationPair keySerializationPair, SerializationPair valueSerializationPair, - ConversionService conversionService) { - - this(TtlFunction.just(ttl), cacheNullValues, usePrefix, keyPrefix, keySerializationPair, valueSerializationPair, - conversionService); - } + private final TtlFunction ttlFunction; @SuppressWarnings("unchecked") - private RedisCacheConfiguration(TtlFunction ttl, Boolean cacheNullValues, Boolean usePrefix, CacheKeyPrefix keyPrefix, - SerializationPair keySerializationPair, SerializationPair valueSerializationPair, - ConversionService conversionService) { + private RedisCacheConfiguration(TtlFunction ttlFunction, Boolean cacheNullValues, Boolean usePrefix, + CacheKeyPrefix keyPrefix, SerializationPair keySerializationPair, + SerializationPair valueSerializationPair, ConversionService conversionService) { - this.ttl = ttl; + this.ttlFunction = ttlFunction; this.cacheNullValues = cacheNullValues; this.usePrefix = usePrefix; this.keyPrefix = keyPrefix; @@ -172,10 +166,10 @@ public class RedisCacheConfiguration { */ public RedisCacheConfiguration computePrefixWith(CacheKeyPrefix cacheKeyPrefix) { - Assert.notNull(cacheKeyPrefix, "Function for computing prefix must not be null"); + Assert.notNull(cacheKeyPrefix, "Function used to compute prefix must not be null"); - return new RedisCacheConfiguration(ttl, cacheNullValues, DEFAULT_USE_PREFIX, cacheKeyPrefix, - keySerializationPair, valueSerializationPair, conversionService); + return new RedisCacheConfiguration(getTtlFunction(), getAllowCacheNullValues(), DEFAULT_USE_PREFIX, + cacheKeyPrefix, getKeySerializationPair(), getValueSerializationPair(), getConversionService()); } /** @@ -187,8 +181,8 @@ public class RedisCacheConfiguration { * @return new {@link RedisCacheConfiguration}. */ public RedisCacheConfiguration disableCachingNullValues() { - return new RedisCacheConfiguration(ttl, DO_NOT_CACHE_NULL_VALUES, usePrefix, keyPrefix, keySerializationPair, - valueSerializationPair, conversionService); + return new RedisCacheConfiguration(getTtlFunction(), DO_NOT_CACHE_NULL_VALUES, usePrefix(), getKeyPrefix(), + getKeySerializationPair(), getValueSerializationPair(), getConversionService()); } /** @@ -199,9 +193,8 @@ public class RedisCacheConfiguration { * @return new {@link RedisCacheConfiguration}. */ public RedisCacheConfiguration disableKeyPrefix() { - - return new RedisCacheConfiguration(ttl, cacheNullValues, DO_NOT_USE_PREFIX, keyPrefix, keySerializationPair, - valueSerializationPair, conversionService); + return new RedisCacheConfiguration(getTtlFunction(), getAllowCacheNullValues(), DO_NOT_USE_PREFIX, + getKeyPrefix(), getKeySerializationPair(), getValueSerializationPair(), getConversionService()); } /** @@ -229,8 +222,8 @@ public class RedisCacheConfiguration { Assert.notNull(ttlFunction, "TtlFunction must not be null"); - return new RedisCacheConfiguration(ttlFunction, cacheNullValues, usePrefix, keyPrefix, keySerializationPair, - valueSerializationPair, conversionService); + return new RedisCacheConfiguration(ttlFunction, getAllowCacheNullValues(), usePrefix(), getKeyPrefix(), + getKeySerializationPair(), getValueSerializationPair(), getConversionService()); } /** @@ -243,8 +236,8 @@ public class RedisCacheConfiguration { Assert.notNull(keySerializationPair, "KeySerializationPair must not be null"); - return new RedisCacheConfiguration(ttl, cacheNullValues, usePrefix, keyPrefix, keySerializationPair, - valueSerializationPair, conversionService); + return new RedisCacheConfiguration(getTtlFunction(), getAllowCacheNullValues(), usePrefix(), getKeyPrefix(), + keySerializationPair, getValueSerializationPair(), getConversionService()); } /** @@ -257,8 +250,8 @@ public class RedisCacheConfiguration { Assert.notNull(valueSerializationPair, "ValueSerializationPair must not be null"); - return new RedisCacheConfiguration(ttl, cacheNullValues, usePrefix, keyPrefix, keySerializationPair, - valueSerializationPair, conversionService); + return new RedisCacheConfiguration(getTtlFunction(), getAllowCacheNullValues(), usePrefix(), getKeyPrefix(), + getKeySerializationPair(), valueSerializationPair, getConversionService()); } /** @@ -271,15 +264,15 @@ public class RedisCacheConfiguration { Assert.notNull(conversionService, "ConversionService must not be null"); - return new RedisCacheConfiguration(ttl, cacheNullValues, usePrefix, keyPrefix, keySerializationPair, - valueSerializationPair, conversionService); + return new RedisCacheConfiguration(getTtlFunction(), getAllowCacheNullValues(), usePrefix(), getKeyPrefix(), + getKeySerializationPair(), getValueSerializationPair(), conversionService); } /** * @return {@literal true} if caching {@literal null} is allowed. */ public boolean getAllowCacheNullValues() { - return cacheNullValues; + return this.cacheNullValues; } /** @@ -287,14 +280,23 @@ public class RedisCacheConfiguration { * the default which resolves to {@link Cache#getName()}. */ public boolean usePrefix() { - return usePrefix; + return this.usePrefix; } /** * @return The {@link ConversionService} used for cache key to {@link String} conversion. Never {@literal null}. */ public ConversionService getConversionService() { - return conversionService; + return this.conversionService; + } + + /** + * Gets the configured {@link CacheKeyPrefix}. + * + * @return the configured {@link CacheKeyPrefix}. + */ + public CacheKeyPrefix getKeyPrefix() { + return this.keyPrefix; } /** @@ -307,32 +309,30 @@ public class RedisCacheConfiguration { Assert.notNull(cacheName, "Cache name must not be null"); - return keyPrefix.compute(cacheName); + return this.keyPrefix.compute(cacheName); } /** * @return never {@literal null}. */ public SerializationPair getKeySerializationPair() { - return keySerializationPair; + return this.keySerializationPair; } /** * @return never {@literal null}. */ public SerializationPair getValueSerializationPair() { - return valueSerializationPair; + return this.valueSerializationPair; } /** - * @return The expiration time (ttl) for cache entries. Never {@literal null}. + * Gets the {@link TtlFunction} used to compute a cache key {@literal time-to-live (TTL) expiration}. + * + * @return the {@link TtlFunction} used to compute expiration time (TTL) for cache entries; never {@literal null}. */ - public Duration getTtl() { - return getTtlFunction().getTimeToLive(null, null); - } - public TtlFunction getTtlFunction() { - return this.ttl; + return this.ttlFunction; } /** diff --git a/src/main/java/org/springframework/data/redis/cache/RedisCacheWriter.java b/src/main/java/org/springframework/data/redis/cache/RedisCacheWriter.java index 6acb1073c..fb1adc6e4 100644 --- a/src/main/java/org/springframework/data/redis/cache/RedisCacheWriter.java +++ b/src/main/java/org/springframework/data/redis/cache/RedisCacheWriter.java @@ -22,16 +22,18 @@ import org.springframework.lang.Nullable; import org.springframework.util.Assert; /** - * {@link RedisCacheWriter} provides low level access to Redis commands ({@code SET, SETNX, GET, EXPIRE,...}) used for - * caching.
- * The {@link RedisCacheWriter} may be shared by multiple cache implementations and is responsible for writing / reading - * binary data to / from Redis. The implementation honors potential cache lock flags that might be set. + * {@link RedisCacheWriter} provides low-level access to Redis commands ({@code SET, SETNX, GET, EXPIRE,...}) used for + * caching. + *

+ * The {@link RedisCacheWriter} may be shared by multiple cache implementations and is responsible for reading/writing + * binary data from/to Redis. The implementation honors potential cache lock flags that might be set. *

* The default {@link RedisCacheWriter} implementation can be customized with {@link BatchStrategy} to tune performance * behavior. * * @author Christoph Strobl * @author Mark Paluch + * @author John Blum * @since 2.0 */ public interface RedisCacheWriter extends CacheStatisticsProvider { @@ -178,6 +180,8 @@ public interface RedisCacheWriter extends CacheStatisticsProvider { @FunctionalInterface interface TtlFunction { + Duration NO_EXPIRATION = Duration.ZERO; + /** * Creates a singleton {@link TtlFunction} using the given {@link Duration}. * @@ -189,7 +193,7 @@ public interface RedisCacheWriter extends CacheStatisticsProvider { Assert.notNull(duration, "TTL Duration must not be null"); - return new SingletonTtlFunction(duration); + return new FixedDurationTtlFunction(duration); } /** @@ -198,19 +202,23 @@ public interface RedisCacheWriter extends CacheStatisticsProvider { * @return a {@link TtlFunction} to create persistent entires that do not expire. */ static TtlFunction persistent() { - return just(Duration.ZERO); + return just(NO_EXPIRATION); } /** - * Compute a {@link Duration time to live duration} using the cache {@code key} and {@code value}. The time to live - * is computed on each write operation. Redis uses milliseconds granularity for timeouts. Any more granular values - * (e.g. micros or nanos) are not considered and are truncated due to rounding. Returning {@link Duration#ZERO} (or - * a value less than {@code Duration.ofMillis(1)}) results in a persistent value that does not expire. + * Compute a {@link Duration time-to-live (TTL)} using the cache {@code key} and {@code value}. + *

+ * The {@link Duration time-to-live (TTL)} is computed on each write operation. Redis uses millisecond + * granularity for timeouts. Any more granular values (e.g. micros or nanos) are not considered + * and will be truncated due to rounding. Returning {@link Duration#ZERO}, or a value less than + * {@code Duration.ofMillis(1)}, results in a persistent value that does not expire. * * @param key the cache key. * @param value the cache value. Can be {@code null} if the cache supports {@code null} value caching. - * @return the time to live. Can be {@link Duration#ZERO} for persistent values (i.e. cache entry does not expire). + * @return the computed {@link Duration time-to-live (TTL)}. Can be {@link Duration#ZERO} for persistent values + * (i.e. cache entry does not expire). */ Duration getTimeToLive(Object key, @Nullable Object value); + } } diff --git a/src/main/java/org/springframework/data/redis/core/types/Expiration.java b/src/main/java/org/springframework/data/redis/core/types/Expiration.java index 8051e8cb4..f21b46f38 100644 --- a/src/main/java/org/springframework/data/redis/core/types/Expiration.java +++ b/src/main/java/org/springframework/data/redis/core/types/Expiration.java @@ -20,117 +20,98 @@ import java.util.concurrent.TimeUnit; import org.springframework.lang.Nullable; import org.springframework.util.Assert; -import org.springframework.util.ObjectUtils; /** - * Expiration holds a value with its associated {@link TimeUnit}. + * {@link Expiration} holds a {@link Long numeric value} with an associated {@link TimeUnit}. * * @author Christoph Strobl * @author Mark Paluch + * @author John Blum + * @see java.time.Duration + * @see java.util.concurrent.TimeUnit * @since 1.7 */ public class Expiration { - private long expirationTime; - private TimeUnit timeUnit; - /** - * Creates new {@link Expiration}. + * Creates a new {@link Expiration} in {@link TimeUnit#MILLISECONDS}. * - * @param expirationTime can be {@literal null}. Defaulted to {@link TimeUnit#SECONDS} - * @param timeUnit - */ - protected Expiration(long expirationTime, @Nullable TimeUnit timeUnit) { - - this.expirationTime = expirationTime; - this.timeUnit = timeUnit != null ? timeUnit : TimeUnit.SECONDS; - } - - /** - * Get the expiration time converted into {@link TimeUnit#MILLISECONDS}. - * - * @return - */ - public long getExpirationTimeInMilliseconds() { - return getConverted(TimeUnit.MILLISECONDS); - } - - /** - * Get the expiration time converted into {@link TimeUnit#SECONDS}. - * - * @return - */ - public long getExpirationTimeInSeconds() { - return getConverted(TimeUnit.SECONDS); - } - - /** - * Get the expiration time. - * - * @return - */ - public long getExpirationTime() { - return expirationTime; - } - - /** - * Get the time unit for the expiration time. - * - * @return - */ - public TimeUnit getTimeUnit() { - return this.timeUnit; - } - - /** - * Get the expiration time converted into the desired {@code targetTimeUnit}. - * - * @param targetTimeUnit must not {@literal null}. - * @return - * @throws IllegalArgumentException - */ - public long getConverted(TimeUnit targetTimeUnit) { - - Assert.notNull(targetTimeUnit, "TargetTimeUnit must not be null"); - return targetTimeUnit.convert(expirationTime, timeUnit); - } - - /** - * Creates new {@link Expiration} with {@link TimeUnit#SECONDS}. - * - * @param expirationTime - * @return - */ - public static Expiration seconds(long expirationTime) { - return new Expiration(expirationTime, TimeUnit.SECONDS); - } - - /** - * Creates new {@link Expiration} with {@link TimeUnit#MILLISECONDS}. - * - * @param expirationTime - * @return + * @param expirationTime {@link Long length of time} for expiration. + * @return a new {@link Expiration} measured in {@link TimeUnit#MILLISECONDS}. */ public static Expiration milliseconds(long expirationTime) { return new Expiration(expirationTime, TimeUnit.MILLISECONDS); } /** - * Creates new {@link Expiration} with the given {@literal unix timestamp} and {@link TimeUnit}. + * Creates a new {@link Expiration} in {@link TimeUnit#SECONDS}. * - * @param unixTimestamp the unix timestamp at which the key will expire. - * @param timeUnit must not be {@literal null}. - * @return new instance of {@link Expiration}. + * @param expirationTime {@link Long length of time} for expiration. + * @return a new {@link Expiration} measured in {@link TimeUnit#SECONDS}. + */ + public static Expiration seconds(long expirationTime) { + return new Expiration(expirationTime, TimeUnit.SECONDS); + } + + /** + * Creates a new {@link Expiration} with the given {@literal unix timestamp} and {@link TimeUnit}. + * + * @param unixTimestamp {@link Long unix timestamp} at which the key will expire. + * @param timeUnit {@link TimeUnit} used to measure the expiration period; must not be {@literal null}. + * @return a new {@link Expiration} with the given {@literal unix timestamp} and {@link TimeUnit}. */ public static Expiration unixTimestamp(long unixTimestamp, TimeUnit timeUnit) { return new ExpireAt(unixTimestamp, timeUnit); } /** - * Obtain an {@link Expiration} that indicates to keep the existing one. Eg. when sending a {@code SET} command. + * Creates new {@link Expiration} with the provided {@link TimeUnit}. Greater units than {@link TimeUnit#SECONDS} are + * converted to {@link TimeUnit#SECONDS}. Units smaller than {@link TimeUnit#MILLISECONDS} are converted to + * {@link TimeUnit#MILLISECONDS} and can lose precision since {@link TimeUnit#MILLISECONDS} is the smallest + * granularity supported by Redis. + * + * @param expirationTime {@link Long length of time} for the {@link Expiration}. + * @param timeUnit {@link TimeUnit} used to measure the {@link Long expiration time}; can be {@literal null}. + * Defaulted to {@link TimeUnit#SECONDS} + * @return a new {@link Expiration} configured with the given {@link Long length of time} in {@link TimeUnit}. + */ + public static Expiration from(long expirationTime, @Nullable TimeUnit timeUnit) { + + if (TimeUnit.NANOSECONDS.equals(timeUnit) + || TimeUnit.MICROSECONDS.equals(timeUnit) + || TimeUnit.MILLISECONDS.equals(timeUnit)) { + + return new Expiration(timeUnit.toMillis(expirationTime), TimeUnit.MILLISECONDS); + } + + return timeUnit != null ? new Expiration(timeUnit.toSeconds(expirationTime), TimeUnit.SECONDS) + : new Expiration(expirationTime, TimeUnit.SECONDS); + } + + /** + * Creates a new {@link Expiration} with the given, required {@link Duration}. *

- * NOTE: Please follow the documentation of the individual commands to see if {@link #keepTtl()} is - * applicable. + * Durations with at least {@literal seconds} resolution uses {@link TimeUnit#SECONDS}. {@link Duration Durations} + * in {@literal milliseconds} use {@link TimeUnit#MILLISECONDS}. + * + * @param duration must not be {@literal null}. + * @return a new {@link Expiration} from the given {@link Duration}. + * @since 2.0 + */ + public static Expiration from(Duration duration) { + + Assert.notNull(duration, "Duration must not be null"); + + return duration.isZero() ? Expiration.persistent() + : duration.toMillis() % 1000 == 0 ? new Expiration(duration.getSeconds(), TimeUnit.SECONDS) + : new Expiration(duration.toMillis(), TimeUnit.MILLISECONDS); + } + + /** + * Obtain an {@link Expiration} that indicates to keep the existing one, e.g. when sending a {@code SET} command. + *

+ * NOTE: Please follow the documentation for the individual commands to see + * if keeping the existing TTL is applicable. * * @return never {@literal null}. * @since 2.4 @@ -140,68 +121,86 @@ public class Expiration { } /** - * Creates new {@link Expiration} with the provided {@link TimeUnit}. Greater units than {@link TimeUnit#SECONDS} are - * converted to {@link TimeUnit#SECONDS}. Units smaller than {@link TimeUnit#MILLISECONDS} are converted to - * {@link TimeUnit#MILLISECONDS} and can lose precision since {@link TimeUnit#MILLISECONDS} is the smallest - * granularity supported by Redis. + * Creates a new persistent, non-expiring {@link Expiration}. * - * @param expirationTime - * @param timeUnit can be {@literal null}. Defaulted to {@link TimeUnit#SECONDS} - * @return - */ - public static Expiration from(long expirationTime, @Nullable TimeUnit timeUnit) { - - if (ObjectUtils.nullSafeEquals(timeUnit, TimeUnit.MICROSECONDS) - || ObjectUtils.nullSafeEquals(timeUnit, TimeUnit.NANOSECONDS) - || ObjectUtils.nullSafeEquals(timeUnit, TimeUnit.MILLISECONDS)) { - return new Expiration(timeUnit.toMillis(expirationTime), TimeUnit.MILLISECONDS); - } - - if (timeUnit != null) { - return new Expiration(timeUnit.toSeconds(expirationTime), TimeUnit.SECONDS); - } - - return new Expiration(expirationTime, TimeUnit.SECONDS); - } - - /** - * Creates new {@link Expiration} with the provided {@link java.time.Duration}. Durations with at least - * {@link TimeUnit#SECONDS} resolution use seconds, durations using milliseconds use {@link TimeUnit#MILLISECONDS} - * resolution. - * - * @param duration must not be {@literal null}. - * @return - * @since 2.0 - */ - public static Expiration from(Duration duration) { - - Assert.notNull(duration, "Duration must not be null"); - - if (duration.isZero()) { - return Expiration.persistent(); - } - - if (duration.toMillis() % 1000 == 0) { - return new Expiration(duration.getSeconds(), TimeUnit.SECONDS); - } - - return new Expiration(duration.toMillis(), TimeUnit.MILLISECONDS); - } - - /** - * Creates new persistent {@link Expiration}. - * - * @return + * @return a new persistent, non-expiring {@link Expiration}. */ public static Expiration persistent() { return new Expiration(-1, TimeUnit.SECONDS); } + private final long expirationTime; + + private final TimeUnit timeUnit; + + /** + * Creates new {@link Expiration}. + * + * @param expirationTime {@link Long length of time} for expiration. Defaulted to {@link TimeUnit#SECONDS}. + * @param timeUnit {@link TimeUnit} used to measure {@link Long expirationTime}. + */ + protected Expiration(long expirationTime, @Nullable TimeUnit timeUnit) { + + this.expirationTime = expirationTime; + this.timeUnit = timeUnit != null ? timeUnit : TimeUnit.SECONDS; + } + + /** + * Get the {@link Long length of time} for this {@link Expiration}. + * + * @return the {@link Long length of time} for this {@link Expiration}. + */ + public long getExpirationTime() { + return this.expirationTime; + } + + /** + * Get the {@link Long expiration time} converted into {@link TimeUnit#MILLISECONDS}. + * + * @return the expiration time converted into {@link TimeUnit#MILLISECONDS}. + */ + public long getExpirationTimeInMilliseconds() { + return getConverted(TimeUnit.MILLISECONDS); + } + + /** + * Get the {@link Long expiration time} converted into {@link TimeUnit#SECONDS}. + * + * @return the {@link Long expiration time} converted into {@link TimeUnit#SECONDS}. + */ + public long getExpirationTimeInSeconds() { + return getConverted(TimeUnit.SECONDS); + } + + /** + * Get the configured {@link TimeUnit} for the {@link #getExpirationTime() expiration time}. + * + * @return the configured {@link TimeUnit} for the {@link #getExpirationTime() expiration time}. + */ + public TimeUnit getTimeUnit() { + return this.timeUnit; + } + + /** + * Converts {@link #getExpirationTime() expiration time} into the given, desired {@link TimeUnit}. + * + * @param targetTimeUnit {@link TimeUnit} used to convert the {@link #getExpirationTime()} expiration time}; + * must not be {@literal null}. + * @return the {@link #getExpirationTime() expiration time} converted into the given, desired {@link TimeUnit}. + * @throws IllegalArgumentException if the given {@link TimeUnit} is {@literal null}. + */ + public long getConverted(TimeUnit targetTimeUnit) { + + Assert.notNull(targetTimeUnit, "TimeUnit must not be null"); + + return targetTimeUnit.convert(getExpirationTime(), getTimeUnit()); + } + /** * @return {@literal true} if {@link Expiration} is set to persistent. */ public boolean isPersistent() { - return expirationTime == -1; + return getExpirationTime() == -1; } /** diff --git a/src/test/java/org/springframework/data/redis/cache/DefaultRedisCacheWriterTests.java b/src/test/java/org/springframework/data/redis/cache/DefaultRedisCacheWriterTests.java index 2c4a47ef3..7ec58b7c0 100644 --- a/src/test/java/org/springframework/data/redis/cache/DefaultRedisCacheWriterTests.java +++ b/src/test/java/org/springframework/data/redis/cache/DefaultRedisCacheWriterTests.java @@ -68,9 +68,10 @@ public class DefaultRedisCacheWriterTests { @BeforeEach void setUp() { - JedisConnectionFactory cf = JedisConnectionFactoryExtension.getConnectionFactory(RedisStanalone.class); + JedisConnectionFactory connectionFactory = + JedisConnectionFactoryExtension.getConnectionFactory(RedisStanalone.class); - connectionFactory = cf; + this.connectionFactory = connectionFactory; doWithConnection(RedisConnection::flushAll); } @@ -80,6 +81,7 @@ public class DefaultRedisCacheWriterTests { RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory) .withStatisticsCollector(CacheStatisticsCollector.create()); + writer.put(CACHE_NAME, binaryCacheKey, binaryCacheValue, Duration.ZERO); doWithConnection(connection -> { @@ -138,9 +140,8 @@ public class DefaultRedisCacheWriterTests { RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory) .withStatisticsCollector(CacheStatisticsCollector.create()); - assertThat(writer.get(CACHE_NAME, binaryCacheKey)) - .isEqualTo(binaryCacheValue); + assertThat(writer.get(CACHE_NAME, binaryCacheKey)).isEqualTo(binaryCacheValue); assertThat(writer.getCacheStatistics(CACHE_NAME).getGets()).isOne(); assertThat(writer.getCacheStatistics(CACHE_NAME).getHits()).isOne(); assertThat(writer.getCacheStatistics(CACHE_NAME).getMisses()).isZero(); @@ -156,8 +157,8 @@ public class DefaultRedisCacheWriterTests { RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory) .withStatisticsCollector(CacheStatisticsCollector.create()); - assertThat(writer.putIfAbsent(CACHE_NAME, binaryCacheKey, binaryCacheValue, - Duration.ZERO)).isNull(); + + assertThat(writer.putIfAbsent(CACHE_NAME, binaryCacheKey, binaryCacheValue, Duration.ZERO)).isNull(); doWithConnection(connection -> { assertThat(connection.get(binaryCacheKey)).isEqualTo(binaryCacheValue); @@ -173,6 +174,7 @@ public class DefaultRedisCacheWriterTests { RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory) .withStatisticsCollector(CacheStatisticsCollector.create()); + assertThat(writer.putIfAbsent(CACHE_NAME, binaryCacheKey, "foo".getBytes(), Duration.ZERO)) .isEqualTo(binaryCacheValue); @@ -188,11 +190,13 @@ public class DefaultRedisCacheWriterTests { RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory) .withStatisticsCollector(CacheStatisticsCollector.create()); + assertThat(writer.putIfAbsent(CACHE_NAME, binaryCacheKey, binaryCacheValue, Duration.ofSeconds(5))).isNull(); doWithConnection(connection -> { assertThat(connection.ttl(binaryCacheKey)).isGreaterThan(3).isLessThan(6); }); + assertThat(writer.getCacheStatistics(CACHE_NAME).getPuts()).isOne(); } @@ -203,9 +207,11 @@ public class DefaultRedisCacheWriterTests { RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory) .withStatisticsCollector(CacheStatisticsCollector.create()); + writer.remove(CACHE_NAME, binaryCacheKey); doWithConnection(connection -> assertThat(connection.exists(binaryCacheKey)).isFalse()); + assertThat(writer.getCacheStatistics(CACHE_NAME).getDeletes()).isOne(); } @@ -219,12 +225,14 @@ public class DefaultRedisCacheWriterTests { RedisCacheWriter writer = nonLockingRedisCacheWriter(connectionFactory) .withStatisticsCollector(CacheStatisticsCollector.create()); + writer.clean(CACHE_NAME, (CACHE_NAME + "::*").getBytes(Charset.forName("UTF-8"))); doWithConnection(connection -> { assertThat(connection.exists(binaryCacheKey)).isFalse(); assertThat(connection.exists("foo".getBytes())).isTrue(); }); + assertThat(writer.getCacheStatistics(CACHE_NAME).getDeletes()).isOne(); } @@ -245,8 +253,8 @@ public class DefaultRedisCacheWriterTests { ((DefaultRedisCacheWriter) lockingRedisCacheWriter(connectionFactory)).lock(CACHE_NAME); - lockingRedisCacheWriter(connectionFactory).put(CACHE_NAME + "-no-the-other-cache", binaryCacheKey, binaryCacheValue, - Duration.ZERO); + lockingRedisCacheWriter(connectionFactory).put(CACHE_NAME + "-no-the-other-cache", binaryCacheKey, + binaryCacheValue, Duration.ZERO); doWithConnection(connection -> { assertThat(connection.exists(binaryCacheKey)).isTrue(); @@ -264,11 +272,11 @@ public class DefaultRedisCacheWriterTests { CountDownLatch afterWrite = new CountDownLatch(1); Thread th = new Thread(() -> { - beforeWrite.countDown(); writer.put(CACHE_NAME, binaryCacheKey, binaryCacheValue, Duration.ZERO); afterWrite.countDown(); }); + th.start(); try { @@ -282,12 +290,14 @@ public class DefaultRedisCacheWriterTests { }); writer.unlock(CACHE_NAME); - afterWrite.await(); + doWithConnection(connection -> { assertThat(connection.exists(binaryCacheKey)).isTrue(); }); + assertThat(writer.getCacheStatistics(CACHE_NAME).getLockWaitDuration(TimeUnit.NANOSECONDS)).isGreaterThan(0); + } finally { th.interrupt(); } @@ -317,8 +327,8 @@ public class DefaultRedisCacheWriterTests { try { writer.put(CACHE_NAME, binaryCacheKey, binaryCacheValue, Duration.ZERO); - } catch (Exception e) { - exceptionRef.set(e); + } catch (Exception cause) { + exceptionRef.set(cause); } finally { afterWrite.countDown(); } @@ -339,8 +349,10 @@ public class DefaultRedisCacheWriterTests { void lockingCacheWriterShouldUsePersistentLocks() { DefaultRedisCacheWriter writer = (DefaultRedisCacheWriter) lockingRedisCacheWriter(connectionFactory, - Duration.ofSeconds(1), TtlFunction.just(Duration.ZERO), BatchStrategies.keys()); + Duration.ofSeconds(1), TtlFunction.persistent(), BatchStrategies.keys()); + writer.lock(CACHE_NAME); + doWithConnection(conn -> { Long ttl = conn.ttl("default-redis-cache-writer-tests~lock".getBytes()); assertThat(ttl).isEqualTo(-1); @@ -352,7 +364,9 @@ public class DefaultRedisCacheWriterTests { DefaultRedisCacheWriter writer = (DefaultRedisCacheWriter) lockingRedisCacheWriter(connectionFactory, Duration.ofSeconds(1), TtlFunction.just(Duration.ofSeconds(60)), BatchStrategies.keys()); + writer.lock(CACHE_NAME); + doWithConnection(conn -> { Long ttl = conn.ttl("default-redis-cache-writer-tests~lock".getBytes()); assertThat(ttl).isGreaterThan(30).isLessThan(70); @@ -373,11 +387,8 @@ public class DefaultRedisCacheWriterTests { private void doWithConnection(Consumer callback) { - RedisConnection connection = connectionFactory.getConnection(); - try { + try (RedisConnection connection = connectionFactory.getConnection()) { callback.accept(connection); - } finally { - connection.close(); } } } diff --git a/src/test/java/org/springframework/data/redis/cache/RedisCacheManagerUnitTests.java b/src/test/java/org/springframework/data/redis/cache/RedisCacheManagerUnitTests.java index 75befe282..59d13ca6e 100644 --- a/src/test/java/org/springframework/data/redis/cache/RedisCacheManagerUnitTests.java +++ b/src/test/java/org/springframework/data/redis/cache/RedisCacheManagerUnitTests.java @@ -215,8 +215,10 @@ class RedisCacheManagerUnitTests { assertThat(customCacheConfiguration).isNotSameAs(defaultCacheConfiguration); assertThat(cacheManagerBuilder.cacheDefaults(customCacheConfiguration)).isSameAs(cacheManagerBuilder); assertThat(cacheManagerBuilder.cacheDefaults().usePrefix()).isFalse(); - assertThat(cacheManagerBuilder.cacheDefaults().getTtl()).isEqualTo(Duration.ofSeconds(10)); + assertThat(cacheManagerBuilder.cacheDefaults().getTtlFunction().getTimeToLive(null, null)) + .isEqualTo(Duration.ofSeconds(10)); assertThat(defaultCacheConfiguration.usePrefix()).isTrue(); - assertThat(defaultCacheConfiguration.getTtl()).isEqualTo(Duration.ofMinutes(30)); + assertThat(defaultCacheConfiguration.getTtlFunction().getTimeToLive(null, null)) + .isEqualTo(Duration.ofMinutes(30)); } }