Polishing.

Remove no longer needed wasLocked flag from CacheWriter.

Original Pull Request: #2948

Author: christophstrobl

src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

@@ -24,6 +24,7 @@
 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 @@ public byte[] get(String name, byte[] key, Supplier<byte[]> valueLoader, @Nullab
 
 		return execute(name, connection -> {
 
-			boolean wasLocked = false;
 			if (isLockingCacheWriter()) {
 				doLock(name, key, null, connection);
-				wasLocked = true;
 			}
 
 			try {
@@ -195,7 +194,7 @@ public byte[] get(String name, byte[] key, Supplier<byte[]> valueLoader, @Nullab
 				doPut(connection, name, key, value, ttl);
 				return value;
 			} finally {
-				if (isLockingCacheWriter() && wasLocked) {
+				if (isLockingCacheWriter()) {
 					doUnlock(name, connection);
 				}
 			}
@@ -274,10 +273,8 @@ public byte[] putIfAbsent(String name, byte[] key, byte[] value, @Nullable Durat
 
 		return execute(name, connection -> {
 
-			boolean wasLocked = false;
 			if (isLockingCacheWriter()) {
 				doLock(name, key, value, connection);
-				wasLocked = true;
 			}
 
 			try {
@@ -299,7 +296,7 @@ public byte[] putIfAbsent(String name, byte[] key, byte[] value, @Nullable Durat
 				return connection.stringCommands().get(key);
 
 			} finally {
-				if (isLockingCacheWriter() && wasLocked) {
+				if (isLockingCacheWriter()) {
 					doUnlock(name, connection);
 				}
 			}
@@ -324,12 +321,9 @@ public void clean(String name, byte[] pattern) {
 
 		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 @@ public void clean(String name, byte[] pattern) {
 				statistics.incDeletesBy(name, (int) deleteCount);
 
 			} finally {
-				if (wasLocked && isLockingCacheWriter()) {
+				if (isLockingCacheWriter()) {
 					doUnlock(name, connection);
 				}
 			}
@@ -373,10 +367,10 @@ public RedisCacheWriter withStatisticsCollector(CacheStatisticsCollector cacheSt
 	 * @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 @@ boolean doLock(String name, Object contextualKey, @Nullable Object contextualVal
 				true)) {
 			checkAndPotentiallyWaitUntilUnlocked(name, connection);
 		}
-
-		return true;
 	}
 
 	/**
@@ -412,6 +404,14 @@ private <T> T execute(String name, Function<RedisConnection, T> callback) {
 		}
 	}
 
+	private void executeWithoutResult(String name, Consumer<RedisConnection> callback) {
+
+		try (RedisConnection connection = this.connectionFactory.getConnection()) {
+			checkAndPotentiallyWaitUntilUnlocked(name, connection);
+			callback.accept(connection);
+		}
+	}
+
 	private <T> T executeLockFree(Function<RedisConnection, T> callback) {
 
 		try (RedisConnection connection = this.connectionFactory.getConnection()) {

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,16 +35,18 @@
 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;
 
 /**
  * Unit tests for {@link DefaultRedisCacheWriter}
  *
  * @author John Blum
+ * @author Christoph Strobl
  */
 @ExtendWith(MockitoExtension.class)
 class DefaultRedisCacheWriterUnitTests {
@@ -109,4 +114,28 @@ void getWithNullTtl() {
 		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());
+	}
 }

src/test/java/org/springframework/data/redis/cache/DefaultRedisCacheWriterTests.java

@@ -453,13 +453,12 @@ void doLockShouldGetLock() throws InterruptedException {
 		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