Skip to content

Commit 684e481

Browse files
committed
GH-11146: LockRegistry: prevent collision on mask
Fixes: #11146 When a thread simultaneously holds two `JdbcLock` instances for different lock keys that happen to map to the same bucket in the 256-slot `DefaultLockRegistry`, the `getHoldCount() > 1` guard in `JdbcLock.unlock()` incorrectly fires. The result is that the `APP_LOCK` database entry for the second lock is never deleted, leaving other threads waiting indefinitely until the TTL expires. * Use per `JdbcLock` (and `RedisLock`) instance `holdCount` property instead, so different keys landing in the same `delegate` would still have their own hold count.
1 parent ebbf785 commit 684e481

4 files changed

Lines changed: 125 additions & 2 deletions

File tree

spring-integration-jdbc/src/main/java/org/springframework/integration/jdbc/lock/JdbcLockRegistry.java

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,8 @@ private final class JdbcLock implements Lock {
173173

174174
private final ReentrantLock delegate;
175175

176+
private int holdCount;
177+
176178
JdbcLock(LockRepository client, Duration idleBetweenTries, String path) {
177179
this.mutex = client;
178180
this.idleBetweenTries = idleBetweenTries;
@@ -192,6 +194,7 @@ public void lock() {
192194
while (!doLock()) {
193195
Thread.sleep(this.idleBetweenTries.toMillis());
194196
}
197+
this.holdCount++;
195198
break;
196199
}
197200
catch (TransientDataAccessException | TransactionTimedOutException | TransactionSystemException e) {
@@ -226,6 +229,7 @@ public void lockInterruptibly() throws InterruptedException {
226229
throw new InterruptedException();
227230
}
228231
}
232+
this.holdCount++;
229233
break;
230234
}
231235
catch (TransientDataAccessException | TransactionTimedOutException | TransactionSystemException e) {
@@ -270,6 +274,9 @@ public boolean tryLock(long time, TimeUnit unit) throws InterruptedException {
270274
if (!acquired) {
271275
this.delegate.unlock();
272276
}
277+
else {
278+
this.holdCount++;
279+
}
273280
return acquired;
274281
}
275282
catch (TransientDataAccessException | TransactionTimedOutException | TransactionSystemException e) {
@@ -295,7 +302,8 @@ public void unlock() {
295302
if (!this.delegate.isHeldByCurrentThread()) {
296303
throw new IllegalMonitorStateException("The current thread doesn't own mutex at " + this.path);
297304
}
298-
if (this.delegate.getHoldCount() > 1) {
305+
if (this.holdCount > 1) {
306+
this.holdCount--;
299307
this.delegate.unlock();
300308
return;
301309
}
@@ -322,6 +330,7 @@ public void unlock() {
322330
}
323331
}
324332
finally {
333+
this.holdCount--;
325334
this.delegate.unlock();
326335
}
327336
}

spring-integration-jdbc/src/test/java/org/springframework/integration/jdbc/lock/JdbcLockRegistryTests.java

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -601,6 +601,63 @@ void noSecondLockOnEviction() throws InterruptedException {
601601
});
602602
}
603603

604+
@Test
605+
void sharedDelegateIsDeletedFromDbOnUnlock() throws Exception {
606+
DefaultLockRepository client1 = newLockRepository();
607+
DefaultLockRepository client2 = newLockRepository();
608+
609+
JdbcLockRegistry registry1 = new JdbcLockRegistry(client1);
610+
// setCacheCapacity(2) gives mask=1 in DefaultLockRegistry → only 2 slots, guaranteed collision in ≤3 keys
611+
registry1.setCacheCapacity(2);
612+
JdbcLockRegistry registry2 = new JdbcLockRegistry(client2);
613+
614+
// Find two distinct lock keys that share the same ReentrantLock (same DefaultLockRegistry slot)
615+
String keyB = null;
616+
DistributedLock lockA = null;
617+
DistributedLock lockB = null;
618+
outer:
619+
for (int i = 0; i < 10; i++) {
620+
for (int j = i + 1; j < 10; j++) {
621+
keyB = "key-" + j;
622+
DistributedLock la = registry1.obtain("key-" + i);
623+
DistributedLock lb = registry1.obtain(keyB);
624+
if (TestUtils.getPropertyValue(la, "delegate") == TestUtils.getPropertyValue(lb, "delegate")) {
625+
626+
lockA = la;
627+
lockB = lb;
628+
break outer;
629+
}
630+
}
631+
}
632+
assertThat(lockA)
633+
.as("Could not find two lock keys mapping to the same DefaultLockRegistry slot")
634+
.isNotNull();
635+
636+
lockA.lock();
637+
// Shared delegate: delegate.holdCount becomes 2 after this call
638+
lockB.lock();
639+
640+
// Unlock B first — before the fix, delegate.getHoldCount()>1 short-circuited and skipped the DB delete
641+
lockB.unlock();
642+
643+
// Another process must be able to acquire keyB immediately (its DB row must be deleted)
644+
DistributedLock lockBOtherProcess = registry2.obtain(keyB);
645+
assertThat(lockBOtherProcess.tryLock(500, TimeUnit.MILLISECONDS))
646+
.as("DB row for '" + keyB + "' was not deleted on unlock — orphaned lock detected")
647+
.isTrue();
648+
lockBOtherProcess.unlock();
649+
650+
assertThatNoException().isThrownBy(lockA::unlock);
651+
}
652+
653+
private DefaultLockRepository newLockRepository() {
654+
DefaultLockRepository client = new DefaultLockRepository(this.dataSource);
655+
client.setApplicationContext(this.context);
656+
client.afterPropertiesSet();
657+
client.afterSingletonsInstantiated();
658+
return client;
659+
}
660+
604661
@SuppressWarnings("unchecked")
605662
private static Map<String, Lock> getRegistryLocks(JdbcLockRegistry registry) {
606663
return TestUtils.getPropertyValue(registry, "locks", Map.class);

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -393,6 +393,8 @@ private abstract class RedisLock implements Lock {
393393

394394
private final ReentrantLock localLock;
395395

396+
private int holdCount;
397+
396398
private volatile long lockedAt;
397399

398400
private volatile ScheduledFuture<?> renewFuture;
@@ -435,6 +437,7 @@ public final void lock() {
435437
while (true) {
436438
try {
437439
if (tryRedisLock(-1L)) {
440+
this.holdCount++;
438441
return;
439442
}
440443
}
@@ -462,6 +465,7 @@ public final void lockInterruptibly() throws InterruptedException {
462465
while (true) {
463466
try {
464467
if (tryRedisLock(-1L)) {
468+
this.holdCount++;
465469
return;
466470
}
467471
}
@@ -499,6 +503,9 @@ public final boolean tryLock(long time, TimeUnit unit) throws InterruptedExcepti
499503
if (!acquired) {
500504
this.localLock.unlock();
501505
}
506+
else {
507+
this.holdCount++;
508+
}
502509
return acquired;
503510
}
504511
catch (Exception e) {
@@ -536,7 +543,8 @@ public final void unlock() {
536543
if (!this.localLock.isHeldByCurrentThread()) {
537544
throw new IllegalStateException("You do not own lock at " + this.lockKey);
538545
}
539-
if (this.localLock.getHoldCount() > 1) {
546+
if (this.holdCount > 1) {
547+
this.holdCount--;
540548
this.localLock.unlock();
541549
return;
542550
}
@@ -581,6 +589,7 @@ public final void unlock() {
581589
ReflectionUtils.rethrowRuntimeException(e);
582590
}
583591
finally {
592+
this.holdCount--;
584593
this.localLock.unlock();
585594
}
586595
}

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1066,6 +1066,54 @@ private void waitForExpire(String key) throws Exception {
10661066
assertThat(n < 100).as(key + " key did not expire").isTrue();
10671067
}
10681068

1069+
@Test
1070+
void sharedDelegateIsDeletedFromRedisOnUnlock() throws Exception {
1071+
RedisLockRegistry registry1 = new RedisLockRegistry(redisConnectionFactory, this.registryKey);
1072+
// setCacheCapacity(2) gives mask=1 in DefaultLockRegistry → only 2 slots, guaranteed collision in ≤3 keys
1073+
registry1.setCacheCapacity(2);
1074+
RedisLockRegistry registry2 = new RedisLockRegistry(redisConnectionFactory, this.registryKey);
1075+
1076+
// Find two distinct lock keys that share the same ReentrantLock (same DefaultLockRegistry slot)
1077+
String keyB = null;
1078+
DistributedLock lockA = null;
1079+
DistributedLock lockB = null;
1080+
outer:
1081+
for (int i = 0; i < 10; i++) {
1082+
for (int j = i + 1; j < 10; j++) {
1083+
DistributedLock la = registry1.obtain("key-" + i);
1084+
DistributedLock lb = registry1.obtain("key-" + j);
1085+
if (TestUtils.getPropertyValue(la, "localLock") == TestUtils.getPropertyValue(lb, "localLock")) {
1086+
keyB = "key-" + j;
1087+
lockA = la;
1088+
lockB = lb;
1089+
break outer;
1090+
}
1091+
}
1092+
}
1093+
assertThat(lockA)
1094+
.as("Could not find two lock keys mapping to the same DefaultLockRegistry slot")
1095+
.isNotNull();
1096+
1097+
lockA.lock();
1098+
// Shared localLock: localLock.holdCount becomes 2 after this call
1099+
lockB.lock();
1100+
1101+
// Unlock B first — before the fix, localLock.getHoldCount()>1 short-circuited and skipped the Redis delete
1102+
lockB.unlock();
1103+
1104+
// Another process must be able to acquire keyB immediately (its Redis key must be deleted)
1105+
DistributedLock lockBOtherProcess = registry2.obtain(keyB);
1106+
assertThat(lockBOtherProcess.tryLock(500, TimeUnit.MILLISECONDS))
1107+
.as("Redis key for '" + keyB + "' was not deleted on unlock — orphaned lock detected")
1108+
.isTrue();
1109+
lockBOtherProcess.unlock();
1110+
1111+
assertThatNoException().isThrownBy(lockA::unlock);
1112+
1113+
registry1.destroy();
1114+
registry2.destroy();
1115+
}
1116+
10691117
@SuppressWarnings("unchecked")
10701118
private static Map<String, Lock> getRedisLockRegistryLocks(RedisLockRegistry registry) {
10711119
return TestUtils.getPropertyValue(registry, "locks", Map.class);

0 commit comments

Comments
 (0)