diff --git a/pom.xml b/pom.xml index 384157aed5..01f6672f43 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-redis - 3.2.0-SNAPSHOT + 3.2.0-GH-2300-SNAPSHOT Spring Data Redis Spring Data module for Redis 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 5b22ffaf4c..c4656a1640 100644 --- a/src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java +++ b/src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java @@ -50,6 +50,8 @@ class DefaultRedisCacheWriter implements RedisCacheWriter { private final RedisConnectionFactory connectionFactory; private final Duration sleepTime; + + private final TtlFunction lockTtl; private final CacheStatisticsCollector statistics; private final BatchStrategy batchStrategy; @@ -68,26 +70,29 @@ class DefaultRedisCacheWriter implements RedisCacheWriter { * @param batchStrategy must not be {@literal null}. */ DefaultRedisCacheWriter(RedisConnectionFactory connectionFactory, Duration sleepTime, BatchStrategy batchStrategy) { - this(connectionFactory, sleepTime, CacheStatisticsCollector.none(), batchStrategy); + this(connectionFactory, sleepTime, TtlFunction.persistent(), CacheStatisticsCollector.none(), batchStrategy); } /** * @param connectionFactory must not be {@literal null}. * @param sleepTime sleep time between lock request attempts. Must not be {@literal null}. Use {@link Duration#ZERO} * to disable locking. + * @param lockTtl Lock TTL function must not be {@literal null}. * @param cacheStatisticsCollector must not be {@literal null}. * @param batchStrategy must not be {@literal null}. */ - DefaultRedisCacheWriter(RedisConnectionFactory connectionFactory, Duration sleepTime, + DefaultRedisCacheWriter(RedisConnectionFactory connectionFactory, Duration sleepTime, TtlFunction lockTtl, CacheStatisticsCollector cacheStatisticsCollector, BatchStrategy batchStrategy) { Assert.notNull(connectionFactory, "ConnectionFactory must not be null"); Assert.notNull(sleepTime, "SleepTime must not be null"); + Assert.notNull(lockTtl, "Lock TTL Function must not be null"); Assert.notNull(cacheStatisticsCollector, "CacheStatisticsCollector must not be null"); Assert.notNull(batchStrategy, "BatchStrategy must not be null"); this.connectionFactory = connectionFactory; this.sleepTime = sleepTime; + this.lockTtl = lockTtl; this.statistics = cacheStatisticsCollector; this.batchStrategy = batchStrategy; } @@ -142,7 +147,7 @@ public byte[] putIfAbsent(String name, byte[] key, byte[] value, @Nullable Durat return execute(name, connection -> { if (isLockingCacheWriter()) { - doLock(name, connection); + doLock(name, key, value, connection); } try { @@ -193,7 +198,7 @@ public void clean(String name, byte[] pattern) { try { if (isLockingCacheWriter()) { - doLock(name, connection); + doLock(name, name, pattern, connection); wasLocked = true; } @@ -227,7 +232,8 @@ public void clearStatistics(String name) { @Override public RedisCacheWriter withStatisticsCollector(CacheStatisticsCollector cacheStatisticsCollector) { - return new DefaultRedisCacheWriter(connectionFactory, sleepTime, cacheStatisticsCollector, this.batchStrategy); + return new DefaultRedisCacheWriter(connectionFactory, sleepTime, lockTtl, cacheStatisticsCollector, + this.batchStrategy); } /** @@ -236,7 +242,7 @@ public RedisCacheWriter withStatisticsCollector(CacheStatisticsCollector cacheSt * @param name the name of the cache to lock. */ void lock(String name) { - execute(name, connection -> doLock(name, connection)); + execute(name, connection -> doLock(name, name, null, connection)); } /** @@ -248,8 +254,12 @@ void unlock(String name) { executeLockFree(connection -> doUnlock(name, connection)); } - private Boolean doLock(String name, RedisConnection connection) { - return connection.setNX(createCacheLockKey(name), new byte[0]); + private Boolean doLock(String name, Object contextualKey, Object contextualValue, RedisConnection connection) { + + Expiration expiration = lockTtl == null ? Expiration.persistent() + : Expiration.from(lockTtl.getTimeToLive(contextualKey, contextualValue)); + + return connection.set(createCacheLockKey(name), new byte[0], expiration, SetOption.SET_IF_ABSENT); } private Long doUnlock(String name, RedisConnection connection) { 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 1e5d842a9b..4a2f7077fc 100644 --- a/src/main/java/org/springframework/data/redis/cache/RedisCache.java +++ b/src/main/java/org/springframework/data/redis/cache/RedisCache.java @@ -31,7 +31,7 @@ import org.springframework.core.convert.ConversionFailedException; import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.TypeDescriptor; -import org.springframework.data.redis.cache.RedisCacheConfiguration.TtlFunction; +import org.springframework.data.redis.cache.RedisCacheWriter.TtlFunction; import org.springframework.data.redis.serializer.RedisSerializationContext; import org.springframework.data.redis.serializer.RedisSerializer; import org.springframework.data.redis.util.ByteUtils; @@ -70,12 +70,12 @@ 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 appropriate + * 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}. + * are {@literal null} or the given {@link String} name for this {@link RedisCache} is {@literal null}. */ protected RedisCache(String name, RedisCacheWriter cacheWriter, RedisCacheConfiguration cacheConfiguration) { @@ -91,7 +91,6 @@ protected RedisCache(String name, RedisCacheWriter cacheWriter, RedisCacheConfig this.ttlFunction = cacheConfiguration.getTtlFunction(); } - /** * Get {@link RedisCacheConfiguration} used. * @@ -136,8 +135,7 @@ public T get(Object key, Callable valueLoader) { ValueWrapper result = get(key); - return result != null ? (T) result.get() - : getSynchronized(key, valueLoader); + return result != null ? (T) result.get() : getSynchronized(key, valueLoader); } @SuppressWarnings("unchecked") @@ -146,8 +144,7 @@ private synchronized T getSynchronized(Object key, Callable valueLoader) ValueWrapper result = get(key); - return result != null ? (T) result.get() - : loadCacheValue(key, valueLoader); + return result != null ? (T) result.get() : loadCacheValue(key, valueLoader); } protected T loadCacheValue(Object key, Callable valueLoader) { @@ -182,8 +179,7 @@ public void put(Object key, @Nullable Object value) { 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'" - + " via RedisCacheConfiguration", - getName()); + + " via RedisCacheConfiguration", getName()); throw new IllegalArgumentException(message); } @@ -248,9 +244,7 @@ public void evict(Object key) { @Nullable protected Object preProcessCacheValue(@Nullable Object value) { - return value != null ? value - : isAllowNullValues() ? NullValue.INSTANCE - : null; + return value != null ? value : isAllowNullValues() ? NullValue.INSTANCE : null; } /** @@ -285,7 +279,7 @@ protected byte[] serializeCacheValue(Object value) { * * @param value array of bytes to deserialize; must not be {@literal null}. * @return an {@link Object} deserialized from the array of bytes using the configured value - * {@link RedisSerializationContext.SerializationPair}; can be {@literal null}. + * {@link RedisSerializationContext.SerializationPair}; can be {@literal null}. * @see RedisCacheConfiguration#getValueSerializationPair() */ @Nullable @@ -346,9 +340,10 @@ protected String convertKey(Object key) { return key.toString(); } - 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()); + 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()); throw new IllegalStateException(message); } 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 b68cba3385..14d8fd58ab 100644 --- a/src/main/java/org/springframework/data/redis/cache/RedisCacheConfiguration.java +++ b/src/main/java/org/springframework/data/redis/cache/RedisCacheConfiguration.java @@ -24,6 +24,7 @@ import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.converter.Converter; import org.springframework.core.convert.converter.ConverterRegistry; +import org.springframework.data.redis.cache.RedisCacheWriter.TtlFunction; import org.springframework.data.redis.serializer.RedisSerializationContext.SerializationPair; import org.springframework.data.redis.serializer.RedisSerializer; import org.springframework.format.support.DefaultFormattingConversionService; @@ -205,7 +206,7 @@ public RedisCacheConfiguration entryTtl(Duration ttl) { Assert.notNull(ttl, "TTL duration must not be null"); - return entryTtl(new SingletonTtlFunction(ttl)); + return entryTtl(TtlFunction.just(ttl)); } /** @@ -391,33 +392,4 @@ public static void registerDefaultConverters(ConverterRegistry registry) { registry.addConverter(SimpleKey.class, String.class, SimpleKey::toString); } - /** - * Function to compute the time to live from the cache {@code key} and {@code value}. - * - * @author Mark Paluch - * @since 3.2 - */ - @FunctionalInterface - interface TtlFunction { - - /** - * 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. - * - * @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). - */ - Duration getTimeToLive(Object key, @Nullable Object value); - } - - private record SingletonTtlFunction(Duration duration) implements TtlFunction { - - @Override - public Duration getTimeToLive(Object key, @Nullable Object value) { - return this.duration; - } - } } 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 977708657b..6acb1073c3 100644 --- a/src/main/java/org/springframework/data/redis/cache/RedisCacheWriter.java +++ b/src/main/java/org/springframework/data/redis/cache/RedisCacheWriter.java @@ -83,14 +83,31 @@ static RedisCacheWriter lockingRedisCacheWriter(RedisConnectionFactory connectio */ static RedisCacheWriter lockingRedisCacheWriter(RedisConnectionFactory connectionFactory, BatchStrategy batchStrategy) { + return lockingRedisCacheWriter(connectionFactory, Duration.ofMillis(50), TtlFunction.persistent(), batchStrategy); + } + + /** + * Create new {@link RedisCacheWriter} with locking behavior. + * + * @param connectionFactory must not be {@literal null}. + * @param sleepTime sleep time between lock access attempts, must not be {@literal null}. + * @param lockTtlFunction TTL function to compute the Lock TTL. The function is called with contextual keys and values + * (such as the cache name on cleanup or the actual key/value on put requests). Must not be {@literal null}. + * @param batchStrategy must not be {@literal null}. + * @return new instance of {@link DefaultRedisCacheWriter}. + * @since 3.2 + */ + static RedisCacheWriter lockingRedisCacheWriter(RedisConnectionFactory connectionFactory, Duration sleepTime, + TtlFunction lockTtlFunction, BatchStrategy batchStrategy) { Assert.notNull(connectionFactory, "ConnectionFactory must not be null"); - return new DefaultRedisCacheWriter(connectionFactory, Duration.ofMillis(50), batchStrategy); + return new DefaultRedisCacheWriter(connectionFactory, sleepTime, lockTtlFunction, CacheStatisticsCollector.none(), + batchStrategy); } /** - * Write the given key/value pair to Redis an set the expiration time if defined. + * Write the given key/value pair to Redis and set the expiration time if defined. * * @param name The cache name must not be {@literal null}. * @param key The key for the cache entry. Must not be {@literal null}. @@ -152,4 +169,48 @@ static RedisCacheWriter lockingRedisCacheWriter(RedisConnectionFactory connectio */ RedisCacheWriter withStatisticsCollector(CacheStatisticsCollector cacheStatisticsCollector); + /** + * Function to compute the time to live from the cache {@code key} and {@code value}. + * + * @author Mark Paluch + * @since 3.2 + */ + @FunctionalInterface + interface TtlFunction { + + /** + * Creates a singleton {@link TtlFunction} using the given {@link Duration}. + * + * @param duration the time to live. Can be {@link Duration#ZERO} for persistent values (i.e. cache entry does not + * expire). + * @return a singleton {@link TtlFunction} using {@link Duration}. + */ + static TtlFunction just(Duration duration) { + + Assert.notNull(duration, "TTL Duration must not be null"); + + return new SingletonTtlFunction(duration); + } + + /** + * Returns a {@link TtlFunction} to create persistent entires that do not expire. + * + * @return a {@link TtlFunction} to create persistent entires that do not expire. + */ + static TtlFunction persistent() { + return just(Duration.ZERO); + } + + /** + * 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. + * + * @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). + */ + Duration getTimeToLive(Object key, @Nullable Object value); + } } diff --git a/src/main/java/org/springframework/data/redis/cache/SingletonTtlFunction.java b/src/main/java/org/springframework/data/redis/cache/SingletonTtlFunction.java new file mode 100644 index 0000000000..267023e83e --- /dev/null +++ b/src/main/java/org/springframework/data/redis/cache/SingletonTtlFunction.java @@ -0,0 +1,35 @@ +/* + * Copyright 2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.redis.cache; + +import java.time.Duration; + +import org.springframework.data.redis.cache.RedisCacheWriter.TtlFunction; +import org.springframework.lang.Nullable; + +/** + * Singleton implementation of {@link TtlFunction}. + * + * @author Mark Paluch + * @since 3.2 + */ +public record SingletonTtlFunction(Duration duration) implements TtlFunction { + + @Override + public Duration getTimeToLive(Object key, @Nullable Object value) { + return this.duration; + } +} 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 1ac878217a..8051e8cb48 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 @@ -177,6 +177,10 @@ 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); } 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 fdf7f0002e..2c4a47ef38 100644 --- a/src/test/java/org/springframework/data/redis/cache/DefaultRedisCacheWriterTests.java +++ b/src/test/java/org/springframework/data/redis/cache/DefaultRedisCacheWriterTests.java @@ -28,7 +28,6 @@ import java.util.function.Consumer; import org.junit.jupiter.api.BeforeEach; - import org.springframework.data.redis.connection.RedisConnection; import org.springframework.data.redis.connection.RedisConnectionFactory; import org.springframework.data.redis.connection.RedisStringCommands.SetOption; @@ -336,8 +335,32 @@ boolean doCheckLock(String name, RedisConnection connection) { .hasCauseInstanceOf(InterruptedException.class); } + @ParameterizedRedisTest // GH-2300 + void lockingCacheWriterShouldUsePersistentLocks() { + + DefaultRedisCacheWriter writer = (DefaultRedisCacheWriter) lockingRedisCacheWriter(connectionFactory, + Duration.ofSeconds(1), TtlFunction.just(Duration.ZERO), BatchStrategies.keys()); + writer.lock(CACHE_NAME); + doWithConnection(conn -> { + Long ttl = conn.ttl("default-redis-cache-writer-tests~lock".getBytes()); + assertThat(ttl).isEqualTo(-1); + }); + } + + @ParameterizedRedisTest // GH-2300 + void lockingCacheWriterShouldApplyLockTtl() { + + 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); + }); + } + @ParameterizedRedisTest // DATAREDIS-1082 - void noOpSatisticsCollectorReturnsEmptyStatsInstance() { + void noOpStatisticsCollectorReturnsEmptyStatsInstance() { DefaultRedisCacheWriter cw = (DefaultRedisCacheWriter) lockingRedisCacheWriter(connectionFactory); CacheStatistics stats = cw.getCacheStatistics(CACHE_NAME);