RedisLockRegistry stopRenew not thread safe

[PiotrDuz]

In what version(s) of Spring Integration are you seeing this issue?

spring-integration-redis 6.5.0

Describe the bug

Basically stopRenew() can be called from multiple threads and results in:
java.lang.NullPointerException: Cannot invoke "java.util.concurrent.ScheduledFuture.cancel(boolean)" because "this.renewFuture" is null at org.springframework.integration.redis.util.RedisLockRegistry$RedisLock.stopRenew(RedisLockRegistry.java:601) ~[spring-integration-redis-6.5.0.jar:6.5.0] at org.springframework.integration.redis.util.RedisLockRegistry$RedisLock.removeLockKey(RedisLockRegistry.java:572) ~[spring-integration-redis-6.5.0.jar:6.5.0] at org.springframework.integration.redis.util.RedisLockRegistry$RedisLock.unlock(RedisLockRegistry.java:536) ~[spring-integration-redis-6.5.0.jar:6.5.0]

To Reproduce

Isn't easy to reproduce, I am not an expert of Redis. BUT by static code analysis I can tell that there is one opportunity for this error to occur.

1. we call unlock(), which calls stopRenew()
2. There is also a taskExecutor that periodically runs renew(). So at the same time renew() could be running in separate thread, and it could happen (?) that
   Boolean.TRUE.equals(RedisLockRegistry.this.redisTemplate.execute( RENEW_REDIS_SCRIPT, Collections.singletonList(this.lockKey), RedisLockRegistry.this.clientId, String.valueOf(RedisLockRegistry.this.expireAfter)));
   returns false, thus we go into stopRenew() and we have a race condition.
   I am not sure why would we get "FALSE" there, but maybe someone here can answer this question?

Expected behavior

I am expecting unlock() to be a safe method, doing it job regardless the internal state of a lock.

Sample

Just happened on production, and as it is a race condition exception I think static analysis should be sufficient.

I will create a PR with proposition to add "synchronised" keyword on stopRenew() method.

Renewable logic is very helpful and was introduced in commit: Commit 4d08e11
Tagging @artembilan , seems like you will be able to tell whether adding "synchronised" is sufficient :)

[rpjicond]

Hi @PiotrDuz, this is a great analysis. The proposed solution of adding synchronized to the stopRenew() method seems correct. I noticed there isn't a PR linked to this issue yet. If you haven't started working on it, I would be happy to implement the fix and work on creating a test case for it. Please let me know.

[PiotrDuz]

I would rather wait for comment from one of maintainers to give a green light on that.

[artembilan]

Thank you for the report and willingness to contribute the fix!
However I don't think that we should go with a synchronized.
Instead I suggest the fix like this:

		protected final void stopRenew() {
			ScheduledFuture<?> renewFutureToCancel = this.renewFuture;
			if (renewFutureToCancel != null) {
				renewFutureToCancel.cancel(true);
				this.renewFuture = null;
			}
		}

It is totally thread-safe and does not require a synchronization.

[rpjicond]

Thank you, @artembilan! The proposed solution looks great. I will implement this fix and create a test case to validate it. I'll submit a pull request shortly.

[PiotrDuz]

@rpjicond waiting for it then :)