Skip to content

Commit

Permalink
GH-3888: Fix NPE in the RedisLockRegistry.destroy() (#3889)
Browse files Browse the repository at this point in the history 
* GH-3888: Fix NPE in the `RedisLockRegistry.destroy()`

Fixed #3888

When `RedisLockType.SPIN_LOCK` (default), the `RedisLockRegistry.destroy()`
causes an NPE on the `redisMessageListenerContainer` since pub-sub is not used
in a busy-spin mode

* Check for `redisMessageListenerContainer` before calling its `destroy()`

**Cherry-pick to 5.5.x**

* * Reset properties in the `RedisLockRegistry` after `destroy()`
  • Loading branch information
@artembilan
artembilan committed Sep 20, 2022
1 parent ab7b579 commit 695e156
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 7 deletions.
14 changes: 9 additions & 5 deletions ...ion-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,11 +245,15 @@ public void destroy() {
if (!this.executorExplicitlySet) {
((ExecutorService) this.executor).shutdown();
}
try {
this.redisMessageListenerContainer.destroy();
}
catch (Exception ex) {
throw new IllegalStateException(ex);
if (this.redisMessageListenerContainer != null) {
try {
this.redisMessageListenerContainer.destroy();
this.redisMessageListenerContainer = null;
this.isRunningRedisMessageListenerContainer = false;
}
catch (Exception ex) {
throw new IllegalStateException(ex);
}
}
}

Expand Down
31 changes: 29 additions & 2 deletions ...edis/src/test/java/org/springframework/integration/redis/util/RedisLockRegistryTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ void testLock(RedisLockType testRedisLockType) {
}
registry.expireUnusedOlderThan(-1000);
assertThat(getRedisLockRegistryLocks(registry)).isEmpty();
registry.destroy();
}

@ParameterizedTest
Expand All @@ -126,6 +127,7 @@ void testLockInterruptibly(RedisLockType testRedisLockType) throws Exception {
}
registry.expireUnusedOlderThan(-1000);
assertThat(getRedisLockRegistryLocks(registry)).isEmpty();
registry.destroy();
}

@ParameterizedTest
Expand Down Expand Up @@ -153,6 +155,7 @@ void testReentrantLock(RedisLockType testRedisLockType) {
}
registry.expireUnusedOlderThan(-1000);
assertThat(getRedisLockRegistryLocks(registry)).isEmpty();
registry.destroy();
}

@ParameterizedTest
Expand Down Expand Up @@ -180,6 +183,7 @@ void testReentrantLockInterruptibly(RedisLockType testRedisLockType) throws Exce
}
registry.expireUnusedOlderThan(-1000);
assertThat(getRedisLockRegistryLocks(registry)).isEmpty();
registry.destroy();
}

@ParameterizedTest
Expand Down Expand Up @@ -207,6 +211,7 @@ void testTwoLocks(RedisLockType testRedisLockType) throws Exception {
}
registry.expireUnusedOlderThan(-1000);
assertThat(getRedisLockRegistryLocks(registry)).isEmpty();
registry.destroy();
}

@ParameterizedTest
Expand Down Expand Up @@ -238,6 +243,7 @@ void testTwoThreadsSecondFailsToGetLock(RedisLockType testRedisLockType) throws
assertThat(((Exception) ise).getMessage()).contains("You do not own lock at");
registry.expireUnusedOlderThan(-1000);
assertThat(getRedisLockRegistryLocks(registry)).isEmpty();
registry.destroy();
}

@ParameterizedTest
Expand Down Expand Up @@ -277,6 +283,7 @@ void testTwoThreads(RedisLockType testRedisLockType) throws Exception {
assertThat(locked.get()).isTrue();
registry.expireUnusedOlderThan(-1000);
assertThat(getRedisLockRegistryLocks(registry)).isEmpty();
registry.destroy();
}

@ParameterizedTest
Expand Down Expand Up @@ -326,6 +333,8 @@ void testTwoThreadsDifferentRegistries(RedisLockType testRedisLockType) throws E
registry2.expireUnusedOlderThan(-1000);
assertThat(getRedisLockRegistryLocks(registry1)).isEmpty();
assertThat(getRedisLockRegistryLocks(registry2)).isEmpty();
registry1.destroy();
registry2.destroy();
}

