Skip to content

Commit d715068

Browse files
authored
RedisLockRegistry: Don't expire not acquired lock
Fix race condition, when methods `RedisLockRegistry#expireUnusedOlderThan` and `RedisLockRegistry#obtain` are executed successively. It's possible to delete the lock from `RedisLockRegistry#expireUnusedOlderThan` method, when lock is created but is not acquired (`RedisLock#getLockedAt = 0`) It can lead to the situation, when `RedisLockRegistry#obtain` returns multiple locks with the same redis-key, which shouldn't happen at all. * Skip locks from expiration when their `lockedAt == 0` - new, not acquired yet. **Cherry-pick to `6.0.x` & `5.5.x`**
1 parent 8fbf75f commit d715068

File tree

2 files changed

+71
-2
lines changed

2 files changed

+71
-2
lines changed

spring-integration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java

+6-1
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@
8181
* @author Artem Bilan
8282
* @author Vedran Pavic
8383
* @author Unseok Kim
84+
* @author Anton Gabov
8485
*
8586
* @since 4.0
8687
*
@@ -235,7 +236,11 @@ public void expireUnusedOlderThan(long age) {
235236
this.locks.entrySet()
236237
.removeIf(entry -> {
237238
RedisLock lock = entry.getValue();
238-
return now - lock.getLockedAt() > age && !lock.isAcquiredInThisProcess();
239+
long lockedAt = lock.getLockedAt();
240+
return now - lockedAt > age
241+
// 'lockedAt = 0' means that the lock is still not acquired!
242+
&& lockedAt > 0
243+
&& !lock.isAcquiredInThisProcess();
239244
});
240245
}
241246
}

spring-integration-redis/src/test/java/org/springframework/integration/redis/util/RedisLockRegistryTests.java

+65-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2014-2022 the original author or authors.
2+
* Copyright 2014-2023 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -31,6 +31,7 @@
3131
import java.util.concurrent.TimeUnit;
3232
import java.util.concurrent.atomic.AtomicBoolean;
3333
import java.util.concurrent.atomic.AtomicInteger;
34+
import java.util.concurrent.atomic.AtomicReference;
3435
import java.util.concurrent.locks.Lock;
3536
import java.util.stream.Collectors;
3637
import java.util.stream.IntStream;
@@ -59,6 +60,7 @@
5960
* @author Vedran Pavic
6061
* @author Unseok Kim
6162
* @author Artem Vozhdayenko
63+
* @author Anton Gabov
6264
*
6365
* @since 4.0
6466
*
@@ -811,6 +813,68 @@ void earlyWakeUpTest(RedisLockType testRedisLockType) throws InterruptedExceptio
811813
registry3.destroy();
812814
}
813815

816+
@ParameterizedTest
817+
@EnumSource(RedisLockType.class)
818+
void testTwoThreadsRemoveAndObtainSameLockSimultaneously(RedisLockType testRedisLockType) throws Exception {
819+
final int TEST_CNT = 200;
820+
final long EXPIRATION_TIME_MILLIS = 10000;
821+
final long LOCK_WAIT_TIME_MILLIS = 500;
822+
final String testKey = "testKey";
823+
824+
final RedisLockRegistry registry = new RedisLockRegistry(redisConnectionFactory, this.registryKey);
825+
registry.setRedisLockType(testRedisLockType);
826+
827+
for (int i = 0; i < TEST_CNT; i++) {
828+
final String lockKey = testKey + i;
829+
final CountDownLatch latch = new CountDownLatch(1);
830+
final AtomicReference<Lock> lock1 = new AtomicReference<>();
831+
final AtomicReference<Lock> lock2 = new AtomicReference<>();
832+
833+
Thread thread1 = new Thread(() -> {
834+
try {
835+
latch.await();
836+
// remove lock
837+
registry.expireUnusedOlderThan(EXPIRATION_TIME_MILLIS);
838+
// obtain new lock and try to acquire
839+
Lock lock = registry.obtain(lockKey);
840+
lock.tryLock(LOCK_WAIT_TIME_MILLIS, TimeUnit.MILLISECONDS);
841+
lock.unlock();
842+
843+
lock1.set(lock);
844+
}
845+
catch (InterruptedException ignore) {
846+
}
847+
});
848+
849+
Thread thread2 = new Thread(() -> {
850+
try {
851+
latch.await();
852+
// remove lock
853+
registry.expireUnusedOlderThan(EXPIRATION_TIME_MILLIS);
854+
// obtain new lock and try to acquire
855+
Lock lock = registry.obtain(lockKey);
856+
lock.tryLock(LOCK_WAIT_TIME_MILLIS, TimeUnit.MILLISECONDS);
857+
lock.unlock();
858+
859+
lock2.set(lock);
860+
}
861+
catch (InterruptedException ignore) {
862+
}
863+
});
864+
865+
thread1.start();
866+
thread2.start();
867+
latch.countDown();
868+
thread1.join();
869+
thread2.join();
870+
871+
// locks must be the same!
872+
assertThat(lock1.get()).isEqualTo(lock2.get());
873+
}
874+
875+
registry.destroy();
876+
}
877+
814878
private Long getExpire(RedisLockRegistry registry, String lockKey) {
815879
StringRedisTemplate template = createTemplate();
816880
String registryKey = TestUtils.getPropertyValue(registry, "registryKey", String.class);

0 commit comments

Comments
 (0)