Skip to content

Commit c5f29fc

Browse files
artembilangaryrussell
artembilan
authored and
garyrussell
committed
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
1 parent 3f522f7 commit c5f29fc

File tree

2 files changed

+74
-55
lines changed

2 files changed

+74
-55
lines changed

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

+27-21
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,18 @@ public final class RedisLockRegistry implements ExpirableLockRegistry, Disposabl
9292
"return false";
9393

9494

95+
private final Map<String, RedisLock> locks = new ConcurrentHashMap<>();
96+
97+
private final String clientId = UUID.randomUUID().toString();
98+
99+
private final String registryKey;
100+
101+
private final StringRedisTemplate redisTemplate;
102+
103+
private final RedisScript<Boolean> obtainLockScript;
104+
105+
private final long expireAfter;
106+
95107
/**
96108
* An {@link ExecutorService} to call {@link StringRedisTemplate#delete(Object)} in
97109
* the separate thread when the current one is interrupted.
@@ -105,18 +117,6 @@ public final class RedisLockRegistry implements ExpirableLockRegistry, Disposabl
105117
*/
106118
private boolean executorExplicitlySet;
107119

108-
private final Map<String, RedisLock> locks = new ConcurrentHashMap<>();
109-
110-
private final String clientId = UUID.randomUUID().toString();
111-
112-
private final String registryKey;
113-
114-
private final StringRedisTemplate redisTemplate;
115-
116-
private final RedisScript<Boolean> obtainLockScript;
117-
118-
private final long expireAfter;
119-
120120
/**
121121
* Constructs a lock registry with the default (60 second) lock expiration.
122122
* @param connectionFactory The connection factory.
@@ -282,13 +282,17 @@ public boolean tryLock(long time, TimeUnit unit) throws InterruptedException {
282282
}
283283

284284
private boolean obtainLock() {
285-
boolean success = RedisLockRegistry.this.redisTemplate.execute(RedisLockRegistry.this.obtainLockScript,
286-
Collections.singletonList(this.lockKey), RedisLockRegistry.this.clientId,
287-
String.valueOf(RedisLockRegistry.this.expireAfter));
288-
if (success) {
285+
Boolean success =
286+
RedisLockRegistry.this.redisTemplate.execute(RedisLockRegistry.this.obtainLockScript,
287+
Collections.singletonList(this.lockKey), RedisLockRegistry.this.clientId,
288+
String.valueOf(RedisLockRegistry.this.expireAfter));
289+
290+
boolean result = Boolean.TRUE.equals(success);
291+
292+
if (result) {
289293
this.lockedAt = System.currentTimeMillis();
290294
}
291-
return success;
295+
return result;
292296
}
293297

294298
@Override
@@ -301,6 +305,11 @@ public void unlock() {
301305
return;
302306
}
303307
try {
308+
if (!isAcquiredInThisProcess()) {
309+
throw new IllegalStateException("Lock was released in the store due to expiration. " +
310+
"The integrity of data protected by this lock may have been compromised.");
311+
}
312+
304313
if (Thread.currentThread().isInterrupted()) {
305314
RedisLockRegistry.this.executor.execute(this::removeLockKey);
306315
}
@@ -377,10 +386,7 @@ public boolean equals(Object obj) {
377386
if (!this.lockKey.equals(other.lockKey)) {
378387
return false;
379388
}
380-
if (this.lockedAt != other.lockedAt) {
381-
return false;
382-
}
383-
return true;
389+
return this.lockedAt == other.lockedAt;
384390
}
385391

386392
private RedisLockRegistry getOuterType() {

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

+47-34
Original file line numberDiff line numberDiff line change
@@ -78,13 +78,13 @@ public void setupShutDown() {
7878
}
7979

8080
private StringRedisTemplate createTemplate() {
81-
return new StringRedisTemplate(this.getConnectionFactoryForTest());
81+
return new StringRedisTemplate(getConnectionFactoryForTest());
8282
}
8383

8484
@Test
8585
@RedisAvailable
86-
public void testLock() throws Exception {
87-
RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey);
86+
public void testLock() {
87+
RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey);
8888
for (int i = 0; i < 10; i++) {
8989
Lock lock = registry.obtain("foo");
9090
lock.lock();
@@ -102,7 +102,7 @@ public void testLock() throws Exception {
102102
@Test
103103
@RedisAvailable
104104
public void testLockInterruptibly() throws Exception {
105-
RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey);
105+
RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey);
106106
for (int i = 0; i < 10; i++) {
107107
Lock lock = registry.obtain("foo");
108108
lock.lockInterruptibly();
@@ -119,8 +119,8 @@ public void testLockInterruptibly() throws Exception {
119119

120120
@Test
121121
@RedisAvailable
122-
public void testReentrantLock() throws Exception {
123-
RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey);
122+
public void testReentrantLock() {
123+
RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey);
124124
for (int i = 0; i < 10; i++) {
125125
Lock lock1 = registry.obtain("foo");
126126
lock1.lock();
@@ -146,7 +146,7 @@ public void testReentrantLock() throws Exception {
146146
@Test
147147
@RedisAvailable
148148
public void testReentrantLockInterruptibly() throws Exception {
149-
RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey);
149+
RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey);
150150
for (int i = 0; i < 10; i++) {
151151
Lock lock1 = registry.obtain("foo");
152152
lock1.lockInterruptibly();
@@ -172,7 +172,7 @@ public void testReentrantLockInterruptibly() throws Exception {
172172
@Test
173173
@RedisAvailable
174174
public void testTwoLocks() throws Exception {
175-
RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey);
175+
RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey);
176176
for (int i = 0; i < 10; i++) {
177177
Lock lock1 = registry.obtain("foo");
178178
lock1.lockInterruptibly();
@@ -198,7 +198,7 @@ public void testTwoLocks() throws Exception {
198198
@Test
199199
@RedisAvailable
200200
public void testTwoThreadsSecondFailsToGetLock() throws Exception {
201-
final RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey);
201+
final RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey);
202202
final Lock lock1 = registry.obtain("foo");
203203
lock1.lockInterruptibly();
204204
final AtomicBoolean locked = new AtomicBoolean();
@@ -228,12 +228,12 @@ public void testTwoThreadsSecondFailsToGetLock() throws Exception {
228228
@Test
229229
@RedisAvailable
230230
public void testTwoThreads() throws Exception {
231-
final RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey);
232-
final Lock lock1 = registry.obtain("foo");
233-
final AtomicBoolean locked = new AtomicBoolean();
234-
final CountDownLatch latch1 = new CountDownLatch(1);
235-
final CountDownLatch latch2 = new CountDownLatch(1);
236-
final CountDownLatch latch3 = new CountDownLatch(1);
231+
RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey);
232+
Lock lock1 = registry.obtain("foo");
233+
AtomicBoolean locked = new AtomicBoolean();
234+
CountDownLatch latch1 = new CountDownLatch(1);
235+
CountDownLatch latch2 = new CountDownLatch(1);
236+
CountDownLatch latch3 = new CountDownLatch(1);
237237
lock1.lockInterruptibly();
238238
assertEquals(1, TestUtils.getPropertyValue(registry, "locks", Map.class).size());
239239
Executors.newSingleThreadExecutor().execute(() -> {
@@ -266,13 +266,13 @@ public void testTwoThreads() throws Exception {
266266
@Test
267267
@RedisAvailable
268268
public void testTwoThreadsDifferentRegistries() throws Exception {
269-
final RedisLockRegistry registry1 = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey);
270-
final RedisLockRegistry registry2 = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey);
271-
final Lock lock1 = registry1.obtain("foo");
272-
final AtomicBoolean locked = new AtomicBoolean();
273-
final CountDownLatch latch1 = new CountDownLatch(1);
274-
final CountDownLatch latch2 = new CountDownLatch(1);
275-
final CountDownLatch latch3 = new CountDownLatch(1);
269+
RedisLockRegistry registry1 = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey);
270+
RedisLockRegistry registry2 = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey);
271+
Lock lock1 = registry1.obtain("foo");
272+
AtomicBoolean locked = new AtomicBoolean();
273+
CountDownLatch latch1 = new CountDownLatch(1);
274+
CountDownLatch latch2 = new CountDownLatch(1);
275+
CountDownLatch latch3 = new CountDownLatch(1);
276276
lock1.lockInterruptibly();
277277
assertEquals(1, TestUtils.getPropertyValue(registry1, "locks", Map.class).size());
278278
Executors.newSingleThreadExecutor().execute(() -> {
@@ -313,11 +313,11 @@ public void testTwoThreadsDifferentRegistries() throws Exception {
313313
@Test
314314
@RedisAvailable
315315
public void testTwoThreadsWrongOneUnlocks() throws Exception {
316-
final RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey);
317-
final Lock lock = registry.obtain("foo");
316+
RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey);
317+
Lock lock = registry.obtain("foo");
318318
lock.lockInterruptibly();
319-
final AtomicBoolean locked = new AtomicBoolean();
320-
final CountDownLatch latch = new CountDownLatch(1);
319+
AtomicBoolean locked = new AtomicBoolean();
320+
CountDownLatch latch = new CountDownLatch(1);
321321
Future<Object> result = Executors.newSingleThreadExecutor().submit(() -> {
322322
try {
323323
lock.unlock();
@@ -341,8 +341,8 @@ public void testTwoThreadsWrongOneUnlocks() throws Exception {
341341
@Test
342342
@RedisAvailable
343343
public void testExpireTwoRegistries() throws Exception {
344-
RedisLockRegistry registry1 = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey, 100);
345-
RedisLockRegistry registry2 = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey, 100);
344+
RedisLockRegistry registry1 = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey, 1);
345+
RedisLockRegistry registry2 = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey, 1);
346346
Lock lock1 = registry1.obtain("foo");
347347
Lock lock2 = registry2.obtain("foo");
348348
assertTrue(lock1.tryLock());
@@ -354,8 +354,21 @@ public void testExpireTwoRegistries() throws Exception {
354354

355355
@Test
356356
@RedisAvailable
357-
public void testEquals() throws Exception {
358-
RedisConnectionFactory connectionFactory = this.getConnectionFactoryForTest();
357+
public void testExceptionOnExpire() throws Exception {
358+
RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey, 1);
359+
Lock lock1 = registry.obtain("foo");
360+
assertTrue(lock1.tryLock());
361+
this.thrown.expect(IllegalStateException.class);
362+
this.thrown.expectMessage("Lock was released in the store due to expiration.");
363+
waitForExpire("foo");
364+
lock1.unlock();
365+
}
366+
367+
368+
@Test
369+
@RedisAvailable
370+
public void testEquals() {
371+
RedisConnectionFactory connectionFactory = getConnectionFactoryForTest();
359372
RedisLockRegistry registry1 = new RedisLockRegistry(connectionFactory, this.registryKey);
360373
RedisLockRegistry registry2 = new RedisLockRegistry(connectionFactory, this.registryKey);
361374
RedisLockRegistry registry3 = new RedisLockRegistry(connectionFactory, this.registryKey2);
@@ -388,7 +401,7 @@ public void testEquals() throws Exception {
388401
@Test
389402
@RedisAvailable
390403
public void testThreadLocalListLeaks() {
391-
RedisLockRegistry registry = new RedisLockRegistry(this.getConnectionFactoryForTest(), this.registryKey, 100);
404+
RedisLockRegistry registry = new RedisLockRegistry(getConnectionFactoryForTest(), this.registryKey, 100);
392405

393406
for (int i = 0; i < 10; i++) {
394407
registry.obtain("foo" + i);
@@ -411,7 +424,7 @@ public void testThreadLocalListLeaks() {
411424
@Test
412425
@RedisAvailable
413426
public void testExpireNotChanged() throws Exception {
414-
RedisConnectionFactory connectionFactory = this.getConnectionFactoryForTest();
427+
RedisConnectionFactory connectionFactory = getConnectionFactoryForTest();
415428
final RedisLockRegistry registry = new RedisLockRegistry(connectionFactory, this.registryKey, 10000);
416429
Lock lock = registry.obtain("foo");
417430
lock.lock();
@@ -429,13 +442,13 @@ public void testExpireNotChanged() throws Exception {
429442
}
430443

431444
private Long getExpire(RedisLockRegistry registry, String lockKey) {
432-
StringRedisTemplate template = this.createTemplate();
445+
StringRedisTemplate template = createTemplate();
433446
String registryKey = TestUtils.getPropertyValue(registry, "registryKey", String.class);
434447
return template.getExpire(registryKey + ":" + lockKey);
435448
}
436449

437450
private void waitForExpire(String key) throws Exception {
438-
StringRedisTemplate template = this.createTemplate();
451+
StringRedisTemplate template = createTemplate();
439452
int n = 0;
440453
while (n++ < 100 && template.keys(this.registryKey + ":" + key).size() > 0) {
441454
Thread.sleep(100);

0 commit comments

Comments
 (0)