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`**
This commit is contained in:
@@ -81,6 +81,7 @@ import org.springframework.util.ReflectionUtils;
|
||||
* @author Artem Bilan
|
||||
* @author Vedran Pavic
|
||||
* @author Unseok Kim
|
||||
* @author Anton Gabov
|
||||
*
|
||||
* @since 4.0
|
||||
*
|
||||
@@ -235,7 +236,11 @@ public final class RedisLockRegistry implements ExpirableLockRegistry, Disposabl
|
||||
this.locks.entrySet()
|
||||
.removeIf(entry -> {
|
||||
RedisLock lock = entry.getValue();
|
||||
return now - lock.getLockedAt() > age && !lock.isAcquiredInThisProcess();
|
||||
long lockedAt = lock.getLockedAt();
|
||||
return now - lockedAt > age
|
||||
// 'lockedAt = 0' means that the lock is still not acquired!
|
||||
&& lockedAt > 0
|
||||
&& !lock.isAcquiredInThisProcess();
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
/*
|
||||
* Copyright 2014-2022 the original author or authors.
|
||||
* Copyright 2014-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.
|
||||
@@ -31,6 +31,7 @@ import java.util.concurrent.LinkedBlockingQueue;
|
||||
import java.util.concurrent.TimeUnit;
|
||||
import java.util.concurrent.atomic.AtomicBoolean;
|
||||
import java.util.concurrent.atomic.AtomicInteger;
|
||||
import java.util.concurrent.atomic.AtomicReference;
|
||||
import java.util.concurrent.locks.Lock;
|
||||
import java.util.stream.Collectors;
|
||||
import java.util.stream.IntStream;
|
||||
@@ -59,6 +60,7 @@ import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
|
||||
* @author Vedran Pavic
|
||||
* @author Unseok Kim
|
||||
* @author Artem Vozhdayenko
|
||||
* @author Anton Gabov
|
||||
*
|
||||
* @since 4.0
|
||||
*
|
||||
@@ -811,6 +813,68 @@ class RedisLockRegistryTests implements RedisContainerTest {
|
||||
registry3.destroy();
|
||||
}
|
||||
|
||||
@ParameterizedTest
|
||||
@EnumSource(RedisLockType.class)
|
||||
void testTwoThreadsRemoveAndObtainSameLockSimultaneously(RedisLockType testRedisLockType) throws Exception {
|
||||
final int TEST_CNT = 200;
|
||||
final long EXPIRATION_TIME_MILLIS = 10000;
|
||||
final long LOCK_WAIT_TIME_MILLIS = 500;
|
||||
final String testKey = "testKey";
|
||||
|
||||
final RedisLockRegistry registry = new RedisLockRegistry(redisConnectionFactory, this.registryKey);
|
||||
registry.setRedisLockType(testRedisLockType);
|
||||
|
||||
for (int i = 0; i < TEST_CNT; i++) {
|
||||
final String lockKey = testKey + i;
|
||||
final CountDownLatch latch = new CountDownLatch(1);
|
||||
final AtomicReference<Lock> lock1 = new AtomicReference<>();
|
||||
final AtomicReference<Lock> lock2 = new AtomicReference<>();
|
||||
|
||||
Thread thread1 = new Thread(() -> {
|
||||
try {
|
||||
latch.await();
|
||||
// remove lock
|
||||
registry.expireUnusedOlderThan(EXPIRATION_TIME_MILLIS);
|
||||
// obtain new lock and try to acquire
|
||||
Lock lock = registry.obtain(lockKey);
|
||||
lock.tryLock(LOCK_WAIT_TIME_MILLIS, TimeUnit.MILLISECONDS);
|
||||
lock.unlock();
|
||||
|
||||
lock1.set(lock);
|
||||
}
|
||||
catch (InterruptedException ignore) {
|
||||
}
|
||||
});
|
||||
|
||||
Thread thread2 = new Thread(() -> {
|
||||
try {
|
||||
latch.await();
|
||||
// remove lock
|
||||
registry.expireUnusedOlderThan(EXPIRATION_TIME_MILLIS);
|
||||
// obtain new lock and try to acquire
|
||||
Lock lock = registry.obtain(lockKey);
|
||||
lock.tryLock(LOCK_WAIT_TIME_MILLIS, TimeUnit.MILLISECONDS);
|
||||
lock.unlock();
|
||||
|
||||
lock2.set(lock);
|
||||
}
|
||||
catch (InterruptedException ignore) {
|
||||
}
|
||||
});
|
||||
|
||||
thread1.start();
|
||||
thread2.start();
|
||||
latch.countDown();
|
||||
thread1.join();
|
||||
thread2.join();
|
||||
|
||||
// locks must be the same!
|
||||
assertThat(lock1.get()).isEqualTo(lock2.get());
|
||||
}
|
||||
|
||||
registry.destroy();
|
||||
}
|
||||
|
||||
private Long getExpire(RedisLockRegistry registry, String lockKey) {
|
||||
StringRedisTemplate template = createTemplate();
|
||||
String registryKey = TestUtils.getPropertyValue(registry, "registryKey", String.class);
|
||||
|
||||
Reference in New Issue
Block a user