@ParameterizedTest
Expand Down Expand Up @@ -355,6 +364,7 @@ void testTwoThreadsWrongOneUnlocks(RedisLockType testRedisLockType) throws Excep
assertThat(((Exception) ise).getMessage()).contains("You do not own lock at");
registry.expireUnusedOlderThan(-1000);
assertThat(getRedisLockRegistryLocks(registry)).isEmpty();
registry.destroy();
}

@ParameterizedTest
Expand All @@ -371,6 +381,8 @@ void testExpireTwoRegistries(RedisLockType testRedisLockType) throws Exception {
waitForExpire("foo");
assertThat(lock2.tryLock()).isTrue();
assertThat(lock1.tryLock()).isFalse();
registry1.destroy();
registry2.destroy();
}

@ParameterizedTest
Expand All @@ -384,6 +396,7 @@ void testExceptionOnExpire(RedisLockType testRedisLockType) throws Exception {
assertThatIllegalStateException()
.isThrownBy(lock1::unlock)
.withMessageContaining("Lock was released in the store due to expiration.");
registry.destroy();
}


Expand Down Expand Up @@ -422,6 +435,9 @@ void testEquals(RedisLockType testRedisLockType) {
lock2.lock();
lock1.unlock();
lock2.unlock();
registry1.destroy();
registry2.destroy();
registry3.destroy();
}

@ParameterizedTest
Expand All @@ -446,6 +462,7 @@ void testThreadLocalListLeaks(RedisLockType testRedisLockType) {
lock.unlock();
}
assertThat(getRedisLockRegistryLocks(registry)).hasSize(10);
registry.destroy();
}

@ParameterizedTest
Expand All @@ -468,6 +485,7 @@ void testExpireNotChanged(RedisLockType testRedisLockType) throws Exception {
result.get();
assertThat(getExpire(registry, "foo")).isEqualTo(expire);
lock.unlock();
registry.destroy();
}

@ParameterizedTest
Expand Down Expand Up @@ -510,6 +528,7 @@ void concurrentObtainCapacityTest(RedisLockType testRedisLockType) throws Interr

registry.expireUnusedOlderThan(-1000);
assertThat(getRedisLockRegistryLocks(registry)).isEmpty();
registry.destroy();
}

@ParameterizedTest
Expand Down Expand Up @@ -558,7 +577,8 @@ void concurrentObtainRemoveOrderTest(RedisLockType testRedisLockType) throws Int
executorService.awaitTermination(5, TimeUnit.SECONDS);

assertThat(getRedisLockRegistryLocks(registry)).containsKeys(
remainLockCheckQueue.toArray(new String[remainLockCheckQueue.size()]));
remainLockCheckQueue.toArray(new String[0]));
registry.destroy();
}

@ParameterizedTest
Expand Down Expand Up @@ -613,7 +633,8 @@ void concurrentObtainAccessRemoveOrderTest(RedisLockType testRedisLockType) thro
executorService.awaitTermination(5, TimeUnit.SECONDS);

assertThat(getRedisLockRegistryLocks(registry)).containsKeys(
remainLockCheckQueue.toArray(new String[remainLockCheckQueue.size()]));
remainLockCheckQueue.toArray(new String[0]));
registry.destroy();
}

@ParameterizedTest
Expand Down Expand Up @@ -642,6 +663,7 @@ void setCapacityTest(RedisLockType testRedisLockType) {
registry.obtain("foo:5");
assertThat(getRedisLockRegistryLocks(registry)).hasSize(4);
assertThat(getRedisLockRegistryLocks(registry)).containsKeys("foo:3", "foo:4", "foo:5");
registry.destroy();
}

@ParameterizedTest
Expand Down Expand Up @@ -690,6 +712,8 @@ void twoRedisLockRegistryTest(RedisLockType testRedisLockType) throws Interrupte

assertThat(future1).isNotCompletedExceptionally();
assertThat(future2).isNotCompletedExceptionally();
registry1.destroy();
registry2.destroy();
}

@ParameterizedTest
Expand Down Expand Up @@ -782,6 +806,9 @@ void earlyWakeUpTest(RedisLockType testRedisLockType) throws InterruptedExceptio
assertThat(awaitTimeout.await(1, TimeUnit.SECONDS)).isFalse();
assertThat(expectOne.get()).isEqualTo(1);
executorService.shutdown();
registry1.destroy();
registry2.destroy();
registry3.destroy();
}

private Long getExpire(RedisLockRegistry registry, String lockKey) {
Expand Down

0 comments on commit 695e156

Please sign in to comment.