From c5f29fc6ef4519f0d2f211dede5045154a3c300e Mon Sep 17 00:00:00 2001 From: Artem Bilan Date: Fri, 14 Dec 2018 11:38:38 -0500 Subject: [PATCH] RedisLock: Throw exception from unlock on expire (#2661) * RedisLock: Throw exception from unlock on expire The lock might be expired in target Redis store in between `lock()` and `unlock()`. So, throw an `IllegalStateException` when lock is expired during unlocking. At the same time the lock lock is unlocked anyway. **Cherry-pick to 5.0.x** * * Create a new test for exception * Add more info into the exception --- .../redis/util/RedisLockRegistry.java | 48 ++++++----- .../redis/util/RedisLockRegistryTests.java | 81 +++++++++++-------- 2 files changed, 74 insertions(+), 55 deletions(-) diff --git a/spring-integration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java b/spring-integration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java index d2e2c4aabe..c636ed9ae9 100644 --- a/spring-integration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java +++ b/spring-integration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java @@ -92,6 +92,18 @@ public final class RedisLockRegistry implements ExpirableLockRegistry, Disposabl "return false"; + private final Map locks = new ConcurrentHashMap<>(); + + private final String clientId = UUID.randomUUID().toString(); + + private final String registryKey; + + private final StringRedisTemplate redisTemplate; + + private final RedisScript obtainLockScript; + + private final long expireAfter; + /** * An {@link ExecutorService} to call {@link StringRedisTemplate#delete(Object)} in * the separate thread when the current one is interrupted. @@ -105,18 +117,6 @@ public final class RedisLockRegistry implements ExpirableLockRegistry, Disposabl */ private boolean executorExplicitlySet; - private final Map locks = new ConcurrentHashMap<>(); - - private final String clientId = UUID.randomUUID().toString(); - - private final String registryKey; - - private final StringRedisTemplate redisTemplate; - - private final RedisScript obtainLockScript; - - private final long expireAfter; - /** * Constructs a lock registry with the default (60 second) lock expiration. * @param connectionFactory The connection factory. @@ -282,13 +282,17 @@ public final class RedisLockRegistry implements ExpirableLockRegistry, Disposabl } private boolean obtainLock() { - boolean success = RedisLockRegistry.this.redisTemplate.execute(RedisLockRegistry.this.obtainLockScript, - Collections.singletonList(this.lockKey), RedisLockRegistry.this.clientId, - String.valueOf(RedisLockRegistry.this.expireAfter)); - if (success) { + Boolean success = + RedisLockRegistry.this.redisTemplate.execute(RedisLockRegistry.this.obtainLockScript, + Collections.singletonList(this.lockKey), RedisLockRegistry.this.clientId, + String.valueOf(RedisLockRegistry.this.expireAfter)); + + boolean result = Boolean.TRUE.equals(success); + + if (result) { this.lockedAt = System.currentTimeMillis(); } - return success; + return result; } @Override @@ -301,6 +305,11 @@ public final class RedisLockRegistry implements ExpirableLockRegistry, Disposabl return; } try { + if (!isAcquiredInThisProcess()) { + throw new IllegalStateException("Lock was released in the store due to expiration. " + + "The integrity of data protected by this lock may have been compromised."); + } + if (Thread.currentThread().isInterrupted()) { RedisLockRegistry.this.executor.execute(this::removeLockKey); } @@ -377,10 +386,7 @@ public final class RedisLockRegistry implements ExpirableLockRegistry, Disposabl if (!this.lockKey.equals(other.lockKey)) { return false; } - if (this.lockedAt != other.lockedAt) { - return false; - } - return true; + return this.lockedAt == other.lockedAt; } private RedisLockRegistry getOuterType() { diff --git a/spring-integration-redis/src/test/java/org/springframework/integration/redis/util/RedisLockRegistryTests.java b/spring-integration-redis/src/test/java/org/springframework/integration/redis/util/RedisLockRegistryTests.java index 709db1d5fc..fb775bd9b4 100644 --- a/spring-integration-redis/src/test/java/org/springframework/integration/redis/util/RedisLockRegistryTests.java +++ b/spring-integration-redis/src/test/java/org/springframework/integration/redis/util/RedisLockRegistryTests.java @@ -78,13 +78,13 @@ public class RedisLockRegistryTests extends RedisAvailableTests { } private StringRedisTemplate createTemplate() { - return new StringRedisTemplate(this.getConnectionFactoryForTest()); + return new StringRedisTemplate(getConnectionFactoryForTest()); } @Test @RedisAvailable - public void testLock() throws Exception { - RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey); + public void testLock() { + RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey); for (int i = 0; i < 10; i++) { Lock lock = registry.obtain("foo"); lock.lock(); @@ -102,7 +102,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { @Test @RedisAvailable public void testLockInterruptibly() throws Exception { - RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey); + RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey); for (int i = 0; i < 10; i++) { Lock lock = registry.obtain("foo"); lock.lockInterruptibly(); @@ -119,8 +119,8 @@ public class RedisLockRegistryTests extends RedisAvailableTests { @Test @RedisAvailable - public void testReentrantLock() throws Exception { - RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey); + public void testReentrantLock() { + RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey); for (int i = 0; i < 10; i++) { Lock lock1 = registry.obtain("foo"); lock1.lock(); @@ -146,7 +146,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { @Test @RedisAvailable public void testReentrantLockInterruptibly() throws Exception { - RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey); + RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey); for (int i = 0; i < 10; i++) { Lock lock1 = registry.obtain("foo"); lock1.lockInterruptibly(); @@ -172,7 +172,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { @Test @RedisAvailable public void testTwoLocks() throws Exception { - RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey); + RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey); for (int i = 0; i < 10; i++) { Lock lock1 = registry.obtain("foo"); lock1.lockInterruptibly(); @@ -198,7 +198,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { @Test @RedisAvailable public void testTwoThreadsSecondFailsToGetLock() throws Exception { - final RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey); + final RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey); final Lock lock1 = registry.obtain("foo"); lock1.lockInterruptibly(); final AtomicBoolean locked = new AtomicBoolean(); @@ -228,12 +228,12 @@ public class RedisLockRegistryTests extends RedisAvailableTests { @Test @RedisAvailable public void testTwoThreads() throws Exception { - final RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey); - final Lock lock1 = registry.obtain("foo"); - final AtomicBoolean locked = new AtomicBoolean(); - final CountDownLatch latch1 = new CountDownLatch(1); - final CountDownLatch latch2 = new CountDownLatch(1); - final CountDownLatch latch3 = new CountDownLatch(1); + RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey); + Lock lock1 = registry.obtain("foo"); + AtomicBoolean locked = new AtomicBoolean(); + CountDownLatch latch1 = new CountDownLatch(1); + CountDownLatch latch2 = new CountDownLatch(1); + CountDownLatch latch3 = new CountDownLatch(1); lock1.lockInterruptibly(); assertEquals(1, TestUtils.getPropertyValue(registry, "locks", Map.class).size()); Executors.newSingleThreadExecutor().execute(() -> { @@ -266,13 +266,13 @@ public class RedisLockRegistryTests extends RedisAvailableTests { @Test @RedisAvailable public void testTwoThreadsDifferentRegistries() throws Exception { - final RedisLockRegistry registry1 = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey); - final RedisLockRegistry registry2 = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey); - final Lock lock1 = registry1.obtain("foo"); - final AtomicBoolean locked = new AtomicBoolean(); - final CountDownLatch latch1 = new CountDownLatch(1); - final CountDownLatch latch2 = new CountDownLatch(1); - final CountDownLatch latch3 = new CountDownLatch(1); + RedisLockRegistry registry1 = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey); + RedisLockRegistry registry2 = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey); + Lock lock1 = registry1.obtain("foo"); + AtomicBoolean locked = new AtomicBoolean(); + CountDownLatch latch1 = new CountDownLatch(1); + CountDownLatch latch2 = new CountDownLatch(1); + CountDownLatch latch3 = new CountDownLatch(1); lock1.lockInterruptibly(); assertEquals(1, TestUtils.getPropertyValue(registry1, "locks", Map.class).size()); Executors.newSingleThreadExecutor().execute(() -> { @@ -313,11 +313,11 @@ public class RedisLockRegistryTests extends RedisAvailableTests { @Test @RedisAvailable public void testTwoThreadsWrongOneUnlocks() throws Exception { - final RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey); - final Lock lock = registry.obtain("foo"); + RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey); + Lock lock = registry.obtain("foo"); lock.lockInterruptibly(); - final AtomicBoolean locked = new AtomicBoolean(); - final CountDownLatch latch = new CountDownLatch(1); + AtomicBoolean locked = new AtomicBoolean(); + CountDownLatch latch = new CountDownLatch(1); Future result = Executors.newSingleThreadExecutor().submit(() -> { try { lock.unlock(); @@ -341,8 +341,8 @@ public class RedisLockRegistryTests extends RedisAvailableTests { @Test @RedisAvailable public void testExpireTwoRegistries() throws Exception { - RedisLockRegistry registry1 = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey, 100); - RedisLockRegistry registry2 = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey, 100); + RedisLockRegistry registry1 = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey, 1); + RedisLockRegistry registry2 = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey, 1); Lock lock1 = registry1.obtain("foo"); Lock lock2 = registry2.obtain("foo"); assertTrue(lock1.tryLock()); @@ -354,8 +354,21 @@ public class RedisLockRegistryTests extends RedisAvailableTests { @Test @RedisAvailable - public void testEquals() throws Exception { - RedisConnectionFactory connectionFactory = this.getConnectionFactoryForTest(); + public void testExceptionOnExpire() throws Exception { + RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey, 1); + Lock lock1 = registry.obtain("foo"); + assertTrue(lock1.tryLock()); + this.thrown.expect(IllegalStateException.class); + this.thrown.expectMessage("Lock was released in the store due to expiration."); + waitForExpire("foo"); + lock1.unlock(); + } + + + @Test + @RedisAvailable + public void testEquals() { + RedisConnectionFactory connectionFactory = getConnectionFactoryForTest(); RedisLockRegistry registry1 = new RedisLockRegistry(connectionFactory, this.registryKey); RedisLockRegistry registry2 = new RedisLockRegistry(connectionFactory, this.registryKey); RedisLockRegistry registry3 = new RedisLockRegistry(connectionFactory, this.registryKey2); @@ -388,7 +401,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { @Test @RedisAvailable public void testThreadLocalListLeaks() { - RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey, 100); + RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey, 100); for (int i = 0; i < 10; i++) { registry.obtain("foo" + i); @@ -411,7 +424,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { @Test @RedisAvailable public void testExpireNotChanged() throws Exception { - RedisConnectionFactory connectionFactory = this.getConnectionFactoryForTest(); + RedisConnectionFactory connectionFactory = getConnectionFactoryForTest(); final RedisLockRegistry registry = new RedisLockRegistry(connectionFactory, this.registryKey, 10000); Lock lock = registry.obtain("foo"); lock.lock(); @@ -429,13 +442,13 @@ public class RedisLockRegistryTests extends RedisAvailableTests { } private Long getExpire(RedisLockRegistry registry, String lockKey) { - StringRedisTemplate template = this.createTemplate(); + StringRedisTemplate template = createTemplate(); String registryKey = TestUtils.getPropertyValue(registry, "registryKey", String.class); return template.getExpire(registryKey + ":" + lockKey); } private void waitForExpire(String key) throws Exception { - StringRedisTemplate template = this.createTemplate(); + StringRedisTemplate template = createTemplate(); int n = 0; while (n++ < 100 && template.keys(this.registryKey + ":" + key).size() > 0) { Thread.sleep(100);