From 0f4585119057805dc4220c9d6be3ca64bca290c0 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 22 Aug 2024 15:09:04 +0200 Subject: [PATCH] Polishing. Remove no longer needed wasLocked flag from CacheWriter. Original Pull Request: #2948 --- .../redis/cache/DefaultRedisCacheWriter.java | 28 ++++++++--------- .../DefaultRedisCachWriterUnitTests.java | 31 ++++++++++++++++++- .../cache/DefaultRedisCacheWriterTests.java | 5 ++- 3 files changed, 46 insertions(+), 18 deletions(-) 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 e7fe73ab6..f82aa50a7 100644 --- a/src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java +++ b/src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java @@ -24,6 +24,7 @@ import java.time.Duration; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; +import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Supplier; @@ -177,10 +178,8 @@ class DefaultRedisCacheWriter implements RedisCacheWriter { return execute(name, connection -> { - boolean wasLocked = false; if (isLockingCacheWriter()) { doLock(name, key, null, connection); - wasLocked = true; } try { @@ -195,7 +194,7 @@ class DefaultRedisCacheWriter implements RedisCacheWriter { doPut(connection, name, key, value, ttl); return value; } finally { - if (isLockingCacheWriter() && wasLocked) { + if (isLockingCacheWriter()) { doUnlock(name, connection); } } @@ -274,10 +273,8 @@ class DefaultRedisCacheWriter implements RedisCacheWriter { return execute(name, connection -> { - boolean wasLocked = false; if (isLockingCacheWriter()) { doLock(name, key, value, connection); - wasLocked = true; } try { @@ -299,7 +296,7 @@ class DefaultRedisCacheWriter implements RedisCacheWriter { return connection.stringCommands().get(key); } finally { - if (isLockingCacheWriter() && wasLocked) { + if (isLockingCacheWriter()) { doUnlock(name, connection); } } @@ -324,12 +321,9 @@ class DefaultRedisCacheWriter implements RedisCacheWriter { execute(name, connection -> { - boolean wasLocked = false; - try { if (isLockingCacheWriter()) { doLock(name, name, pattern, connection); - wasLocked = true; } long deleteCount = batchStrategy.cleanCache(connection, name, pattern); @@ -342,7 +336,7 @@ class DefaultRedisCacheWriter implements RedisCacheWriter { statistics.incDeletesBy(name, (int) deleteCount); } finally { - if (wasLocked && isLockingCacheWriter()) { + if (isLockingCacheWriter()) { doUnlock(name, connection); } } @@ -373,10 +367,10 @@ class DefaultRedisCacheWriter implements RedisCacheWriter { * @param name the name of the cache to lock. */ void lock(String name) { - execute(name, connection -> doLock(name, name, null, connection)); + executeWithoutResult(name, connection -> doLock(name, name, null, connection)); } - boolean doLock(String name, Object contextualKey, @Nullable Object contextualValue, RedisConnection connection) { + void doLock(String name, Object contextualKey, @Nullable Object contextualValue, RedisConnection connection) { RedisStringCommands commands = connection.stringCommands(); Expiration expiration = Expiration.from(this.lockTtl.getTimeToLive(contextualKey, contextualValue)); @@ -386,8 +380,6 @@ class DefaultRedisCacheWriter implements RedisCacheWriter { true)) { checkAndPotentiallyWaitUntilUnlocked(name, connection); } - - return true; } /** @@ -412,6 +404,14 @@ class DefaultRedisCacheWriter implements RedisCacheWriter { } } + private void executeWithoutResult(String name, Consumer callback) { + + try (RedisConnection connection = this.connectionFactory.getConnection()) { + checkAndPotentiallyWaitUntilUnlocked(name, connection); + callback.accept(connection); + } + } + private T executeLockFree(Function callback) { try (RedisConnection connection = this.connectionFactory.getConnection()) { diff --git a/src/test/java/org/springframework/data/redis/cache/DefaultRedisCachWriterUnitTests.java b/src/test/java/org/springframework/data/redis/cache/DefaultRedisCachWriterUnitTests.java index 233333ef8..d05d5bd6b 100644 --- a/src/test/java/org/springframework/data/redis/cache/DefaultRedisCachWriterUnitTests.java +++ b/src/test/java/org/springframework/data/redis/cache/DefaultRedisCachWriterUnitTests.java @@ -16,10 +16,13 @@ package org.springframework.data.redis.cache; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatException; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -32,9 +35,10 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; - +import org.springframework.dao.PessimisticLockingFailureException; import org.springframework.data.redis.connection.RedisConnection; import org.springframework.data.redis.connection.RedisConnectionFactory; +import org.springframework.data.redis.connection.RedisKeyCommands; import org.springframework.data.redis.connection.RedisStringCommands; import org.springframework.data.redis.core.types.Expiration; @@ -42,6 +46,7 @@ import org.springframework.data.redis.core.types.Expiration; * Unit tests for {@link DefaultRedisCacheWriter} * * @author John Blum + * @author Christoph Strobl */ @ExtendWith(MockitoExtension.class) class DefaultRedisCacheWriterUnitTests { @@ -109,4 +114,28 @@ class DefaultRedisCacheWriterUnitTests { verify(this.mockConnection, times(1)).close(); verifyNoMoreInteractions(this.mockConnection, mockStringCommands); } + + @Test // GH-2890 + void mustNotUnlockWhenLockingFails() { + + byte[] key = "TestKey".getBytes(); + byte[] value = "TestValue".getBytes(); + + RedisStringCommands mockStringCommands = mock(RedisStringCommands.class); + RedisKeyCommands mockKeyCommands = mock(RedisKeyCommands.class); + + doReturn(mockStringCommands).when(this.mockConnection).stringCommands(); + doReturn(mockKeyCommands).when(this.mockConnection).keyCommands(); + doThrow(new PessimisticLockingFailureException("you-shall-not-pass")).when(mockStringCommands).set(any(byte[].class), + any(byte[].class), any(), any()); + + RedisCacheWriter cacheWriter = spy( + new DefaultRedisCacheWriter(this.mockConnectionFactory, Duration.ofMillis(10), mock(BatchStrategy.class)) + .withStatisticsCollector(this.mockCacheStatisticsCollector)); + + assertThatException() + .isThrownBy(() -> cacheWriter.get("TestCache", key, () -> value, Duration.ofMillis(10), false)); + + verify(mockKeyCommands, never()).del(any()); + } } 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 e903b24bc..4d6c2cd0f 100644 --- a/src/test/java/org/springframework/data/redis/cache/DefaultRedisCacheWriterTests.java +++ b/src/test/java/org/springframework/data/redis/cache/DefaultRedisCacheWriterTests.java @@ -453,13 +453,12 @@ public class DefaultRedisCacheWriterTests { DefaultRedisCacheWriter cw = new DefaultRedisCacheWriter(connectionFactory, Duration.ofMillis(10), BatchStrategies.keys()) { - boolean doLock(String name, Object contextualKey, @Nullable Object contextualValue, RedisConnection connection) { + void doLock(String name, Object contextualKey, @Nullable Object contextualValue, RedisConnection connection) { - boolean doLock = super.doLock(name, contextualKey, contextualValue, connection); + super.doLock(name, contextualKey, contextualValue, connection); // any concurrent access (aka not waiting until the lock is acquired) will result in a concurrency greater 1 assertThat(concurrency.incrementAndGet()).isOne(); - return doLock; } @Nullable