RedisLockRegistry setExecutor nullPointerException

[nbonavia]

In what version(s) of Spring Integration are you seeing this issue?
spring-integration-redis:6.2.1

Describe the bug

By looking at the RedisLockRegistry (https://github.com/spring-projects/spring-integration/blob/main/spring-integration-redis/src/main/java/org/springframework/integration/redis/util/RedisLockRegistry.java), the setExecutor method attempts to set the

this.redisMessageListenerContainer.setTaskExecutor(this.executor);
this.redisMessageListenerContainer.setSubscriptionExecutor(this.executor);

Since redisMessgeListenerContainer is lazy initialised, it's likely that the setExecutor method will fail since it does not have any null checking for this.redisMessageListenerContainer.

To Reproduce

If you run the below code snippet the execution will fail due to a NullPointerException (

RedisLockRegistry redisLockRegistry = 
                                                new RedisLockRegistry(redisConnectionFactory, lockRegistryKey):

redisLockRegistry.setRedisLockType(RedisLockRegistry.RedisLockType.PUB_SUB_LOCK);
redisLockRegistry.setExecutor(Executors.newFixedThreadPool(100));

Expected behavior

I would expect that setting the executor will have a null check for redisMessageListenerContainer as shown in the code below: -

public void setExecutor(Executor executor) {
  this.executor = executor;
  this.executorExplicitlySet = true;
  if (this.redisMessageListenerContainer != null) {
    this.redisMessageListenerContainer.setTaskExecutor(this.executor);
    this.redisMessageListenerContainer.setSubscriptionExecutor(this.executor);
  }
}

Sample

RedisLockRegistry redisLockRegistry = 
                                                new RedisLockRegistry(redisConnectionFactory, lockRegistryKey):

redisLockRegistry.setRedisLockType(RedisLockRegistry.RedisLockType.PUB_SUB_LOCK);
redisLockRegistry.setExecutor(Executors.newFixedThreadPool(100));

[artembilan]

Confirmed.
Those two calls from setExecutor() are redundant and must be removed.
See setupUnlockMessageListener() method before.

Please, consider to contribute the fix: https://github.com/spring-projects/spring-integration/blob/main/CONTRIBUTING.adoc

Thanks

[nbonavia]

Confirmed. Those two calls from setExecutor() are redundant and must be removed. See setupUnlockMessageListener() method before.

Please, consider to contribute the fix: https://github.com/spring-projects/spring-integration/blob/main/CONTRIBUTING.adoc

Thanks

@artembilan , I will create a pull request with the change, however I still think there's value in keeping the calls in setExecutor just in case someone calls the setExecutor() method after the setupUnlockMessageListener() has already been invoked, this way the executor will not propagate down to the redisMessageListenerContainer.

Do you agree?
Neville.

[artembilan]

Well, I prefer to stay away from such a pattern where we try to mutate the configuration when the component has already started.
Therefore setters must be called only once during configuration phase.
This way we would have only one source of truth.
Sure! What you suggest is a simple fix, but that might lead to some undesired encouragement for consumers of our API that they may change those properties at runtime.

WDYT?

[nbonavia]

Well, I prefer to stay away from such a pattern where we try to mutate the configuration when the component has already started. Therefore setters must be called only once during configuration phase. This way we would have only one source of truth. Sure! What you suggest is a simple fix, but that might lead to some undesired encouragement for consumers of our API that they may change those properties at runtime.

WDYT?

I agree with you, and I do not have a use case when you would want to change the executor on an already running instance, however would you agree with me that we should throw an IllegalStateException if the redisMessageListenerContainer is not null just in case someone tries to set the Executor outside the configuration state.

Basically, I would rather fail the process than having an inconsistent behaviour.

[artembilan]

Well, that might makes sense, but then we would need to have such a guard everywhere in the project since any setter may suffer from same consequences.
I think it is better to formally agree that setters are immutable after configuration phase, rather then overcomplicate our code with noisy errors just to make consumer of our API to feel uncomfortable.

It is really a bad practice to try to mutate components state at runtime. An anti-pattern if you wish.

[nbonavia]

Well, that might makes sense, but then we would need to have such a guard everywhere in the project since any setter may suffer from same consequences. I think it is better to formally agree that setters are immutable after configuration phase, rather then overcomplicate our code with noisy errors just to make consumer of our API to feel uncomfortable.

It is really a bad practice to try to mutate components state at runtime. An anti-pattern if you wish.

I see your point. I'll create a Pull request with an initialisation test to make sure that executors can be set up and remove the said lines from the setter method as you recommended.

Thanks.