From 9d73762d5c32ddcf70c3ffcb43020fddd7e862e6 Mon Sep 17 00:00:00 2001 From: Artem Bilan Date: Tue, 20 Sep 2022 14:17:37 -0400 Subject: [PATCH] GH-3888: Fix NPE in the `RedisLockRegistry.destroy()` (#3889) * GH-3888: Fix NPE in the `RedisLockRegistry.destroy()` Fixed https://github.com/spring-projects/spring-integration/issues/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()` --- .../redis/util/RedisLockRegistry.java | 14 ++++++---- .../redis/util/RedisLockRegistryTests.java | 28 +++++++++++++++++++ 2 files changed, 37 insertions(+), 5 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 d3014f3f8e..ad901afb6b 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 @@ -245,11 +245,15 @@ public final class RedisLockRegistry implements ExpirableLockRegistry, Disposabl 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); + } } } 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 713121d2fd..03ece69b19 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 @@ -120,6 +120,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { } registry.expireUnusedOlderThan(-1000); assertThat(TestUtils.getPropertyValue(registry, "locks", Map.class).size()).isEqualTo(0); + registry.destroy(); } @Test @@ -139,6 +140,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { } registry.expireUnusedOlderThan(-1000); assertThat(TestUtils.getPropertyValue(registry, "locks", Map.class).size()).isEqualTo(0); + registry.destroy(); } @Test @@ -166,6 +168,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { } registry.expireUnusedOlderThan(-1000); assertThat(TestUtils.getPropertyValue(registry, "locks", Map.class).size()).isEqualTo(0); + registry.destroy(); } @Test @@ -193,6 +196,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { } registry.expireUnusedOlderThan(-1000); assertThat(TestUtils.getPropertyValue(registry, "locks", Map.class).size()).isEqualTo(0); + registry.destroy(); } @Test @@ -220,6 +224,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { } registry.expireUnusedOlderThan(-1000); assertThat(TestUtils.getPropertyValue(registry, "locks", Map.class).size()).isEqualTo(0); + registry.destroy(); } @Test @@ -251,6 +256,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { assertThat(((Exception) ise).getMessage()).contains("You do not own lock at"); registry.expireUnusedOlderThan(-1000); assertThat(TestUtils.getPropertyValue(registry, "locks", Map.class).size()).isEqualTo(0); + registry.destroy(); } @Test @@ -290,6 +296,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { assertThat(locked.get()).isTrue(); registry.expireUnusedOlderThan(-1000); assertThat(TestUtils.getPropertyValue(registry, "locks", Map.class).size()).isEqualTo(0); + registry.destroy(); } @Test @@ -339,6 +346,8 @@ public class RedisLockRegistryTests extends RedisAvailableTests { registry2.expireUnusedOlderThan(-1000); assertThat(TestUtils.getPropertyValue(registry1, "locks", Map.class).size()).isEqualTo(0); assertThat(TestUtils.getPropertyValue(registry2, "locks", Map.class).size()).isEqualTo(0); + registry1.destroy(); + registry2.destroy(); } @Test @@ -368,6 +377,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { assertThat(((Exception) ise).getMessage()).contains("You do not own lock at"); registry.expireUnusedOlderThan(-1000); assertThat(TestUtils.getPropertyValue(registry, "locks", Map.class).size()).isEqualTo(0); + registry.destroy(); } @Test @@ -384,6 +394,8 @@ public class RedisLockRegistryTests extends RedisAvailableTests { waitForExpire("foo"); assertThat(lock2.tryLock()).isTrue(); assertThat(lock1.tryLock()).isFalse(); + registry1.destroy(); + registry2.destroy(); } @Test @@ -397,6 +409,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { assertThatIllegalStateException() .isThrownBy(lock1::unlock) .withMessageContaining("Lock was released in the store due to expiration."); + registry.destroy(); } @@ -435,6 +448,9 @@ public class RedisLockRegistryTests extends RedisAvailableTests { lock2.lock(); lock1.unlock(); lock2.unlock(); + registry1.destroy(); + registry2.destroy(); + registry3.destroy(); } @Test @@ -459,6 +475,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { lock.unlock(); } assertThat(TestUtils.getPropertyValue(registry, "locks", Map.class).size()).isEqualTo(10); + registry.destroy(); } @Test @@ -481,6 +498,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { result.get(); assertThat(getExpire(registry, "foo")).isEqualTo(expire); lock.unlock(); + registry.destroy(); } @Test @@ -523,6 +541,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { registry.expireUnusedOlderThan(-1000); assertThat(TestUtils.getPropertyValue(registry, "locks", Map.class).size()).isEqualTo(0); + registry.destroy(); } @Test @@ -572,6 +591,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { assertThat(getRedisLockRegistryLocks(registry)).containsKeys( remainLockCheckQueue.toArray(new String[remainLockCheckQueue.size()])); + registry.destroy(); } @Test @@ -627,6 +647,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { assertThat(getRedisLockRegistryLocks(registry)).containsKeys( remainLockCheckQueue.toArray(new String[remainLockCheckQueue.size()])); + registry.destroy(); } @Test @@ -655,6 +676,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { registry.obtain("foo:5"); assertThat(TestUtils.getPropertyValue(registry, "locks", Map.class).size()).isEqualTo(4); assertThat(getRedisLockRegistryLocks(registry)).containsKeys("foo:3", "foo:4", "foo:5"); + registry.destroy(); } @RedisAvailable @@ -701,6 +723,8 @@ public class RedisLockRegistryTests extends RedisAvailableTests { }); endDownLatch.await(); + registry1.destroy(); + registry2.destroy(); } @RedisAvailable @@ -795,6 +819,9 @@ public class RedisLockRegistryTests extends RedisAvailableTests { assertThat(awaitTimeout.await(1, TimeUnit.SECONDS)).isFalse(); assertThat(expectOne.get()).isEqualTo(1); executorService.shutdown(); + registry1.destroy(); + registry2.destroy(); + registry3.destroy(); } @@ -812,6 +839,7 @@ public class RedisLockRegistryTests extends RedisAvailableTests { registry = new RedisLockRegistry(mock(RedisConnectionFactory.class), "foo"); registry.setRedisLockType(testRedisLockType); assertThat(TestUtils.getPropertyValue(registry, "ulinkAvailable", Boolean.class)).isTrue(); + registry.destroy(); } private Long getExpire(RedisLockRegistry registry, String lockKey) {